Merge upstream/main (aac4aed489d1) — header trim, jsCast removal, JSScriptFetcher removal#199
Conversation
https://bugs.webkit.org/show_bug.cgi?id=210791 rdar://170479667 Reviewed by nobody. In the in iteration of this fix we applied the new logic, which allowed percent height descendants to resolve against ancestor flex items depending on which phase of flex layout we are in, to work in both standards in quirks mode. However, this ended up causing problems for documents in standards mode even though the bug we were attempting to fix was in quirks mode. So in this patch we can just make this fix more scoped to only target documents in quirks mode. Originally-landed-as: 310254@main (aeb2ead). rdar://170479667 * Source/WebCore/rendering/RenderFlexibleBox.cpp: (WebCore::RenderFlexibleBox::flexItemHasPercentHeightDescendants const): In some cases, percent height descendants can get registered on the flexbox rather than the flex item. Before this patch, that meant skipContainingBlockForPercentHeightCalculation would always return true for those flex items. However, now we may get a different value for that flex item depending on which phase of flex layout we are in. Instead, if the flexbox has percent height descendants registered then we need to always check if the flex item is the containing block for that descendant. Canonical link: https://commits.webkit.org/311739@main
https://bugs.webkit.org/show_bug.cgi?id=312855 Reviewed by Anne van Kesteren and Rupin Mittal. - Extract synchronizeStyleAttributeForSelectorInvalidation() to eliminate duplicated style attribute synchronization logic between dirtyStyleAttribute() and invalidateStyleAttribute(). - Remove redundant isSVGElement() guard around dynamicDowncast<SVGElement>, which already performs the same type check. - Avoid unnecessary inlineStyleChanged() in setInlineStyleCustomProperty() when addParsedProperty() reports no changes were made. - Cache inlineStyle() in inlineStyleCSSOMWrapper() to avoid double lookup, and use nullptr instead of 0. * Source/WebCore/dom/StyledElement.cpp: (WebCore::StyledElement::inlineStyleCSSOMWrapper): (WebCore::StyledElement::synchronizeStyleAttributeForSelectorInvalidation): (WebCore::StyledElement::dirtyStyleAttribute): (WebCore::StyledElement::invalidateStyleAttribute): (WebCore::StyledElement::setInlineStyleCustomProperty): * Source/WebCore/dom/StyledElement.h: Canonical link: https://commits.webkit.org/311740@main
… flaky text failure rdar://175302325 https://bugs.webkit.org/show_bug.cgi?id=312949 Unreviewed test gardening. * LayoutTests/platform/ios/TestExpectations: Canonical link: https://commits.webkit.org/311741@main
…anceFairPlayStreamingAVFObjC keySystem identifier <https://bugs.webkit.org/show_bug.cgi?id=306902> <rdar://169182803> Reviewed by David Kilzer. Use `MAKE_STATIC_STRING_IMPL` for the FairPlay keySystem string to place the `StaticStringImpl` in the `__DATA` section instead of on the heap. This matches the pattern already used by `CDMInstanceClearKey::keySystem()`. Heap-allocated `StringImpl` objects are not MTE-tagged, making them potentially exploitable via arbitrary decrement primitives in the GPU process. * Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm: (WebCore::CDMInstanceFairPlayStreamingAVFObjC::keySystem const): Originally-landed-as: 305413.263@safari-7624-branch (6995e81). rdar://173968929 Canonical link: https://commits.webkit.org/311742@main
https://bugs.webkit.org/show_bug.cgi?id=306377 <rdar://168927397> Reviewed by Alan Baradlay. The text-wrap-pretty algorithm was storing indices into the breakOpportunities array in InlineItemPosition::index, but this field expects inline item indices. This type confusion caused out-of-bounds access when hyphenation created additional breaks. This patch fixes the crash by: 1. Storing actual inline item indices (breakOpportunities[i]) instead of breakOpportunities array indices in state[].lineEnd 2. Adding bounds checking before accessing m_inlineItemList 3. Fixing unsigned integer underflow in hasEnoughItemsForNextLine check by rewriting the comparison to use addition instead of subtraction * LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6-expected.txt: Added. * LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html: Added. * Source/WebCore/layout/formattingContexts/inline/InlineContentConstrainer.cpp: (WebCore::Layout::InlineContentConstrainer::layoutSingleLineForPretty): (WebCore::Layout::InlineContentConstrainer::prettifyRange): Originally-landed-as: 305413.232@safari-7624-branch (551e424). rdar://173968920 Canonical link: https://commits.webkit.org/311743@main
…base element URL https://bugs.webkit.org/show_bug.cgi?id=312880 Reviewed by Anne van Kesteren. The HTMLPreloadScanner already tracks the predicted base element URL from <base> tags and passes it when creating preload requests for images, scripts, and stylesheets. However, the CSSPreloadScanner was not receiving this URL, so relative @import URLs were resolved against an empty base URL. This fell back to document.baseURL() which may not yet reflect <base> tags the preload scanner has already seen, since the main parser runs behind the preload scanner. Fix this by passing m_predictedBaseElementURL through to CSSPreloadScanner::scan() and using it when creating PreloadRequests for @import rules. Test: http/tests/preload/css-import-base-tag.html * LayoutTests/http/tests/preload/css-import-base-tag-expected.txt: Added. * LayoutTests/http/tests/preload/css-import-base-tag.html: Added. * LayoutTests/http/tests/preload/resources/base-tag-import.css: Added. (body): * Source/WebCore/html/parser/CSSPreloadScanner.cpp: (WebCore::CSSPreloadScanner::scan): (WebCore::CSSPreloadScanner::emitRule): * Source/WebCore/html/parser/CSSPreloadScanner.h: * Source/WebCore/html/parser/HTMLPreloadScanner.cpp: (WebCore::TokenPreloadScanner::scan): Canonical link: https://commits.webkit.org/311744@main
…t cannot access https://bugs.webkit.org/show_bug.cgi?id=306827 rdar://168696983 Reviewed by Chris Dumez. This will prevent local file loads from succeeding when the WebContent process does not have access to the file, but the Networking process does. * Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp: (WebKit::NetworkResourceLoadParameters::createSandboxExtensionHandlesIfNecessary): * Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h: * Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp: (WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess): Originally-landed-as: 305413.239@safari-7624-branch (b87ed58). rdar://173968931 Canonical link: https://commits.webkit.org/311745@main
https://bugs.webkit.org/show_bug.cgi?id=312876 Reviewed by Anne van Kesteren. The HTML spec defines a 'dirtiness' concept for option elements: once option.selected is set via the IDL (JavaScript), the option becomes "dirty" and subsequent changes to the selected content attribute should not affect selectedness. This was missing in WebKit, causing two WPT test failures. Spec: https://html.spec.whatwg.org/multipage/form-elements.html#concept-option-dirtiness The implementation follows Blink's approach: use ImplementedAs in the IDL to separate the JS binding setter (which sets the dirty flag) from the internal setSelected() method (which does not). In attributeChanged, the selected attribute only updates selectedness when the option is not dirty. Form reset clears the dirty flag. For web compatibility, dirtiness is not set when the option is owned by a select element and the selectedness did not actually change. This matches Blink behavior (https://crbug.com/570367). No new tests, rebaselined existing WPT test. This test was already passing in both Chrome and Firefox. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-selected-expected.txt: * Source/WebCore/html/HTMLOptionElement.cpp: (WebCore::HTMLOptionElement::attributeChanged): (WebCore::HTMLOptionElement::selectedForBindings const): (WebCore::HTMLOptionElement::setSelectedForBindings): * Source/WebCore/html/HTMLOptionElement.h: * Source/WebCore/html/HTMLOptionElement.idl: * Source/WebCore/html/HTMLSelectElement.cpp: (WebCore::HTMLSelectElement::reset): Canonical link: https://commits.webkit.org/311746@main
https://bugs.webkit.org/show_bug.cgi?id=280081 rdar://136346356 Reviewed by Mike Wyrzykowski. Remove clamps that prevented negative playbackRate value. Add layout tests for negative rate play/pause and negative rate looping. * LayoutTests/model-element/model-element-animations-loop-expected.txt: * LayoutTests/model-element/model-element-animations-loop.html: * LayoutTests/model-element/model-element-animations-playback-expected.txt: * LayoutTests/model-element/model-element-animations-playback.html: * Source/WebCore/Modules/model-element/ModelPlayerAnimationState.cpp: (WebCore::ModelPlayerAnimationState::setPlaybackRate): (WebCore::ModelPlayerAnimationState::currentTime const): * Source/WebKit/WebKitSwift/RealityKit/WKRKEntity.swift: (WKRKEntity.playbackRate): * Source/WebKit/WebProcess/Model/ModelProcessModelPlayer.cpp: (WebKit::ModelProcessModelPlayer::setPlaybackRate): Canonical link: https://commits.webkit.org/311747@main
https://bugs.webkit.org/show_bug.cgi?id=312935 rdar://175288394 Reviewed by Tim Nguyen. Both overloads have had zero callers since https://commits.webkit.org/228139@main (August 2020). * Source/WebCore/platform/graphics/GlyphBuffer.h: (WebCore::GlyphBuffer::expandLastAdvance): Deleted. Canonical link: https://commits.webkit.org/311748@main
…thout checking progress was made, meaning we can loop forever https://bugs.webkit.org/show_bug.cgi?id=312932 rdar://175280603 Reviewed by Joshua Hoffman. In 311153@main we fixed three while-loops in AccessibilityObject.cpp where VisiblePosition::next() or previous() could return the same position, causing an infinite loop and permanently hanging the main-thread. This change applies the same defensive pattern to five additional loops that have the same vulnerability: 1. leftLineVisiblePositionRange: previous() loop without progress check 2. rightLineVisiblePositionRange: next() loop without progress check 3. updateAXLineStartForVisiblePosition: while(true) with previous() and no stuck-position guard 4. characterOffsetFromVisiblePosition: nextVisuallyDistinctCandidate loop with no guard despite comments acknowledging the risk 5. characterOffsetForBounds: bidirectional CharacterOffset iteration with no stuck-position check * LayoutTests/accessibility/left-right-line-range-display-table-in-flex-expected.txt: Added. * LayoutTests/accessibility/left-right-line-range-display-table-in-flex.html: Added. * LayoutTests/platform/glib/TestExpectations: * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::characterOffsetFromVisiblePosition): (WebCore::AXObjectCache::characterOffsetForBounds): * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::updateAXLineStartForVisiblePosition): (WebCore::AccessibilityObject::leftLineVisiblePositionRange const): (WebCore::AccessibilityObject::rightLineVisiblePositionRange const): Canonical link: https://commits.webkit.org/311749@main
…to linker-generated helper functions https://bugs.webkit.org/show_bug.cgi?id=312947 rdar://175271158 Reviewed by Richard Robinson. When delay-init linking an Objective-C class, the linker can now generate a helper function with a name of the format "_OBJC_CLASS_$_TheClassName$loadHelper_x8". This causes failures from our check-for-inappropriate-objc-class-names script, which detects that symbol as an Objective-C class named "TheClassName$loadHelper_x8". Prevent this by excluding all symbols from the __TEXT segment when considering Objective-C class names, since the metadata for classes (i.e. any content that would ever legitimately produce issues) is placed into other sections. While I was in the area, I also changed the output formatting to print 'error:' in lowercase since Xcode knows how to surface such prefixed lines as separate errors rather than burying that output in the build transcript. * Tools/Scripts/check-for-inappropriate-objc-class-names: Augment the `nm` invocation to print the full mach-o info about the symbols so that the segment is included in the output. That then requires updates to the name extraction to account for the fact that the line now contains more text than just the symbol name. Canonical link: https://commits.webkit.org/311750@main
…tes callers https://bugs.webkit.org/show_bug.cgi?id=312863 Reviewed by Darin Adler and Ryosuke Niwa. bestFitSourceForImageAttributes() was taking `const AtomString&` for its srcAttribute parameter, but only calls isNull(), isEmpty(), and passed it to StringViewWithUnderlyingString — none of which require AtomString. Three of its five callers constructed a temporary AtomString from a String just to satisfy the signature, which hashes the string and inserts it into the global atom table for no benefit. Change the parameter type to `const String&`. The two callers that already pass AtomString (HTMLImageElement) work without changes via implicit conversion. * Source/WebCore/html/parser/HTMLPreloadScanner.cpp: (WebCore::TokenPreloadScanner::StartTagScanner::processAttributes): * Source/WebCore/html/parser/HTMLSrcsetParser.cpp: (WebCore::bestFitSourceForImageAttributes): * Source/WebCore/html/parser/HTMLSrcsetParser.h: (WebCore::bestFitSourceForImageAttributes): * Source/WebCore/loader/LinkLoader.cpp: (WebCore::LinkLoader::preloadIfNeeded): Canonical link: https://commits.webkit.org/311751@main
https://bugs.webkit.org/show_bug.cgi?id=312961 rdar://175311669 Reviewed by Dan Glastonbury. This adds a missing WPT test for deselectAll on SVGSVGElement Interface. see w3c/svgwg#1095 Test: imported/w3c/web-platform-tests/svg/struct/SVGSVGElement-deselectAll.html * LayoutTests/imported/w3c/web-platform-tests/svg/struct/SVGSVGElement-deselectAll-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/svg/struct/SVGSVGElement-deselectAll.html: Added. Canonical link: https://commits.webkit.org/311752@main
…okens are active https://bugs.webkit.org/show_bug.cgi?id=312867 Reviewed by Ryosuke Niwa. When a DocumentParserYieldToken is released and immediately retaken, didEndYieldingParser() schedules a 0s resume timer, and then didBeginYieldingParser() sets yield tokens active again. When the timer fires, continueNextChunkTimerFired() calls pumpTokenizer() which asserts !isScheduledForResume() — but that returns true because yield tokens are active, causing an assertion failure. In release builds, this leads to a busy-loop through the event loop until the yield tokens are cleared. Fix this by checking m_documentHasActiveParserYieldTokens in continueNextChunkTimerFired() and returning early, letting didEndYieldingParser() handle scheduling the resume when tokens are eventually released. Tests: Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenPlugIn.mm Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenTests.h Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenTests.mm * Source/WebCore/html/parser/HTMLParserScheduler.cpp: (WebCore::HTMLParserScheduler::continueNextChunkTimerFired): * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenPlugIn.mm: (-[ParserYieldTokenPlugIn releaseAndRetakeDocumentParserToken]): * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenTests.h: * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/ParserYieldTokenTests.mm: (TEST(ParserYieldTokenTests, ReleaseAndRetakeYieldTokenDoesNotCrash)): Canonical link: https://commits.webkit.org/311753@main
https://bugs.webkit.org/show_bug.cgi?id=312877 rdar://175240759 Reviewed by Dan Glastonbury. Skip the test, it's a known unimplemented failure: OpenGL allows sampling integer textures with float samplers. This is disallowed in WebGL. Add the test to WebGLCompatibilityTest to ensure the rule is implemented. * Source/ThirdParty/ANGLE/src/tests/angle_end2end_tests_expectations.txt: * Source/ThirdParty/ANGLE/src/tests/gl_tests/TextureTest.cpp: * Source/ThirdParty/ANGLE/src/tests/gl_tests/WebGLCompatibilityTest.cpp: Canonical link: https://commits.webkit.org/311754@main
…_NAME redefinitions https://bugs.webkit.org/show_bug.cgi?id=312974 Reviewed by Claudio Saavedra. Fixes a copy/paste regression from 299705@main (commit af7a25e). The two new WEBKIT_CHECK_HAVE_FUNCTION() checks added for drmModeCreateDumbBuffer and drmModeDestroyDumbBuffer were both passed HAVE_DRM_GET_FORMAT_MODIFIER_NAME as the HAVE_* variable to define, which overwrites the result of the preceding drmGetFormatModifierName check. On systems whose libdrm lacks the dumb-buffer functions (e.g. Ubuntu 22.04 without the JHBuild moduleset) this leads to build failures. Covered by Ubuntu 22.04 release builders. * Source/cmake/OptionsGTK.cmake: * Source/cmake/OptionsWPE.cmake: Canonical link: https://commits.webkit.org/311755@main
https://bugs.webkit.org/show_bug.cgi?id=312963 Reviewed by Yusuke Suzuki. tryAppendUBFX previously matched only BitAnd(ZShr, mask). Extend it to also accept SShr (arithmetic right shift). This is safe because the existing lsb + width <= datasize guard ensures the mask never reaches the sign-extension bits introduced by the arithmetic shift, so ubfx (which zero-extends) produces the identical result. Also move tryAppendUBFX before the 0xff/0xffff/0xffffffff zero-extend special cases so that the common pixel-unpacking pattern (pixel >> N) & 0xff is not stolen by ZeroExtend8To32 and instead collapses into a single ubfx. Non-shift BitAnd(x, 0xff) still falls through to the existing zero-extend path, and x86_64 is unaffected since ExtractUnsignedBitfield* has no valid form there. This mirrors a recent V8 change that reported ~2% on JetStream3 gaussian-blur: https://chromium-review.googlesource.com/c/v8/v8/+/7768219 FTL codegen for `((p>>24)&0xff)+((p>>16)&0xff)+((p>>8)&0xff)+(p&0xff)`: Before: asr w2, w0, #0x18 and w2, w2, #0xff asr w3, w0, #0x10 and w3, w3, #0xff asr w4, w0, #0x8 and w4, w4, #0xff and w0, w0, #0xff add w2, w2, w3 add w0, w4, w0 add w0, w2, w0 After: lsr w2, w0, #0x18 ubfx w3, w0, #0x10, #0x8 ubfx w4, w0, #0x8, #0x8 and w0, w0, #0xff add w2, w2, w3 add w0, w4, w0 add w0, w2, w0 Microbenchmark (Apple Silicon): TipOfTree Patched arithmetic-shift-and-mask 19.0037+-0.2975 18.6483+-0.0836 might be 1.0191x faster Tests: JSTests/microbenchmarks/arithmetic-shift-and-mask.js Source/JavaScriptCore/b3/testb3_2.cpp * JSTests/microbenchmarks/arithmetic-shift-and-mask.js: Added. (test): * Source/JavaScriptCore/b3/B3LowerToAir.cpp: * Source/JavaScriptCore/b3/testb3.h: * Source/JavaScriptCore/b3/testb3_2.cpp: (testUbfx32ArithmeticShiftAnd): (testUbfx64ArithmeticShiftAnd): (addBitTests): Canonical link: https://commits.webkit.org/311756@main
https://bugs.webkit.org/show_bug.cgi?id=312956 Reviewed by Yusuke Suzuki. pinballHandlerFinishReject is called from offlineasm (InPlaceInterpreter.asm) using the SysV argument registers (`move sp, a0` where a0 = rdi), same as the other four pinballHandler* functions. However its declaration was missing the SYSV_ABI attribute, so on Windows x64 the C++ side was compiled with the Win64 calling convention and read its argument from rcx instead of rdi, causing a crash on `*context->vm` when a JSPI suspension promise is rejected. This has been broken since the initial JSPI implementation in 299221@main. SYSV_ABI is a no-op on non-Windows so the issue was not visible on other platforms. * Source/JavaScriptCore/runtime/PinballCompletion.cpp: Canonical link: https://commits.webkit.org/311757@main
…sh.html is a constant timeout https://bugs.webkit.org/show_bug.cgi?id=312931 Reviewed by Xabier Rodriguez-Calvar. There is no need to flush the decoder if it hasn't started yet. After fixing that the flush promise still hanged because the scheduled decode task just before was a rejected promise. The decoder now returns a resolved promise when the encoded frame is empty. * LayoutTests/platform/glib/TestExpectations: * Source/WebCore/platform/audio/gstreamer/AudioDecoderGStreamer.cpp: (WebCore::GStreamerInternalAudioDecoder::decode): (WebCore::GStreamerInternalAudioDecoder::flush): Canonical link: https://commits.webkit.org/311758@main
…ourceChanged https://bugs.webkit.org/show_bug.cgi?id=312819 Reviewed by Rob Buis. Refactor `CSSSVGResourceElementClient::resourceChanged()` so the in-layout early return only fires for clients that currently have a `RenderLayer` -- those are handled by the post-layout `recursiveUpdateLayerPositions()` phase. Clients without layers fall through to a direct `repaintOldAndNewPositionsForSVGRenderer()` call. Also mark layered clients with `setSelfAndDescendantsNeedPositionUpdate()` so their repaint rects are regenerated even when the client's own geometry did not change. At present every LBSE SVG client owns a layer so the behavioral delta is just the new `setSelfAndDescendantsNeedPositionUpdate()` call - the relevant tests are marked as failing, and still fail because of other issues in the current LBSE but will be fixed after the conditional-layer-creation PR lands. Also the non-layer branch only becomes active in the follow-up conditional-layer-creation patch, not yet today. * Source/WebCore/rendering/ReferencedSVGResources.cpp: (WebCore::CSSSVGResourceElementClient::resourceChanged): Canonical link: https://commits.webkit.org/311759@main
…e binding support for dma-buf backed GPU atlas textures" https://bugs.webkit.org/show_bug.cgi?id=312971 Reviewed by Carlos Garcia Campos. This partially reverts 311234@main, keeping only the "avoid upload fence" in dma-buf upload code path change and a BitmapTexture specific fix to remove the LinearTexture/VivanteSuperTiledTexture related flags if dma-buf support backing support is not available. The original patch caused a peformance regression in MotionMark/Images, due to the glFlush() usage in the upload thread, necessary to support deferred texture binding, but the feature itself didn't fix the rendering glitches observed on particular configurations, so revert that logic. * Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.cpp: (WebCore::SkiaGPUAtlas::create): (WebCore::SkiaGPUAtlas::uploadImages): (WebCore::SkiaGPUAtlas::ensureBackendTexture): Deleted. * Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.h: * Source/WebCore/platform/graphics/skia/SkiaPaintingEngine.cpp: (WebCore::SkiaPaintingEngine::createAtlas): (WebCore::SkiaPaintingEngine::record): * Source/WebCore/platform/graphics/skia/SkiaPaintingEngine.h: * Source/WebCore/platform/graphics/skia/SkiaReplayCanvas.cpp: (WebCore::SkiaReplayCanvas::SkiaReplayCanvas): * Source/WebCore/platform/graphics/texmap/BitmapTexture.cpp: (WebCore::BitmapTexture::BitmapTexture): (WebCore::BitmapTexture::reset): (WebCore::BitmapTexture::bindDMABufToTexture): Deleted. * Source/WebCore/platform/graphics/texmap/BitmapTexture.h: Canonical link: https://commits.webkit.org/311760@main
https://bugs.webkit.org/show_bug.cgi?id=312708 Reviewed by Sam Weinig and Chris Dumez. The progressed test still has a harness error due to an unrelated bug. Canonical link: https://commits.webkit.org/311761@main
https://bugs.webkit.org/show_bug.cgi?id=312837 rdar://175205696 Reviewed by Ryosuke Niwa. * Source/WebCore/SaferCPPExpectations/NoDeleteCheckerExpectations: * Source/WebCore/css/CSSStyleProperties.cpp: (WebCore::InlineCSSStyleProperties::didMutate): * Source/WebCore/dom/Node.h: Canonical link: https://commits.webkit.org/311762@main
…ayer https://bugs.webkit.org/show_bug.cgi?id=312984 Reviewed by Rob Buis. SVGLayerTransformUpdater bailed out early when the renderer had no layer, skipping updateLayerTransform(). Some SVG renderers (e.g. RenderSVGViewportContainer) compute supplemental transforms (viewBox, zoom, pan) in their updateLayerTransform() override that are required by applyTransform() during painting, regardless of whether the renderer is layer-backed. Covered by existing tests, once conditional-layer creation is enabled for LBSE. * Source/WebCore/rendering/svg/SVGLayerTransformUpdater.h: (WebCore::SVGLayerTransformUpdater::SVGLayerTransformUpdater): (WebCore::SVGLayerTransformUpdater::~SVGLayerTransformUpdater): Canonical link: https://commits.webkit.org/311763@main
…ltered" https://bugs.webkit.org/show_bug.cgi?id=312723 rdar://175279759 Reviewed by Qianlang Chen. Console messages in a collapsed console group (`console.groupCollapsed()`) were counted as invisible messages. This triggered the hidden messages banner, which is meant to be shown when messages are hidden as a result of filtering. Clicking the button in the banner to clear filters and reveal the hidden messages does nothing for collapsed console groups. This patch isolates the check for messages hidden as a result of filtering before showing the hidden messages banner. * Source/WebInspectorUI/UserInterface/Views/LogContentView.js: (WI.LogContentView.prototype.didAppendConsoleMessageView): (WI.LogContentView.prototype._isMessageFilteredOut): (WI.LogContentView.prototype._isMessageVisible): Canonical link: https://commits.webkit.org/311764@main
https://bugs.webkit.org/show_bug.cgi?id=312972 Reviewed by Ryosuke Niwa. * Source/WebCore/html/parser/HTMLSrcsetParser.cpp: (WebCore::replaceURLsInSrcsetAttribute): Canonical link: https://commits.webkit.org/311765@main
…enderers. https://bugs.webkit.org/show_bug.cgi?id=310940 rdar://166836126 Reviewed by Alan Baradlay. According to css-overflow, when a box has some type of scrollbar change, it is supposed to impact its intrinsic size. "When reserving space for a scrollbar placed at the edge of an element’s box, the reserved space is inserted between the inner border edge and the outer padding edge... When the box is intrinsically sized, this reserved space is added to the size of its contents." https://drafts.csswg.org/css-overflow-3/#scrollbar-sizing For example, consider the following testcase: <div style="width: max-content; background-color: green;"> <div style="height: 79px; overflow: auto;"> <div style="width: 10px; height: 80px;"></div> </div> </div> When the child of the max-content div ends up getting a scrollbar due to its overflowing content, the logical width of the max-content div should be updated to reflact that. However, we currently fail to detect this change and the size of the width: max-content div ends up remaining the same. This patch serves as a first step towards fixing this class of bugs by adding some infrastructure that keeps track of scrollbar changes for a subtree where the root is intrinsically sized according to its computed logical width. The basic idea is that when we run into such a renderer we want to start keeping track of any descendants that gain or lose a scrollbar. If that happens we will want to recompute our logical width in response. Tests: imported/w3c/web-platform-tests/css/css-overflow/max-width-container-with-scrollable-descendant-ref.html imported/w3c/web-platform-tests/css/css-overflow/max-width-container-with-scrollable-descendant.html * Source/WebCore/rendering/SubtreeScrollbarChangesState.cpp: Added. SubtreeScrollbarChangesState is a new bit of state that indicates a renderer is interested in knowing about any time a descendant has a scrollbar change. This state is created with the renderer that wants to know about these changes and is passed down the subtree during layout. By the time we are back at the subtree root we should have a list of any descendants that gained or lost a scrollbar. SubtreeScrollbarChangesHandler is a new stack-based helper class that can be used to indicate that the passed-in renderer will be able to handle and contain any changes that come from a descendant gaining/losing a scrollbar. The renderer that is the subtree root is certainly a renderer that will need to handle the changes, but it is possible for a descendant of that root to be able to contain the changes. For example, if the descendant has a fixed width, then the scrollbar changes below it will not have an impact above. (WebCore::SubtreeScrollbarChangesHandler::SubtreeScrollbarChangesHandler): If the handler is not the subtree root and is some other descendant that can contain the scrollbar changes, then we need to make sure to properly save the list of renderers that is already in the SubtreeScrollbarChangesState and make sure the list in LocalFrameViewLayoutContext is empty. This is because we want to only handle the descendants of the handler and not, for example, those of the siblings of the handler. (WebCore::SubtreeScrollbarChangesHandler::~SubtreeScrollbarChangesHandler): 1.) Make sure that we restore the list of descendants that were saved in the constructor by adding a ScopeExit that does so. It will also debug assert that the list of descendants which was populated for the handler is empty. 2.) Make sure that the list of descendants is handled properly. If we are the subtree root, then we need to invalidate the preferred widths of descendants and the containing block chain up to the subtree root so that when we perform layout again on the subtree root we can properly update the logical width. If we are not the subtree root then we will need to just call layoutBlock on the renderers which is what would have happened in the RelayoutScopeForScrollbarChange destructor. A ListHashSet is used here to make sure that we call layoutBlock in the same order we would have traversed the tree. * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::layout): We need to initialize the state and handle it some time before and after we perform layout. Doing it here will allow us to do so before the various renderers perform layout via layoutBlock. We will need to create the SubtreeScrollbarChangesHandler if either this renderer is the one that has the intrinsic logical width or the renderer is some descendant that can properly contain the changes as we are walking back up the tree. * Source/WebCore/rendering/RelayoutScopeForScrollbarChange.cpp: (WebCore::RelayoutScopeForScrollbarChange::~RelayoutScopeForScrollbarChange): If we are currently tracking a subtree for scrollbar changes then we will add them into the list to be propagated back up. In that case we also do not want to call layoutBlock here since it should be handled by an ancestor. Canonical link: https://commits.webkit.org/311766@main
https://bugs.webkit.org/show_bug.cgi?id=312982 Reviewed by Nikolas Zimmermann. In an upcoming PR svg elements can set transforms without requiring a layer, so prepare some if conditions for that. In addition, there was no point checking the clip path/mask itself for its transform in transformationMatrixFromChild, the important part is just the child transform. * Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp: (WebCore::SVGBoundingBoxComputation::handleRootOrContainer const): * Source/WebCore/svg/SVGClipPathElement.cpp: (WebCore::SVGClipPathElement::calculateClipContentRepaintRect): * Source/WebCore/svg/SVGMaskElement.cpp: (WebCore::SVGMaskElement::calculateMaskContentRepaintRect): Canonical link: https://commits.webkit.org/311767@main
…ger button has child elements https://bugs.webkit.org/show_bug.cgi?id=278019 rdar://134337357 Reviewed by Ryosuke Niwa. On macOS/iOS, buttons are not mouse-focusable. A workaround records that the last focus trigger was a click, so that a subsequent programmatic focus() call hides :focus-visible. However, the workaround only checked the innermost element under the mouse. When a button has child elements (e.g. <button><div>text</div></button>), the hit-test target is the <div>, not the <button>, so the workaround did not fire and :focus-visible incorrectly appeared. Walk up from the hit-test target to find a non-mouse-focusable form control ancestor instead of only checking the innermost element. Test: fast/selectors/focus-visible-script-focus-from-click.html * LayoutTests/fast/selectors/focus-visible-script-focus-from-click-expected.txt: Added. * LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html: Added. * Source/WebCore/page/EventHandler.cpp: (WebCore::EventHandler::dispatchMouseEvent): Canonical link: https://commits.webkit.org/311768@main
Unreviewed gardening. * LayoutTests/platform/glib/TestExpectations: Canonical link: https://commits.webkit.org/311972@main
…edded-content/media-elements/preserves-pitch.html is a flaky text failure. rdar://175529361 https://bugs.webkit.org/show_bug.cgi?id=313259 Unreviewed test gardening. * LayoutTests/platform/mac-wk2/TestExpectations: Canonical link: https://commits.webkit.org/311973@main
…he number of available backreferences https://bugs.webkit.org/show_bug.cgi?id=313158 rdar://167746769 Reviewed by Yijia Huang. This patch prevents the subpattern count from being incremented when a non-capturing group is encountered. Test: JSTests/stress/regexp-noncapturing-group-backreference-count.js * JSTests/stress/regexp-noncapturing-group-backreference-count.js: Added. (expectInvalid): * Source/JavaScriptCore/yarr/YarrParser.h: (JSC::Yarr::Parser::parseParenthesesBegin): Canonical link: https://commits.webkit.org/311974@main
…esolution https://bugs.webkit.org/show_bug.cgi?id=313232 Reviewed by Antti Koivisto. The old name focused on the "anonymous" property, but the function's purpose is to decide whether a block should be skipped when walking the containing block chain for percentage resolution. The new name describes the behavior. Also moved the RenderView early return from skipContainingBlockForPercentHeightCalculation into the quirks mode lambda where it belongs -- RenderView should not be skipped despite having auto height, same as flex containers and grid. * Source/WebCore/rendering/RenderBox.cpp: (WebCore::RenderBox::skipContainingBlockForPercentHeightCalculation const): * Source/WebCore/rendering/RenderBoxModelObject.cpp: (WebCore::RenderBoxModelObject::containingBlockForAutoHeightDetectionGeneric const): (WebCore::hasDefiniteHeightByStyle): * Source/WebCore/rendering/RenderElement.h: (WebCore::RenderElement::shouldSkipForPercentageResolution const): * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::RenderReplaced::computeReplacedLogicalHeightUsingGeneric const): Canonical link: https://commits.webkit.org/311975@main
…d Armv7) https://bugs.webkit.org/show_bug.cgi?id=312942 Reviewed by Fujii Hironori. See the discussion on WebKit#62849. This broke gdb on linux, so we should revert for now until we can fix that. * Source/WTF/wtf/Assertions.cpp: * Source/WTF/wtf/Assertions.h: * Source/WTF/wtf/PlatformEnable.h: * Source/cmake/OptionsWPE.cmake: Canonical link: https://commits.webkit.org/311976@main
…s-flex.html https://bugs.webkit.org/show_bug.cgi?id=313239 Reviewed by Antti Koivisto. A flex item with margin-left: calc(10% + 100px) inside a min-content flex container should contribute 100px to the container's intrinsic width (the percentage resolves to 0 in intrinsic sizing). marginIntrinsicLogicalWidthForChild only supports fixed values. The fix uses Style::evaluateMinimum which properly extracts the fixed component from calc() expressions (the percentage part resolves to 0 when the available width is 0). * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/calc-margins-percent-flex-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/calc-margins-percent-flex.html: Added. * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::marginIntrinsicLogicalWidthForChild const): Canonical link: https://commits.webkit.org/311977@main
https://bugs.webkit.org/show_bug.cgi?id=313176 rdar://173236391 Reviewed by Elliott Williams. Remove visionOS strict safety exclusion and update WKRKEntity and WKStageMode to compile with strict safety enabled. * Configurations/CommonBase.xcconfig: * Source/WebKit/WebKitSwift/RealityKit/WKRKEntity.swift: (WKRKEntity.load(from:withAttributionTaskID:entityMemoryLimit:)): (WKRKEntity.resizedImage(_:)): (WKRKEntity.applyIBLData(_:attributionHandler:)): (WKRKEntity.setParentCore(_:preservingWorldTransform:)): * Source/WebKit/WebKitSwift/StageMode/WKStageMode.swift: (WKStageModeInteractionDriver.interactionContainerRef): (WKStageModeInteractionDriver.setContainerTransformInPortal): (WKStageModeInteractionDriver.applySimulatorState): Canonical link: https://commits.webkit.org/311978@main
…eObjectInOSR https://bugs.webkit.org/show_bug.cgi?id=313170 rdar://175456941 Reviewed by Keith Miller. operationPopulateObjectInOSR was checking materialization->indexingType() (the compile-time type) to decide whether to issue a write barrier after populating a PhantomNewArrayWithButterfly. The correct check is against the array's actual runtime indexing type. Canonical link: https://commits.webkit.org/311979@main
https://bugs.webkit.org/show_bug.cgi?id=313218 rdar://175496917 Reviewed by Yijia Huang. Adding a bit more comments and cleaned up special methods to make the intent clear as JSBigInt::cachedMod is complicated algorithm (this is basically `BarrettDivRem` algorithm). Test: JSTests/stress/bigint-cached-mod-edge-cases.js * JSTests/stress/bigint-cached-mod-edge-cases.js: Added. (shouldBe): (throw.new.Error): * Source/JavaScriptCore/runtime/JSBigInt.cpp: (JSC::JSBigInt::multiplySpecialHigh): (JSC::JSBigInt::cachedMod): * Source/JavaScriptCore/runtime/JSBigInt.h: Canonical link: https://commits.webkit.org/311980@main
…a does not fallback properly when body is scrollable rdar://175203020 https://bugs.webkit.org/show_bug.cgi?id=312831 Reviewed by Elika Etemad. Fixed-positioned elements that use position-area should still use the viewport as its containing block, not its scrollable containing block. Test: imported/w3c/web-platform-tests/css/css-anchor-position/anchor-scroll-position-try-016.html * LayoutTests/imported/w3c/web-platform-tests/css/css-anchor-position/anchor-scroll-position-try-016-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-anchor-position/anchor-scroll-position-try-016.html: Added. * Source/WebCore/rendering/RenderBox.cpp: (WebCore::RenderBox::containingBlockRangeForPositioned const): Canonical link: https://commits.webkit.org/311981@main
…aed489d1 # Conflicts: # Source/JavaScriptCore/API/APICast.h # Source/JavaScriptCore/API/JSContext.mm # Source/JavaScriptCore/API/JSWeakObjectMapRefPrivate.cpp # Source/JavaScriptCore/CMakeLists.txt # Source/JavaScriptCore/bytecode/CodeBlock.cpp # Source/JavaScriptCore/bytecode/CodeBlock.h # Source/JavaScriptCore/bytecode/InlineCallFrame.h # Source/JavaScriptCore/bytecode/PutByVariant.h # Source/JavaScriptCore/bytecode/SpeculatedType.h # Source/JavaScriptCore/debugger/Debugger.h # Source/JavaScriptCore/inspector/InjectedScriptHost.h # Source/JavaScriptCore/inspector/PerGlobalObjectWrapperWorld.h # Source/JavaScriptCore/inspector/ScriptFunctionCall.h # Source/JavaScriptCore/interpreter/CallFrame.h # Source/JavaScriptCore/interpreter/FrameTracers.h # Source/JavaScriptCore/interpreter/Interpreter.cpp # Source/JavaScriptCore/interpreter/Interpreter.h # Source/JavaScriptCore/interpreter/Register.h # Source/JavaScriptCore/interpreter/RegisterInlines.h # Source/JavaScriptCore/jit/JITCode.h # Source/JavaScriptCore/jit/SnippetParams.h # Source/JavaScriptCore/runtime/AbstractModuleRecord.h # Source/JavaScriptCore/runtime/ArrayStorageInlines.h # Source/JavaScriptCore/runtime/BufferMemoryHandle.h # Source/JavaScriptCore/runtime/ButterflyInlines.h # Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp # Source/JavaScriptCore/runtime/Error.cpp # Source/JavaScriptCore/runtime/ErrorInstance.cpp # Source/JavaScriptCore/runtime/Exception.cpp # Source/JavaScriptCore/runtime/ExceptionHelpers.h # Source/JavaScriptCore/runtime/ExecutableBaseInlines.h # Source/JavaScriptCore/runtime/HashMapHelper.h # Source/JavaScriptCore/runtime/IntlObject.h # Source/JavaScriptCore/runtime/JSArrayInlines.h # Source/JavaScriptCore/runtime/JSBigInt.h # Source/JavaScriptCore/runtime/JSBigIntInlines.h # Source/JavaScriptCore/runtime/JSCInlines.h # Source/JavaScriptCore/runtime/JSCJSValue.h # Source/JavaScriptCore/runtime/JSCJSValueInlines.h # Source/JavaScriptCore/runtime/JSCalleeInlines.h # Source/JavaScriptCore/runtime/JSCast.h # Source/JavaScriptCore/runtime/JSDateMath.h # Source/JavaScriptCore/runtime/JSFunction.cpp # Source/JavaScriptCore/runtime/JSFunction.h # Source/JavaScriptCore/runtime/JSGlobalObject.cpp # Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h # Source/JavaScriptCore/runtime/JSGlobalProxy.h # Source/JavaScriptCore/runtime/JSGlobalProxyInlines.h # Source/JavaScriptCore/runtime/JSLexicalEnvironment.h # Source/JavaScriptCore/runtime/JSMicrotask.cpp # Source/JavaScriptCore/runtime/JSModuleLoader.cpp # Source/JavaScriptCore/runtime/JSNativeStdFunction.h # Source/JavaScriptCore/runtime/JSObject.h # Source/JavaScriptCore/runtime/JSPromise.cpp # Source/JavaScriptCore/runtime/JSPromisePrototype.cpp # Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h # Source/JavaScriptCore/runtime/JSRunLoopTimer.h # Source/JavaScriptCore/runtime/JSScriptFetchParameters.h # Source/JavaScriptCore/runtime/JSScriptFetcher.h # Source/JavaScriptCore/runtime/JSString.h # Source/JavaScriptCore/runtime/MatchResult.h # Source/JavaScriptCore/runtime/NativeFunction.h # Source/JavaScriptCore/runtime/NullSetterFunction.h # Source/JavaScriptCore/runtime/Operations.h # Source/JavaScriptCore/runtime/ScriptExecutable.h # Source/JavaScriptCore/runtime/ShadowRealmObject.h # Source/JavaScriptCore/runtime/Structure.h # Source/JavaScriptCore/runtime/StructureInlines.h # Source/JavaScriptCore/runtime/VM.h # Source/JavaScriptCore/runtime/VMInlines.h # Source/JavaScriptCore/runtime/WriteBarrierInlines.h # Source/JavaScriptCore/wasm/WasmContext.h # Source/WTF/wtf/PlatformEnable.h # Source/WTF/wtf/PlatformHave.h # Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedPlatformLayer.cpp
…ource include fixes
Adopts upstream's removal of jsCast/jsDynamicCast (now uncheckedDowncast/
dynamicDowncast, template arg loses the *) and JSScriptFetcher/
JSScriptFetchParameters (moduleLoader hooks now take RefPtr<ScriptFetcher>/
RefPtr<ScriptFetchParameters>).
- 1588 jsCast/jsDynamicCast call sites migrated across 248 files +
generate-classes.ts/generate-jssink.ts templates
- jsSecureCast → downcast
- 13 redundant same-type casts dropped (new API static_asserts against them)
- JSType.zig: JSScriptFetcher/JSScriptFetchParameters removed, 23+ renumbered
-2, EmbedderArrayLike = 237 added
- ZigGlobalObject/BakeGlobalObject/NodeVM moduleLoader hooks retyped;
scriptFetcher.isObject() short-circuit removed (old JS-loader quirk, dead
with the C++ loader; --eval result capture keys on specifier, not fetcher)
- NodeVM: scriptFetchParametersToImportAttributes() rebuilds {type} for the
importModuleDynamically callback
- bindings.cpp/ZigGlobalObject.cpp: include MicrotaskQueue.h (QueuedTask now
forward-declared in VM.h)
- WEBKIT_VERSION → autobuild-preview-pr-199-b091462b
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThis pull request removes macOS 14 support from build configurations, updates CMake policies for Swift compilation, refactors WebAssembly debugger test infrastructure, and adds comprehensive test coverage across JavaScript and layout test suites. Changes
Possibly related PRs
|
There was a problem hiding this comment.
Actionable comments posted: 59
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
LayoutTests/fast/css/vertical-text-overflow-ellipsis-text-align-center.html (1)
37-61:⚠️ Potential issue | 🟡 MinorAdd empty
altattributes to test images.All four updated
<img>tags are missingalt, which triggers HTMLHint and weakens accessibility hygiene in test fixtures. Usealt=""(decorative image) to avoid affecting layout behavior.Suggested patch
-<div class="testDiv">Lorem ipsum dolor sit amet, <img src="resources/greenbox.png" width="25" height="25"> consectetur adipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> +<div class="testDiv">Lorem ipsum dolor sit amet, <img src="resources/greenbox.png" width="25" height="25" alt=""> consectetur adipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> -<div class="testDiv">Lorem ipsum dolor sit amet, consectetur ad<img src="resources/greenbox.png" width="25" height="25">ipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> +<div class="testDiv">Lorem ipsum dolor sit amet, consectetur ad<img src="resources/greenbox.png" width="25" height="25" alt="">ipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> -<div class="testDiv rtl">Lorem ipsum dolor sit amet, <img src="resources/greenbox.png" width="25" height="25"> consectetur adipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> +<div class="testDiv rtl">Lorem ipsum dolor sit amet, <img src="resources/greenbox.png" width="25" height="25" alt=""> consectetur adipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> -<div class="testDiv rtl">Lorem ipsum dolor sit amet, consectetur ad<img src="resources/greenbox.png" width="25" height="25">ipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div> +<div class="testDiv rtl">Lorem ipsum dolor sit amet, consectetur ad<img src="resources/greenbox.png" width="25" height="25" alt="">ipiscing elit. Vivamus vitae eros non libero faucibus sagittis sed ut eros.</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css/vertical-text-overflow-ellipsis-text-align-center.html` around lines 37 - 61, The four inline <img> elements in the vertical-text-overflow-ellipsis test markup (the images inside the <div class="testDiv"> blocks, including the RTL variants) lack alt attributes; add alt="" to each of those <img src="resources/greenbox.png" width="25" height="25"> occurrences to mark them decorative and satisfy HTMLHint/accessibility checks while preserving layout and behavior.LayoutTests/fast/text/font-face-empty-string.html (1)
24-24:⚠️ Potential issue | 🟡 MinorAdd an explicit assertion for
fontface.family = ''.After removing the throw check, this path is now unverified. Please assert the intended behavior (at least non-throw) so the test still protects the semantic change.
Suggested test adjustment
+shouldNotThrow("fontface.family = ''"); shouldNotThrow("fontface.family = '\"\"'");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/text/font-face-empty-string.html` at line 24, The test removed the check for assigning an empty string to fontface.family, leaving that behavior unverified; add an explicit assertion that assigning an empty string to fontface.family does not throw and yields the expected value. Update the test to include a non-throw check (e.g. using shouldNotThrow with the expression fontface.family = '' ) and then assert the resulting fontface.family value (referencing the fontface variable and its family property) to ensure the assignment both succeeds and produces the intended empty-string state.LayoutTests/fast/harness/results.html (1)
1467-1481:⚠️ Potential issue | 🟠 MajorKeep ref-test reference links absolute in file mode.
Line 1477 and Line 1481 pass a bare filesystem path into
resultLink(), butresultLink()only recognizeshttp(s)://andfile://as absolute. On non-default variants this becomeswk1//Volumes/..., so thereference/ref mismatchlinks break when viewing local results.Suggested fix
if (resultToken.indexOf('IMAGE') != -1) { const nameParts = Utils.splitExtension(testResult.name); - const testBase = nameParts[0] + const testBase = nameParts[0]; const testExtension = nameParts[1]; const variantParams = nameParts[2]; + const referenceBase = this._resultsController.shouldUseGithubLinks() + ? this._resultsController.layoutTestsBasePath() + testBase + : 'file://' + this._resultsController.layoutTestsBasePath() + testBase; if (testResult.isMismatchRefTest()) { - result += this._resultsController.resultLink(this._resultsController.layoutTestsBasePath() + testBase, '-expected-mismatch' + testExtension + variantParams, 'ref mismatch'); + result += this._resultsController.resultLink(referenceBase, '-expected-mismatch' + testExtension + variantParams, 'ref mismatch'); result += this._resultsController.resultLink(testPrefix, '-actual.png', 'actual'); } else { if (testResult.isMatchRefTest()) - result += this._resultsController.resultLink(this._resultsController.layoutTestsBasePath() + testBase, '-expected' + testExtension + variantParams, 'reference'); + result += this._resultsController.resultLink(referenceBase, '-expected' + testExtension + variantParams, 'reference');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/harness/results.html` around lines 1467 - 1481, The ref-test links built in imageResultLinks (using this._resultsController.layoutTestsBasePath() + testBase) can end up as bare filesystem paths so resultLink() doesn't treat them as absolute; update imageResultLinks to detect when the combined base+testBase is a local filesystem path (e.g. does not start with 'http://' or 'file://') and, before calling this._resultsController.resultLink for the 'reference' and 'ref mismatch' cases, prepend the 'file://' scheme to the path so resultLink receives an absolute file:// URL; apply the same normalization for both the isMatchRefTest() and isMismatchRefTest() branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Line 24: The unconditional cmake_policy(SET CMP0157 NEW) call can fail on
CMake <3.29; guard it by testing policy availability before setting it: check if
POLICY CMP0157 is defined (using if(POLICY CMP0157)) and only then call
cmake_policy(SET CMP0157 NEW); update the CMakeLists logic around the existing
cmake_policy usage to perform that conditional check so older CMake (3.20–3.28)
won't attempt to set CMP0157.
In `@JSTests/microbenchmarks/regexp-forward-reference.js`:
- Around line 4-5: Change the mutable bindings for the regular expressions to
immutable ones: replace the declarations of re1 and re2 (the variables defined
as "let re1 = /\2(a)(b)/;" and "let re2 = /\k<y>(?<x>a)(?<y>b)/;") with const
declarations so they read as constant bindings, since neither re1 nor re2 is
reassigned.
In `@JSTests/stress/array-allocation-sink-double-nan-not-hole.js`:
- Around line 7-27: Replace the coercive global isNaN check with the strict
Number.isNaN for the variable result (so undefined/hole won't falsely pass), and
add an explicit runtime assertion that the index 1 exists in the final array
(the arr that was captured when i === breakAt) using the in-operator to reflect
the comment; locate the variables result, breakAt and arr in the loop and place
the Number.isNaN check and the "1 in arr" assertion right after the loop where
result is validated.
In `@JSTests/stress/async-function-empty-resolved-promise.js`:
- Around line 47-51: The disposal counter assertions are wrong because run()
invokes the test loop 1000 times so disposedSync/disposedAsync will grow beyond
1; update the checks around shouldBe(await onlyUsing(), undefined) /
shouldBe(await onlyAwaitUsing(), undefined) to assert the counters equal the
current iteration count (or reset disposedSync/disposedAsync to 0 at the start
of each run iteration) rather than the hard-coded 1 so repeated run() calls
pass; locate references to onlyUsing, onlyAwaitUsing, and the
disposedSync/disposedAsync counters to implement the fix.
In `@JSTests/stress/bigint-cached-mod-edge-cases.js`:
- Around line 260-274: The test never exercises the negative-dividend cached-mod
path because after the first modulo x2 and x4 fall into (-p,0] and subsequent
iterations take the |x|<|y| early return; modify the loops around p2/x2 and
p4/x4 so that each iteration reinflates the dividend (e.g. reset x2 to the
original -(1n << 200n) and x4 to -(1n << 500n) at the start of each loop) or
instead pin the loop inputs to predetermined negative remainders from a
known-good oracle; ensure you change the loop body that currently does x2 = x2 %
p2 and x4 = x4 % p4 to use the reinflated values so the negative-dividend
cached-mod path is actually exercised.
- Around line 308-330: The current checks after switching divisors only assert
range membership (shouldBe(x >= 0n && x < p2, true)) which can hide stale-cache
errors; change each block (the n=2->n=2 case with p1/p2 and the n=2->n=4 case
with p1/p2) to compute the expected final remainder in a separate reference
variable (e.g., let expected = initialX; run the same two sequences of 150
squarings and mod operations against p1 then p2 to produce expected) and then
replace the range assertion with an exact equality assertion like shouldBe(x,
expected) to ensure exact remainders are validated.
In
`@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js`:
- Around line 13-17: The try/catch around sab.grow silently swallows all
exceptions; change it to only tolerate expected resize failures and rethrow
unexpected errors—for example replace the empty catch with logic that catches
only the expected exception type (e.g., RangeError or the concrete exception
your runtime throws for failed grow) or checks the error signature (e instanceof
RangeError) and rethrows otherwise; apply the same change to the second
identical block (the other sab.grow try/catch at lines 27-31) so genuine
regressions aren't hidden while still allowing resize failures in the stress
loop.
- Around line 34-35: The test currently ignores the worker report returned by
$.agent.getReport(); capture its return value and assert it matches the expected
payload to avoid false positives. Replace the ignored call with code that
assigns the result of $.agent.getReport() to a variable (e.g., report), then use
the test harness assertion (e.g., assertEq or assert.sameValue) to compare
report against the expectedReport value previously sent by the worker; reference
$.agent.getReport() and expectedReport (or the concrete expected value) when
making the change.
In `@JSTests/stress/intl-segmenter-word-iswordlike-number.js`:
- Around line 22-29: Extract the repeated loop that iterates over
segmenter.segment(input) and asserts the tuple into a single helper (e.g.,
assertSegments or checkSegments) that accepts the segmenter, input string, and
expected results array; inside the helper iterate for (let { segment, index,
isWordLike } of segmenter.segment(input)), assert using shouldBe(result[0],
index), shouldBe(result[1], index + segment.length), shouldBe(result[2],
segment), shouldBe(result[3], isWordLike), and finally assert the cursor equals
results.length, then replace the four duplicated loops with calls to this helper
so callers no longer duplicate cursor/iteration/assert logic.
In `@JSTests/stress/json-parse-jsstring-cache-coherence.js`:
- Around line 102-109: The test currently generates strings like "v?x" causing
constant first/last/length and only one (first,last,len) group; change the
string construction in the loop that builds values and keys so each entry varies
first, last, or length (e.g., vary prefix/suffix characters and lengths) to
create many distinct (first,last,len) groups—update the loop that populates
values and keys to produce differing first/last/len patterns rather than always
"v?x".
In `@JSTests/stress/megamorphic-cache-super-property.js`:
- Around line 27-32: The test currently calls new Base().readSuper() and ignores
its return value, which can mask regressions in super property resolution;
modify the test to capture and assert the result of new Base().readSuper()
(verifying it matches the expected value when Base.prototype is set to obj1)
before proceeding to call readProp(obj2), so use the return from Base.prototype
assignment/readSuper() to assert the super precondition holds and only then
check the readProp(obj2) result.
In `@JSTests/stress/promise-prototype-finally-bug-312466.js`:
- Line 42: Test labels use awkward possessive punctuation (e.g., "this'") which
reduces readability; update the label strings passed to shouldThrow to use
correct wording such as "mimic this constructor does not have [[Construct]]:
${name}" or "mimic this object's constructor does not have [[Construct]]:
${name}" and apply the same wording cleanup to the other occurrences referenced
(lines showing similar labels around shouldThrow at the other two sites). Locate
the shouldThrow calls in the file (e.g., the call with template string
containing "mimic this' constructor") and replace the malformed "this'"/"it's"
fragments with the corrected phrasing for clearer failure output.
In `@JSTests/stress/temporal-timezone.js`:
- Line 1: The test currently unconditionally skips on Windows and Linux with the
line "//@ skip if $hostOS == "windows" || $hostOS == "linux""; add a short
rationale comment immediately above that line describing the specific failure
mode (e.g., timezone-related flakiness, failing assertions, or platform-specific
behavior), include the bug/issue tracker reference or bug ID that tracks the
fix, and note any repro steps or criteria for re-enabling the test (what must be
fixed or which platforms/builds should pass). Ensure the comment references the
exact skip condition string so future readers can correlate the rationale with
this skip.
In `@JSTests/wasm/debugger/lib/environment.py`:
- Around line 29-37: The method that computes vm_path and jsc_path when
self.build_config is set currently logs an error if jsc_path doesn't exist but
still returns the invalid paths; update this behavior in the function that
contains vm_path/jsc_path (the block using self.build_config, vm_path, jsc_path)
to avoid returning non-existent paths by either raising an exception immediately
when jsc_path.exists() is False or returning a clearly invalid sentinel (e.g.,
None) so callers must call validate(); ensure you reference and adjust the
caller validate() accordingly (or document that validate() will be invoked) so
downstream code doesn't receive an invalid path.
- Line 8: The import list in environment.py includes an unused symbol
log_verbose; remove log_verbose from the from .utils import ... statement so the
line reads from .utils import log_info, log_warning, log_error, log_success, and
then run the linter or tests to confirm no other references to log_verbose
remain in this module (search for log_verbose to ensure it's not used
elsewhere).
In `@JSTests/wasm/debugger/lib/session.py`:
- Around line 219-233: The reader thread in _start_reader (inner function _read)
should catch exceptions from stream.readline so the daemon thread doesn't die
silently; wrap the read loop in a try/except, log the exception (use
self._verbose printing or existing logger), and if ready_event is provided set
it (or set an error flag into self._q) so waiting code (e.g., _jsc_ready.wait)
fails fast instead of timing out; ensure you still drain/close the stream and
return after handling the exception.
In `@JSTests/wasm/debugger/tests/jsc.py`:
- Around line 32-33: The test result check currently only inspects result.stdout
for the string "0 failures found."—also verify the subprocess exit status by
checking result.returncode and treat any non-zero return code as a failure;
update the logic around the variable result in jsc.py (the block that raises
"testwasmdebugger failed") to first assert result.returncode == 0 (or combine
with the stdout check) and raise the exception when returncode != 0 or when "0
failures found." is missing to make failures explicit and robust.
In `@JSTests/wasm/debugger/tests/tests.py`:
- Around line 1525-1528: The class attribute extra_jsc_options on
WasmStackOverflowTrapTestCase is a mutable list; change it to an immutable tuple
(e.g., ("--maxPerThreadStackUsage=524288",)) or replace it with a `@property` that
returns a new list to avoid shared-mutation risks; update the declaration on the
WasmStackOverflowTrapTestCase class and ensure any consumers (like the test
runner that already copies via list(...)) still work by converting the tuple to
a list where needed.
In `@JSTests/wasm/multi-memory-spec-harness.js`:
- Around line 5-24: The code uses assert after import and drainMicrotasks() but
doesn't guard against a failed import; add an explicit check after
drainMicrotasks() to verify the imported symbol (assert) is defined and call
$vm.crash or throw a clear error if not. Concretely, after drainMicrotasks() add
a guard like if (typeof assert === "undefined" || !assert) $vm.crash("Failed to
import assert module") so subsequent uses (assert.eq, test, promise_test) won't
fail nondeterministically.
In `@JSTests/wasm/multi-memory-spec-harness/sync_index.js`:
- Around line 180-184: In the catch(e) block that throws new
Error('WebAssembly.Module ctor unexpectedly throws ${typeof e}:
${e}${e.stack}'), replace the single-quoted template with a proper interpolated
string (use backticks) or build the message via concatenation so the ${...}
expressions are evaluated; update the throw in the catch(e) inside sync_index.js
so the error message includes typeof e, e, and e.stack (refer to the catch(e)
block and the new Error(...) call).
In `@JSTests/wasm/stress/fmin-fmax-constant-fold.js`:
- Around line 65-117: The hot test loop recomputes floating constants each
iteration (e.g., Math.fround(Math.pow(2, -149)) used for
f32_min_denorm/f32_max_denorm, Math.pow(2, -1074) for f64 denormals, and
Math.fround(2.72)/Math.fround(3.14) for f32 normal checks), which adds JS
overhead; hoist these computed expected values (and any other non-trivial
computed expectations) into local constants before the for (let i = 0; i <
wasmTestLoopCount; ++i) loop and then use those variables in the assertions
referencing instance.exports.f32_min_denorm, f32_max_denorm, f64_min_denorm,
f64_max_denorm, f32_min_normal, f32_max_normal, etc., leaving simple literals
like NaN/Infinity unchanged.
In `@LayoutTests/accessibility/aria-hidden-deep-dom-no-crash.html`:
- Around line 2-6: Add a lang attribute to the <html> tag and add a <title>
element inside the <head> to satisfy HTML lint rules; update the existing <html>
start tag to include an appropriate lang value (e.g., lang="en") and insert a
brief <title> within the <head> section near the existing <script> includes so
the document has both language and title metadata.
In `@LayoutTests/accessibility/base-select-option-border-radius-path.html`:
- Around line 44-48: The test currently waits only for opt1 with
waitUntilIDHasPathWith("opt1", 1, "Curve to") then immediately asserts
pathSegmentCountOfID for opt1/opt2/opt3, which can race for opt2/opt3; update
the setup to wait for all option AXPaths before asserting by calling
waitUntilIDHasPathWith for "opt2" and "opt3" (or loop over
["opt1","opt2","opt3"]) to ensure each ID has the expected path segment before
running the expect checks that use pathSegmentCountOfID.
In `@LayoutTests/accessibility/base-select-option-press-closes-popover.html`:
- Line 2: The HTML test page is missing document language and title metadata;
update the root <html> element to include an appropriate lang attribute (e.g.,
lang="en") and add a <title> element inside the <head> section to provide page
title text; modify the existing <html> tag and ensure a
<head><title>…</title></head> is present so baseline HTML/accessibility checks
pass.
In `@LayoutTests/accessibility/element-path-with-css-transforms.html`:
- Line 2: The root <html> element is missing a lang attribute which triggers the
html-lang-require check; update the opening <html> tag to include an appropriate
language code (e.g., lang="en" or the document's actual language) so the element
<html> has a lang attribute, ensuring accessibility validators pass.
- Around line 52-53: The test currently only waits for item1's cached path via
waitUntilIDHasPathWith("item1", 1, "Curve to"), which can let assertions for
item2/item3 race; add additional waits using waitUntilIDHasPathWith for "item2"
and "item3" (matching the same expected path count/text pattern used for item1)
before running assertions so all elements' paths are populated and the test is
no longer flaky.
- Around line 3-33: Add a <title> element inside the existing <head> of the HTML
(in LayoutTests/accessibility/element-path-with-css-transforms.html) to satisfy
the title-require lint rule; place the title between the script/style tags and
the closing </head> so the document has a non-empty <title> (e.g., descriptive
text like "Element path with CSS transforms") and ensure it is the sole <title>
element in the head.
- Around line 67-68: The size-equality assertions only compare bounds1 and
bounds2, missing bounds3; update the checks that call expect (the lines using
expect("Math.abs(bounds1.width - bounds2.width) < 2", "true") and
expect("Math.abs(bounds1.height - bounds2.height) < 2", "true")) to also verify
bounds3 matches the others (e.g., compare bounds1.width to bounds3.width and
bounds1.height to bounds3.height or assert pairwise equality among bounds1,
bounds2, bounds3) so that item3's size deviations are caught; keep the same
tolerance logic and use the same expect invocation pattern.
In `@LayoutTests/accessibility/image-data.html`:
- Around line 2-10: Add missing document metadata and alt text: update the root
html element to include a lang attribute (e.g., lang="en"), add a <title>
element inside the <head> (e.g., "Accessibility image data test"), and add a
descriptive alt attribute to the image element with id "image" (e.g., alt="Slice
of cake"). Modify the <html> tag, the <head> contents to include <title>, and
the <img id="image"> element to include alt text so the test HTML is
standards-compliant and accessible.
In `@LayoutTests/accessibility/left-right-line-range-display-table-in-flex.html`:
- Around line 2-17: The HTML is missing document metadata: add a lang attribute
to the <html> element (e.g., lang="en") and insert a <title> element inside the
<head> (e.g., <title>...) so lint checks pass; update the top-level <html> tag
to include the lang and add a meaningful <title> within the existing <head>
block near the <script> and <style> entries.
In
`@LayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic.html`:
- Around line 2-7: Add the missing document language and title metadata by
setting the lang attribute on the <html> element (e.g., lang="en") and adding a
<title> element inside the <head> (e.g., <title>...</title>); update the
existing <html> tag and the <head> block in
live-region-relevant-text-non-atomic.html so the document has a lang attribute
and a non-empty <title>.
In `@LayoutTests/editing/execCommand/break-out-of-list-resets-start-number.html`:
- Around line 2-6: Add a lang attribute to the <html> element and include a
<title> element inside the <head> to satisfy HTML lint rules; update the file's
root <html> tag to include lang="en" (or appropriate language) and add a brief
<title> (e.g., "break-out-of-list-resets-start-number") in the <head> above or
below the existing <script> tags so HTMLHint no longer flags the document.
In `@LayoutTests/editing/execCommand/outdent-list-resets-start-number.html`:
- Around line 2-6: The HTML file is missing a lang attribute on the <html>
element and a <title> element in the <head>, causing lint failures; update the
<html> start tag to include an appropriate lang (e.g., <html lang="en">) and add
a minimal <title> inside the <head> (e.g., <title>Outdent list resets start
number</title>) so markup remains functionally identical but passes HTMLHint.
In
`@LayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash.html`:
- Around line 11-15: Wrap the enabling call
window.internals?.setImmediateRendererDestructionEnabled(true) in a try/finally
so setImmediateRendererDestructionEnabled(false) always runs: enable immediately
before the operations (document.body.offsetHeight;
document.getElementById('target').prepend('A'); document.body.offsetHeight;) and
put the disabling call in the finally block to guarantee cleanup even if an
exception occurs; reference the
window.internals?.setImmediateRendererDestructionEnabled call and the operations
on the 'target' element to locate where to add the try/finally.
In `@LayoutTests/fast/css/expedia-group-animation-quirk.html`:
- Around line 68-73: The test currently uses logical OR and thus passes if only
one expected timing is present; update the assertions to require both expected
values for delays and durations. Specifically, change the checks around
style.animationDelay (variable delays) to assert it includes both "0s" and
"0.06s" (or verify two entries and both exact values), and change the checks
around style.animationDuration (variable durations) to assert it includes both
"0.18s" and "0.06s" (or verify two entries and both exact values) so partial
regressions cannot pass.
- Around line 1-12: The test HTML is missing an explicit root element and title;
update the document to include a top-level <html> element wrapping the existing
<head> (and any body if added later) and add a <title> element inside the <head>
(above or before the existing script includes like
testharness.js/testharnessreport.js) so the page is well-formed and satisfies
the linter.
In
`@LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html`:
- Around line 3-12: Add a <title> element inside the existing <head> block so
the document meets HTML lint requirements; update the head that currently
contains meta and style (the block around <head> ... </head>) to include a
descriptive <title> (e.g., "Text Wrap Test") immediately after the <meta
charset="UTF-8"> line.
- Around line 18-26: The HTML opens nested divs (e.g., <div id="testArena">,
<div class="comment-thread">, <div class="comment">, <div
class="avatar-column">, the wrapping <div style="max-width: 684px"> and <div
class="comment-text">) that are not closed before </body>; add the corresponding
closing </div> tags in the same nesting order so each of those elements is
explicitly closed (close comment-text, the 684px wrapper, avatar-column/comment
container, comment-thread, and finally testArena) before the </body> to make the
repro deterministic.
In `@LayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf.html`:
- Around line 70-74: The test currently asserts indexOf.call(nodeList,
container.firstChild) and indexOf.call(nodeList, outside) but doesn't verify
that nodeList is live; update the test function to perform a DOM mutation (e.g.,
append a new child to container or remove container.firstChild) and then assert
indexOf.call(nodeList, theMutatedNode) reflects the change (for append: returns
the new index; for remove: returns -1), using the existing nodeList, container,
outside and container.firstChild identifiers so the liveness behavior is
validated.
In `@LayoutTests/fast/dom/tree-walker-disconnected-currentNode.html`:
- Line 27: The failure message passed to testFailed contains a grammar typo
("This tests requires GCController"); update the string literal in the
testFailed(...) call to correct grammar (e.g., "This test requires
GCController") by editing the argument to the testFailed function on that line
so the test output reads correctly.
In `@LayoutTests/fast/events/anchor-mouse-focusable-quirk.html`:
- Around line 5-8: Add missing document metadata by updating the root html
element and head: set a language on the <html> tag (e.g., <html lang="en">) and
add a minimal <title> element inside the <head> (e.g.,
<title>anchor-mouse-focusable-quirk</title>) so the test page includes both lang
and title metadata alongside the existing <script
src="../../resources/js-test.js"></script>.
In `@LayoutTests/fast/flexbox/deprecated-flex-box-writing-mode-shrink.html`:
- Around line 41-54: Add explicit HTML semantics to the replaced and form
elements: give the <img> element an alt attribute (e.g., alt=""), give the
<iframe> element a title attribute (e.g., title=""), and give the <button>
element an explicit type attribute (e.g., type="button") so there are no
implicit defaults; update the occurrences in the file where the <img>, <iframe>,
and <button> elements are defined (look for the img with data-expected-width,
the iframe with data-expected-width, and the button containing "XXXXXXX").
In `@LayoutTests/fast/forms/HTMLOptionElement_label02.html`:
- Line 5: The test's option element uses an explicit empty label (the <option
label=""> element) but then includes visible text "should display this", which
contradicts the stated intent to validate empty-label behavior; either remove
the inner text so the option has no visible label (leave <option
label=""></option>) to test empty-label rendering, or if the test intent
changed, update the test comment/description to reflect that a non-empty visible
label is expected and set the label attribute to a non-empty value (e.g.,
label="should display this") so markup and intent match.
In `@LayoutTests/fast/forms/ios/select-option-removed-update.html`:
- Line 28: The fixed 300ms sleep in the test (await UIHelper.delayFor(300)) is
brittle; replace it with a polling wait that repeatedly calls
UIHelper.selectMenuItems() until the expected count is observed after calling
select.remove(0). If UIHelper exposes a waitForCondition or similar helper, use
it to await the condition (items.length === expectedCount) with a sensible
timeout (e.g., 1000ms); if no polling helper exists, keep the delay change but
prefer adding a comment explaining the pragmatic choice.
In `@LayoutTests/fast/harness/results.html`:
- Around line 320-328: The code in static testPrefix (calling
testNameAndVariant) only removes the first separator with variant =
variant.slice(1), leaving leading '?' or '#' in variant which later become URL
query/fragment after encodeURI; update the sanitization of variant in testPrefix
so after slice(1) you strip or replace any leading '?' or '#' (or include them
in the invalid-character replacement) before building prefix so '#' and '?'
cannot survive into encodeURI and turn into URL fragments/queries (refer to
testPrefix, variant, prefix, testNameAndVariant, and the later encodeURI usage).
In `@LayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.html`:
- Line 3: The HTML contains an invalid closing tag for the void element embed
(the element with id "embed"); remove the trailing </embed> so the embed is a
void element (leave only the opening tag <embed id="embed" style="top: 200vmin;
visibility: hidden;" type="application/pdf" src="about:blank">) and ensure no
closing tag remains for the embed element.
In `@LayoutTests/fast/reflections/reflection-removed-crash-2.html`:
- Line 1: The test HTML lacks an explicit doctype causing it to run in quirks
mode; prepend a standards-mode doctype by inserting <!DOCTYPE html> at the very
top of the file before the existing <style> tag so the
reflection-removed-crash-2.html test is parsed in standards mode.
In `@LayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband.html`:
- Line 2: The file is missing an HTML lang attribute and a document title;
update the top-level <html> tag to include a lang attribute (e.g., lang="en")
and add a <head> section containing a <title> element with an appropriate title
for the test (e.g., "scroll-to-smooth-during-rubberband") so the document passes
HTMLHint checks; ensure the <html> and <head>/<title> changes are applied
consistently where the same pattern appears (also at the other occurrence
flagged).
- Around line 32-47: The test never asserts that the rubberband path was hit;
add an assertion that verifies the scroll handler actually set
triggeredScrollToZero to true after the drag and after
UIHelper.waitForTargetScrollAnimationToSettle(document.scrollingElement).
Specifically, after calling UIHelper.dragFromPointToPoint and
waitForTargetScrollAnimationToSettle, add an assertion (e.g., using shouldBe or
equivalent) that triggeredScrollToZero is true so the smooth-scroll branch in
the window.addEventListener("scroll", ...) handler was exercised before
asserting window.pageYOffset is 0.
In
`@LayoutTests/fast/scrolling/scroll-anchoring/scroll-anchoring-during-rubberband.html`:
- Around line 31-35: The test calls UIHelper.renderingUpdate() without checking
UIHelper exists, which can throw if the helper is unavailable; update the load
handler to guard UIHelper before invoking renderingUpdate (e.g., check
window.UIHelper and typeof UIHelper.renderingUpdate === 'function') and only
await UIHelper.renderingUpdate() when present, while keeping the existing check
for window.internals and
internals.unconstrainedScrollTo(document.scrollingElement, 0, -100).
In `@LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html`:
- Line 2: The root HTML element is missing a language attribute which triggers
the html-lang-require lint; update the root <html> tag (the file’s top-level
<html> element) to include a lang attribute (for example lang="en") so the
document declares its language and the lint passes.
In `@LayoutTests/fast/selectors/has-invalidation-traversal-size.html`:
- Around line 9-10: The global rule ":has(> .trigger) > .target" is leaking
across scenarios and masking failures; restrict its scope by moving or rewriting
that selector so it only targets the specific test container (e.g., prefix with
the scenario's wrapper class or move the rule into the scenario-specific <style>
block) so that only the intended test case's ".target" can turn green and other
scenarios remain isolated.
- Around line 2-4: The document is missing required metadata: add a lang
attribute to the <html> element (e.g., <html lang="en">) and include a <title>
element inside the <head> (e.g., <title>...) to satisfy lint rules; locate the
main <html> tag and the <head> section in the file and insert those
attributes/elements accordingly.
- Line 141: Remove the stray unmatched closing </script> tag that was added at
the end of the document (the extra "</script>" token); locate the orphaned
closing script tag and delete it so the HTML is well-formed (check around where
other script blocks end to ensure no corresponding opening <script> is missing).
In
`@LayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.html`:
- Around line 44-74: The load handler may reject when awaiting
UIHelper.requestDebugText and thus never call testRunner.notifyDone; wrap the
async body (the for loop and await UIHelper.requestDebugText calls) in a try
block and move testRunner.notifyDone() into a finally block so notifyDone always
runs; keep testRunner.waitUntilDone() before the try and preserve pushing
headings/containers into results inside the try; reference the load event
listener, UIHelper.requestDebugText, and
testRunner.notifyDone/testRunner.waitUntilDone when making the change.
In
`@LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html`:
- Around line 7-10: The feature-detection check is wrong because `!` binds
tighter than `in`, so replace the condition that currently references
"AudioDecoder" with a correct check (e.g., use the `!("AudioDecoder" in window)`
form or `typeof AudioDecoder === "undefined"`) so that the code correctly
detects absence of AudioDecoder and still calls window.testRunner?.notifyDone()
and returns; update the if condition in the same block that currently names
"AudioDecoder" and window.
In `@LayoutTests/TestExpectations`:
- Around line 5995-6002: These skipped test lines (e.g.,
imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.all.tentative.https.window.html,
imported/w3c/web-platform-tests/workers/SharedWorker-extendedLifetime.html,
etc.) need a webkit.org bug reference added so the skips are tracked; locate
those entries in LayoutTests/TestExpectations and, for each Skip line, append a
WebKit bug identifier or URL (for example "bug:
https://bugs.webkit.org/show_bug.cgi?id=NNNNNN" or "Bug NNNNNN") and commit the
updated expectation so each flaky skip has a corresponding bug reference.
- Line 7766: The Skip annotation for the test
SVGGeometry-isPointInStroke-with-null-path.html should include the same bug
reference as the related test SVGGeometry-isPointInFill-with-null-path.html
(webkit.org/b/290133); update the expectation line for
SVGGeometry-isPointInStroke-with-null-path.html to append the bug URL (e.g. "[
Skip (bug: webkit.org/b/290133) ]" or the project's preferred bug notation) so
both tests consistently reference the same issue.
- Around line 1702-1703: The two entries
imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm and
imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html lack an
expectation status and will break the TestExpectations parser; open the
TestExpectations file and either remove these two lines if they should no longer
be explicitly listed, or append the correct expectation token (for example `[
Pass ]`, `[ Skip ]`, or `[ ImageOnlyFailure ]`) to each entry so they follow the
required "test-path expectation" format, ensuring the tokens are added exactly
after the path strings for the functions that parse expectations.
---
Outside diff comments:
In `@LayoutTests/fast/css/vertical-text-overflow-ellipsis-text-align-center.html`:
- Around line 37-61: The four inline <img> elements in the
vertical-text-overflow-ellipsis test markup (the images inside the <div
class="testDiv"> blocks, including the RTL variants) lack alt attributes; add
alt="" to each of those <img src="resources/greenbox.png" width="25"
height="25"> occurrences to mark them decorative and satisfy
HTMLHint/accessibility checks while preserving layout and behavior.
In `@LayoutTests/fast/harness/results.html`:
- Around line 1467-1481: The ref-test links built in imageResultLinks (using
this._resultsController.layoutTestsBasePath() + testBase) can end up as bare
filesystem paths so resultLink() doesn't treat them as absolute; update
imageResultLinks to detect when the combined base+testBase is a local filesystem
path (e.g. does not start with 'http://' or 'file://') and, before calling
this._resultsController.resultLink for the 'reference' and 'ref mismatch' cases,
prepend the 'file://' scheme to the path so resultLink receives an absolute
file:// URL; apply the same normalization for both the isMatchRefTest() and
isMismatchRefTest() branches.
In `@LayoutTests/fast/text/font-face-empty-string.html`:
- Line 24: The test removed the check for assigning an empty string to
fontface.family, leaving that behavior unverified; add an explicit assertion
that assigning an empty string to fontface.family does not throw and yields the
expected value. Update the test to include a non-throw check (e.g. using
shouldNotThrow with the expression fontface.family = '' ) and then assert the
resulting fontface.family value (referencing the fontface variable and its
family property) to ensure the assignment both succeeds and produces the
intended empty-string state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de264970-7814-4ae6-99d9-5467262a654b
⛔ Files ignored due to path filters (2)
JSTests/wasm/debugger/resources/swift-wasm/fatal-error-test/fatal-error-test.wasmis excluded by!**/*.wasmJSTests/wasm/modules/wasm-imports-js-namespace-reexport/importer.wasmis excluded by!**/*.wasm
📒 Files selected for processing (298)
CMakeLists.txtCMakePresets.jsonConfigurations/CommonBase.xcconfigConfigurations/DeploymentTargets.xcconfigConfigurations/MakefileConfigurations/SDKAdditions.xcconfigConfigurations/Version.xcconfigConfigurations/WebKitProjectPaths.xcconfigConfigurations/WebKitTargetConditionals.xcconfigJSTests/complex.yamlJSTests/complex/intl-default-timezone-backward-link.jsJSTests/complex/intl-default-timezone-etc-gmt-offset.jsJSTests/complex/intl-default-timezone-etc-unknown.jsJSTests/complex/intl-default-timezone-region-alias.jsJSTests/complex/intl-default-timezone-three-letter-alias.jsJSTests/complex/intl-default-timezone-utc-alias.jsJSTests/microbenchmarks/arithmetic-shift-and-mask.jsJSTests/microbenchmarks/array-is-array-cse.jsJSTests/microbenchmarks/define-properties-slow-path.jsJSTests/microbenchmarks/json-parse-repeated-short-strings.jsJSTests/microbenchmarks/regexp-forward-reference.jsJSTests/microbenchmarks/rope-single-char-endsWith.jsJSTests/microbenchmarks/rope-single-char-startsWith.jsJSTests/stress/array-allocation-sink-double-hole-inline-materialization.jsJSTests/stress/array-allocation-sink-double-hole-osr-exit.jsJSTests/stress/array-allocation-sink-double-nan-not-hole.jsJSTests/stress/array-is-array-clobberize.jsJSTests/stress/array-is-array-proxy-effects.jsJSTests/stress/async-function-empty-resolved-promise.jsJSTests/stress/bigint-cached-mod-edge-cases.jsJSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.jsJSTests/stress/intl-canonical-iana-time-zone.jsJSTests/stress/intl-datetimeformat.jsJSTests/stress/intl-segmenter-word-iswordlike-number.jsJSTests/stress/iterator-prototype-includes.jsJSTests/stress/json-parse-jsstring-cache-coherence.jsJSTests/stress/megamorphic-cache-super-property.jsJSTests/stress/object-define-properties-proxy-trap-order.jsJSTests/stress/promise-prototype-finally-bug-312466.jsJSTests/stress/promise-prototype-finally-should-accept-non-promise-objects.jsJSTests/stress/promise-prototype-finally.jsJSTests/stress/regexp-jit-sibling-alt-frame-slot-collision.jsJSTests/stress/regexp-noncapturing-group-backreference-count.jsJSTests/stress/string-starts-ends-with-rope-single-char.jsJSTests/stress/temporal-timezone.jsJSTests/stress/yarr-jit-fixedcount-paren-context-free-on-skip.jsJSTests/test262/config.yamlJSTests/test262/expectations.yamlJSTests/wasm.yamlJSTests/wasm/debugger/lib/__init__.pyJSTests/wasm/debugger/lib/core/__init__.pyJSTests/wasm/debugger/lib/core/base.pyJSTests/wasm/debugger/lib/core/environment.pyJSTests/wasm/debugger/lib/core/registry.pyJSTests/wasm/debugger/lib/core/utils.pyJSTests/wasm/debugger/lib/discovery/__init__.pyJSTests/wasm/debugger/lib/discovery/auto_discovery.pyJSTests/wasm/debugger/lib/environment.pyJSTests/wasm/debugger/lib/runners/__init__.pyJSTests/wasm/debugger/lib/runners/parallel.pyJSTests/wasm/debugger/lib/runners/process_manager.pyJSTests/wasm/debugger/lib/runners/sequential.pyJSTests/wasm/debugger/lib/session.pyJSTests/wasm/debugger/lib/utils.pyJSTests/wasm/debugger/resources/c-wasm/add/main.jsJSTests/wasm/debugger/resources/swift-wasm/build.shJSTests/wasm/debugger/resources/swift-wasm/crash-test/main.jsJSTests/wasm/debugger/resources/swift-wasm/do-catch-throw/main.jsJSTests/wasm/debugger/resources/swift-wasm/dynamic-module-load/main.jsJSTests/wasm/debugger/resources/swift-wasm/fatal-error-test/Package.swiftJSTests/wasm/debugger/resources/swift-wasm/fatal-error-test/Sources/fatal-error-test/main.swiftJSTests/wasm/debugger/resources/swift-wasm/fatal-error-test/main.jsJSTests/wasm/debugger/resources/swift-wasm/globals-test/main.jsJSTests/wasm/debugger/resources/swift-wasm/test/main.jsJSTests/wasm/debugger/resources/wasm/call-indirect.jsJSTests/wasm/debugger/resources/wasm/call-ref.jsJSTests/wasm/debugger/resources/wasm/call.jsJSTests/wasm/debugger/resources/wasm/delegate.jsJSTests/wasm/debugger/resources/wasm/dynamic-module-load.jsJSTests/wasm/debugger/resources/wasm/js-js-wasm-js-js-wasm.jsJSTests/wasm/debugger/resources/wasm/js-wasm-js-wasm.jsJSTests/wasm/debugger/resources/wasm/multi-vm-same-module-different-funcs.jsJSTests/wasm/debugger/resources/wasm/multi-vm-same-module-same-func.jsJSTests/wasm/debugger/resources/wasm/named-streaming-module.jsJSTests/wasm/debugger/resources/wasm/nop-drop-select-end.jsJSTests/wasm/debugger/resources/wasm/rethrow.jsJSTests/wasm/debugger/resources/wasm/return-call-indirect.jsJSTests/wasm/debugger/resources/wasm/return-call-ref.jsJSTests/wasm/debugger/resources/wasm/return-call.jsJSTests/wasm/debugger/resources/wasm/streaming-module-load.jsJSTests/wasm/debugger/resources/wasm/system-call.jsJSTests/wasm/debugger/resources/wasm/throw-catch-all.jsJSTests/wasm/debugger/resources/wasm/throw-catch.jsJSTests/wasm/debugger/resources/wasm/throw-ref.jsJSTests/wasm/debugger/resources/wasm/trap-div-by-zero.jsJSTests/wasm/debugger/resources/wasm/trap-oob-memory.jsJSTests/wasm/debugger/resources/wasm/trap-out-of-bounds-call-indirect.jsJSTests/wasm/debugger/resources/wasm/trap-stack-overflow.jsJSTests/wasm/debugger/resources/wasm/try-table.jsJSTests/wasm/debugger/resources/wasm/unreachable.jsJSTests/wasm/debugger/resources/wasm/url-named-streaming-module.jsJSTests/wasm/debugger/resources/wasm/wasm-js-wasm-js-wasm.jsJSTests/wasm/debugger/resources/wasm/wasm-wasm-js-wasm-wasm.jsJSTests/wasm/debugger/resources/wasm/wasm-wasm-wasm.jsJSTests/wasm/debugger/test-wasm-debugger.pyJSTests/wasm/debugger/tests/__init__.pyJSTests/wasm/debugger/tests/jsc.pyJSTests/wasm/debugger/tests/tests.pyJSTests/wasm/modules/wasm-imports-js-namespace-reexport.jsJSTests/wasm/modules/wasm-imports-js-namespace-reexport/importer.watJSTests/wasm/modules/wasm-imports-js-namespace-reexport/inner.jsJSTests/wasm/modules/wasm-imports-js-namespace-reexport/reexport.jsJSTests/wasm/multi-memory-spec-harness.jsJSTests/wasm/multi-memory-spec-harness/sync_index.jsJSTests/wasm/multi-memory-spec-tests/address0.wast.jsJSTests/wasm/multi-memory-spec-tests/address1.wast.jsJSTests/wasm/multi-memory-spec-tests/align0.wast.jsJSTests/wasm/multi-memory-spec-tests/binary0.wast.jsJSTests/wasm/multi-memory-spec-tests/data0.wast.jsJSTests/wasm/multi-memory-spec-tests/data1.wast.jsJSTests/wasm/multi-memory-spec-tests/data_drop0.wast.jsJSTests/wasm/multi-memory-spec-tests/exports0.wast.jsJSTests/wasm/multi-memory-spec-tests/float_exprs0.wast.jsJSTests/wasm/multi-memory-spec-tests/float_exprs1.wast.jsJSTests/wasm/multi-memory-spec-tests/float_memory0.wast.jsJSTests/wasm/multi-memory-spec-tests/imports0.wast.jsJSTests/wasm/multi-memory-spec-tests/imports1.wast.jsJSTests/wasm/multi-memory-spec-tests/imports2.wast.jsJSTests/wasm/multi-memory-spec-tests/imports3.wast.jsJSTests/wasm/multi-memory-spec-tests/imports4.wast.jsJSTests/wasm/multi-memory-spec-tests/linking0.wast.jsJSTests/wasm/multi-memory-spec-tests/linking1.wast.jsJSTests/wasm/multi-memory-spec-tests/linking2.wast.jsJSTests/wasm/multi-memory-spec-tests/linking3.wast.jsJSTests/wasm/multi-memory-spec-tests/load0.wast.jsJSTests/wasm/multi-memory-spec-tests/load1.wast.jsJSTests/wasm/multi-memory-spec-tests/load2.wast.jsJSTests/wasm/multi-memory-spec-tests/memory-multi.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_copy0.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_copy1.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_fill0.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_grow.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_init0.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_size0.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_size1.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_size2.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_size3.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_size_import.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_trap0.wast.jsJSTests/wasm/multi-memory-spec-tests/memory_trap1.wast.jsJSTests/wasm/multi-memory-spec-tests/start0.wast.jsJSTests/wasm/multi-memory-spec-tests/store0.wast.jsJSTests/wasm/multi-memory-spec-tests/store1.wast.jsJSTests/wasm/multi-memory-spec-tests/store2.wast.jsJSTests/wasm/multi-memory-spec-tests/traps0.wast.jsJSTests/wasm/stress/data-segment-multimemory.jsJSTests/wasm/stress/fmin-fmax-constant-fold.jsJSTests/wasm/stress/int-div-rem-constant-folding.jsJSTests/wasm/stress/simd-const-relaxed-dot-product.jsJSTests/wasm/stress/simd-const-relaxed-f32-madd.jsJSTests/wasm/stress/simd-const-relaxed-f32-trunc.jsJSTests/wasm/stress/simd-const-relaxed-f64-madd.jsJSTests/wasm/stress/simd-const-relaxed-f64-trunc.jsJSTests/wasm/stress/simd-const-relaxed-lane-select.jsJSTests/wasm/stress/simd-const-relaxed-min-max.jsJSTests/wasm/stress/simd-const-relaxed-q15mulr.jsJSTests/wasm/stress/simd-const-relaxed-swizzle.jsJSTests/wasm/stress/wide-arithmetic-const-inputs.jsJSTests/wasm/stress/wide-arithmetic-unreachable.jsLayoutTests/TestExpectationsLayoutTests/accessibility/aria-hidden-deep-dom-no-crash-expected.txtLayoutTests/accessibility/aria-hidden-deep-dom-no-crash.htmlLayoutTests/accessibility/aria-hidden-focus-not-overridden-when-caught-expected.txtLayoutTests/accessibility/aria-hidden-focus-not-overridden-when-caught.htmlLayoutTests/accessibility/aria-hidden-focus-override-expected.txtLayoutTests/accessibility/aria-hidden-focus-override.htmlLayoutTests/accessibility/base-select-option-border-radius-path-expected.txtLayoutTests/accessibility/base-select-option-border-radius-path.htmlLayoutTests/accessibility/base-select-option-press-closes-popover-expected.txtLayoutTests/accessibility/base-select-option-press-closes-popover.htmlLayoutTests/accessibility/crash-table-recursive-layout-expected.txtLayoutTests/accessibility/element-path-with-css-transforms-expected.txtLayoutTests/accessibility/element-path-with-css-transforms.htmlLayoutTests/accessibility/focusable-inside-hidden-expected.txtLayoutTests/accessibility/focusable-inside-hidden.htmlLayoutTests/accessibility/image-data-expected.txtLayoutTests/accessibility/image-data.htmlLayoutTests/accessibility/image-link-expected.txtLayoutTests/accessibility/left-right-line-range-display-table-in-flex-expected.txtLayoutTests/accessibility/left-right-line-range-display-table-in-flex.htmlLayoutTests/accessibility/mac/client/absolute-position-iframe.htmlLayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic-expected.txtLayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic.htmlLayoutTests/compositing/clipping/border-radius-async-overflow-stacking.htmlLayoutTests/compositing/reflections/opacity-in-reflection.htmlLayoutTests/css3/filters/backdrop/backdrop-filter-nested.htmlLayoutTests/css3/filters/backdrop/backdrop-filter-with-reflection-value-change.htmlLayoutTests/css3/filters/blur-filter-page-scroll-parents.htmlLayoutTests/css3/filters/blur-filter-page-scroll-self.htmlLayoutTests/css3/filters/blur-filter-page-scroll.htmlLayoutTests/css3/filters/effect-brightness-clamping.htmlLayoutTests/css3/filters/filter-repaint.htmlLayoutTests/css3/flexbox/flexitem.htmlLayoutTests/editing/execCommand/break-out-of-list-resets-start-number-expected.txtLayoutTests/editing/execCommand/break-out-of-list-resets-start-number.htmlLayoutTests/editing/execCommand/outdent-list-resets-start-number-expected.txtLayoutTests/editing/execCommand/outdent-list-resets-start-number.htmlLayoutTests/editing/spelling/grammar-correction-shown-clicking-middle-of-word-expected.txtLayoutTests/editing/spelling/grammar-correction-shown-clicking-middle-of-word.htmlLayoutTests/fast/animation/css-animation-throttling-expected.txtLayoutTests/fast/animation/css-animation-throttling.htmlLayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash-expected.txtLayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash.htmlLayoutTests/fast/css/expedia-group-animation-quirk-expected.txtLayoutTests/fast/css/expedia-group-animation-quirk.htmlLayoutTests/fast/css/hotels-animation-quirk-expected.txtLayoutTests/fast/css/hotels-animation-quirk.htmlLayoutTests/fast/css/vertical-text-overflow-ellipsis-text-align-center.htmlLayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6-expected.txtLayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.htmlLayoutTests/fast/dom/Document/open-triggered-by-mutation-observer-on-element-from-a-parsed-doc-crash-expected.txtLayoutTests/fast/dom/Document/open-triggered-by-mutation-observer-on-element-from-a-parsed-doc-crash.htmlLayoutTests/fast/dom/HTMLFormElement/associated-elements-after-index-assertion-fail2-expected.txtLayoutTests/fast/dom/HTMLLabelElement/label-control-expected.txtLayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf-expected.txtLayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf.htmlLayoutTests/fast/dom/gc-dom-tree-lifetime-shadow-tree.htmlLayoutTests/fast/dom/gc-dom-tree-lifetime.htmlLayoutTests/fast/dom/tree-walker-disconnected-currentNode-expected.txtLayoutTests/fast/dom/tree-walker-disconnected-currentNode.htmlLayoutTests/fast/events/anchor-mouse-focusable-quirk-expected.txtLayoutTests/fast/events/anchor-mouse-focusable-quirk.htmlLayoutTests/fast/flexbox/deprecated-flex-box-writing-mode-shrink-expected.txtLayoutTests/fast/flexbox/deprecated-flex-box-writing-mode-shrink.htmlLayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-deep-expected.htmlLayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-deep.htmlLayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-expected.htmlLayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item.htmlLayoutTests/fast/forms/HTMLOptionElement_label02-expected.htmlLayoutTests/fast/forms/HTMLOptionElement_label02.htmlLayoutTests/fast/forms/HTMLOptionElement_label04-expected.htmlLayoutTests/fast/forms/HTMLOptionElement_label04.htmlLayoutTests/fast/forms/appearance-base/internal-auto-base-function-expected.htmlLayoutTests/fast/forms/fieldset/fieldset-deprecated-flexbox-expected.htmlLayoutTests/fast/forms/ios/select-option-removed-update.htmlLayoutTests/fast/forms/label/labelable-elements-expected.txtLayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txtLayoutTests/fast/forms/label/labels-add-parent-label-expected.txtLayoutTests/fast/forms/label/labels-change-htmlFor-attribute-expected.txtLayoutTests/fast/forms/label/labels-multiple-sibling-labels-expected.txtLayoutTests/fast/forms/label/labels-parent-and-sibling-labels-expected.txtLayoutTests/fast/forms/label/labels-remove-htmlFor-attribute-expected.txtLayoutTests/fast/forms/label/labels-remove-htmlFor-label-expected.txtLayoutTests/fast/forms/label/labels-remove-parent-label-expected.txtLayoutTests/fast/forms/menulist-no-renderer-for-unexpected-children-expected.txtLayoutTests/fast/forms/menulist-no-renderer-for-unexpected-children.htmlLayoutTests/fast/forms/select/menulist-focusable-items-expected.txtLayoutTests/fast/harness/full_results.jsonLayoutTests/fast/harness/results-expected.txtLayoutTests/fast/harness/results.htmlLayoutTests/fast/loader/main-document-url-for-non-http-loads-expected.txtLayoutTests/fast/loader/resources/subframe-notify-done.htmlLayoutTests/fast/mediasession/metadata/artworkdownload.htmlLayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash-expected.txtLayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.htmlLayoutTests/fast/reflections/reflection-removed-crash-2-expected.txtLayoutTests/fast/reflections/reflection-removed-crash-2.htmlLayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband-expected.txtLayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband.htmlLayoutTests/fast/scrolling/scroll-anchoring/scroll-anchoring-during-rubberband-expected.txtLayoutTests/fast/scrolling/scroll-anchoring/scroll-anchoring-during-rubberband.htmlLayoutTests/fast/selectors/focus-visible-script-focus-from-click-expected.txtLayoutTests/fast/selectors/focus-visible-script-focus-from-click.htmlLayoutTests/fast/selectors/has-invalidation-traversal-size-expected.txtLayoutTests/fast/selectors/has-invalidation-traversal-size.htmlLayoutTests/fast/shadow-dom/shadow-at-root-during-disconnect.htmlLayoutTests/fast/table/border-collapsing/004-vertical-expected.txtLayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox-expected.txtLayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.htmlLayoutTests/fast/text/font-face-empty-string-expected.txtLayoutTests/fast/text/font-face-empty-string.htmlLayoutTests/fast/text/font-face-family-expected.txtLayoutTests/fast/text/font-face-family.htmlLayoutTests/fast/text/range-bounding-rect-across-lines-expected.txtLayoutTests/fast/text/range-bounding-rect-across-lines.htmlLayoutTests/fast/text/tatechuyoko-expected.txtLayoutTests/fast/viewport/ios/shrink-to-fit-content-large-width-breakpoint-expected.txtLayoutTests/fast/viewport/ios/shrink-to-fit-content-responsive-viewport-with-horizontal-overflow-expected.txtLayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash-expected.txtLayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.htmlLayoutTests/fast/webgpu/ftoi.htmlLayoutTests/fast/webgpu/nocrash/fuzz-273021.htmlLayoutTests/fast/webgpu/nocrash/fuzz-273585.htmlLayoutTests/fast/webgpu/nocrash/fuzz-274289.htmlLayoutTests/fast/webgpu/nocrash/fuzz-274334.htmlLayoutTests/fast/webgpu/nocrash/fuzz-275294.htmlLayoutTests/fast/webgpu/nocrash/fuzz-276279.htmlLayoutTests/fast/webgpu/nocrash/fuzz-277035.html
💤 Files with no reviewable changes (30)
- JSTests/stress/iterator-prototype-includes.js
- LayoutTests/css3/filters/blur-filter-page-scroll.html
- LayoutTests/css3/filters/blur-filter-page-scroll-self.html
- LayoutTests/css3/filters/blur-filter-page-scroll-parents.html
- LayoutTests/css3/filters/effect-brightness-clamping.html
- LayoutTests/fast/css/hotels-animation-quirk-expected.txt
- LayoutTests/fast/css/hotels-animation-quirk.html
- Configurations/WebKitProjectPaths.xcconfig
- JSTests/test262/config.yaml
- JSTests/test262/expectations.yaml
- JSTests/wasm/debugger/lib/init.py
- JSTests/wasm/debugger/lib/core/environment.py
- JSTests/wasm/debugger/lib/core/registry.py
- JSTests/wasm/debugger/lib/core/utils.py
- JSTests/wasm/debugger/lib/discovery/auto_discovery.py
- JSTests/wasm/debugger/lib/runners/parallel.py
- JSTests/wasm/debugger/lib/runners/process_manager.py
- LayoutTests/css3/filters/filter-repaint.html
- LayoutTests/fast/text/font-face-empty-string-expected.txt
- Configurations/CommonBase.xcconfig
- Configurations/DeploymentTargets.xcconfig
- Configurations/SDKAdditions.xcconfig
- Configurations/Version.xcconfig
- JSTests/wasm/debugger/lib/runners/sequential.py
- JSTests/wasm/debugger/lib/core/base.py
- LayoutTests/fast/webgpu/nocrash/fuzz-277035.html
- LayoutTests/fast/webgpu/nocrash/fuzz-274334.html
- LayoutTests/fast/webgpu/nocrash/fuzz-274289.html
- LayoutTests/fast/webgpu/nocrash/fuzz-275294.html
- LayoutTests/fast/webgpu/nocrash/fuzz-273585.html
| let re1 = /\2(a)(b)/; | ||
| let re2 = /\k<y>(?<x>a)(?<y>b)/; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use const for regex bindings.
re1 and re2 are never reassigned; const better communicates intent.
Proposed diff
- let re1 = /\2(a)(b)/;
- let re2 = /\k<y>(?<x>a)(?<y>b)/;
+ const re1 = /\2(a)(b)/;
+ const re2 = /\k<y>(?<x>a)(?<y>b)/;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let re1 = /\2(a)(b)/; | |
| let re2 = /\k<y>(?<x>a)(?<y>b)/; | |
| const re1 = /\2(a)(b)/; | |
| const re2 = /\k<y>(?<x>a)(?<y>b)/; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/microbenchmarks/regexp-forward-reference.js` around lines 4 - 5,
Change the mutable bindings for the regular expressions to immutable ones:
replace the declarations of re1 and re2 (the variables defined as "let re1 =
/\2(a)(b)/;" and "let re2 = /\k<y>(?<x>a)(?<y>b)/;") with const declarations so
they read as constant bindings, since neither re1 nor re2 is reassigned.
| shouldBe(await onlyUsing(), undefined); | ||
| shouldBe(disposedSync, 1); | ||
|
|
||
| shouldBe(await onlyAwaitUsing(), undefined); | ||
| shouldBe(disposedAsync, 1); |
There was a problem hiding this comment.
Fix disposal counter assertions across repeated run() calls.
run() executes 1000 times (Line 81-82), but Lines 48 and 51 assert absolute value 1. The second iteration will fail (2 !== 1), so the stress test fails for the wrong reason.
Suggested fix
async function run() {
@@
- shouldBe(await onlyUsing(), undefined);
- shouldBe(disposedSync, 1);
+ const syncBefore = disposedSync;
+ shouldBe(await onlyUsing(), undefined);
+ shouldBe(disposedSync, syncBefore + 1);
@@
- shouldBe(await onlyAwaitUsing(), undefined);
- shouldBe(disposedAsync, 1);
+ const asyncBefore = disposedAsync;
+ shouldBe(await onlyAwaitUsing(), undefined);
+ shouldBe(disposedAsync, asyncBefore + 1);Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/async-function-empty-resolved-promise.js` around lines 47 -
51, The disposal counter assertions are wrong because run() invokes the test
loop 1000 times so disposedSync/disposedAsync will grow beyond 1; update the
checks around shouldBe(await onlyUsing(), undefined) / shouldBe(await
onlyAwaitUsing(), undefined) to assert the counters equal the current iteration
count (or reset disposedSync/disposedAsync to 0 at the start of each run
iteration) rather than the hard-coded 1 so repeated run() calls pass; locate
references to onlyUsing, onlyAwaitUsing, and the disposedSync/disposedAsync
counters to implement the fix.
| try { | ||
| if (sab.byteLength < ${maxBytes}) { | ||
| sab.grow(sab.byteLength + 64); | ||
| } | ||
| } catch (e) {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid swallowing unexpected exceptions in stress loops.
Catching everything can hide real regressions. Prefer rethrowing non-expected exception types and only tolerating resize-related failures.
♻️ Proposed refinement
- } catch (e) {}
+ } catch (e) {
+ if (!(e instanceof RangeError || e instanceof TypeError))
+ throw e;
+ }
...
- } catch (e) {}
+ } catch (e) {
+ if (!(e instanceof RangeError || e instanceof TypeError))
+ throw e;
+ }Also applies to: 27-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js`
around lines 13 - 17, The try/catch around sab.grow silently swallows all
exceptions; change it to only tolerate expected resize failures and rethrow
unexpected errors—for example replace the empty catch with logic that catches
only the expected exception type (e.g., RangeError or the concrete exception
your runtime throws for failed grow) or checks the error signature (e instanceof
RangeError) and rethrows otherwise; apply the same change to the second
identical block (the other sab.grow try/catch at lines 27-31) so genuine
regressions aren't hidden while still allowing resize failures in the stress
loop.
| $.agent.sleep(50); | ||
| $.agent.getReport(); |
There was a problem hiding this comment.
Assert the expected agent report value.
Line 35 currently ignores the report payload, which can allow false positives if the worker reports an unexpected state.
🧪 Proposed fix
$.agent.sleep(50);
- $.agent.getReport();
+ const report = $.agent.getReport();
+ if (report !== "done")
+ throw new Error(`Unexpected agent report: ${report}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js`
around lines 34 - 35, The test currently ignores the worker report returned by
$.agent.getReport(); capture its return value and assert it matches the expected
payload to avoid false positives. Replace the ignored call with code that
assigns the result of $.agent.getReport() to a variable (e.g., report), then use
the test harness assertion (e.g., assertEq or assert.sameValue) to compare
report against the expectedReport value previously sent by the worker; reference
$.agent.getReport() and expectedReport (or the concrete expected value) when
making the change.
| for (let { segment, index, isWordLike } of segmenter.segment(input)) { | ||
| let result = results[cursor++]; | ||
| shouldBe(result[0], index); | ||
| shouldBe(result[1], index + segment.length); | ||
| shouldBe(result[2], segment); | ||
| shouldBe(result[3], isWordLike); | ||
| } | ||
| shouldBe(cursor, results.length); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting repeated segment-assert loop into a helper.
This is repeated four times and can be centralized to reduce maintenance overhead when expected tuple shape evolves.
♻️ Optional refactor sketch
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error('bad value: ' + actual + ' expected: ' + expected);
}
+
+function assertSegments(segmenter, input, results) {
+ let cursor = 0;
+ for (let { segment, index, isWordLike } of segmenter.segment(input)) {
+ let result = results[cursor++];
+ shouldBe(result[0], index);
+ shouldBe(result[1], index + segment.length);
+ shouldBe(result[2], segment);
+ shouldBe(result[3], isWordLike);
+ }
+ shouldBe(cursor, results.length);
+}Then each case can become:
- let cursor = 0;
- for (let { segment, index, isWordLike } of segmenter.segment(input)) {
- let result = results[cursor++];
- shouldBe(result[0], index);
- shouldBe(result[1], index + segment.length);
- shouldBe(result[2], segment);
- shouldBe(result[3], isWordLike);
- }
- shouldBe(cursor, results.length);
+ assertSegments(segmenter, input, results);Also applies to: 43-50, 64-71, 89-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/intl-segmenter-word-iswordlike-number.js` around lines 22 -
29, Extract the repeated loop that iterates over segmenter.segment(input) and
asserts the tuple into a single helper (e.g., assertSegments or checkSegments)
that accepts the segmenter, input string, and expected results array; inside the
helper iterate for (let { segment, index, isWordLike } of
segmenter.segment(input)), assert using shouldBe(result[0], index),
shouldBe(result[1], index + segment.length), shouldBe(result[2], segment),
shouldBe(result[3], isWordLike), and finally assert the cursor equals
results.length, then replace the four duplicated loops with calls to this helper
so callers no longer duplicate cursor/iteration/assert logic.
| @@ -0,0 +1,93 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
There was a problem hiding this comment.
Add lang on the root <html> element.
This file currently fails HTML lint (html-lang-require). Add a language tag to keep test assets lint-clean.
Suggested fix
-<html>
+<html lang="en">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <html> | |
| <html lang="en"> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 2-2: An lang attribute must be present on elements.
(html-lang-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html` at
line 2, The root HTML element is missing a language attribute which triggers the
html-lang-require lint; update the root <html> tag (the file’s top-level <html>
element) to include a lang attribute (for example lang="en") so the document
declares its language and the lint passes.
| if (!"AudioDecoder" in window) { | ||
| window.testRunner?.notifyDone(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical: Fix the feature detection syntax.
The operator precedence is incorrect. The ! operator binds tighter than in, so this evaluates as (!"AudioDecoder") in window, which checks whether the boolean false exists as a property in the window object, rather than checking if AudioDecoder is missing.
🐛 Proposed fix for feature detection
- if (!"AudioDecoder" in window) {
+ if (!("AudioDecoder" in window)) {
window.testRunner?.notifyDone();
return;
}Alternatively, you can use:
- if (!"AudioDecoder" in window) {
+ if (typeof AudioDecoder === "undefined") {
window.testRunner?.notifyDone();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!"AudioDecoder" in window) { | |
| window.testRunner?.notifyDone(); | |
| return; | |
| } | |
| if (!("AudioDecoder" in window)) { | |
| window.testRunner?.notifyDone(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html`
around lines 7 - 10, The feature-detection check is wrong because `!` binds
tighter than `in`, so replace the condition that currently references
"AudioDecoder" with a correct check (e.g., use the `!("AudioDecoder" in window)`
form or `typeof AudioDecoder === "undefined"`) so that the code correctly
detects absence of AudioDecoder and still calls window.testRunner?.notifyDone()
and returns; update the if condition in the same block that currently names
"AudioDecoder" and window.
| imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm | ||
| imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html |
There was a problem hiding this comment.
Missing expectation status — test runner will fail to parse these entries.
Lines 1702-1703 are missing their expectation status (e.g., [ Pass ], [ Skip ], [ ImageOnlyFailure ]). The TestExpectations format requires each entry to have an expectation:
Proposed fix (assuming these should be removed from failure expectations)
If these tests are now passing and should be removed from explicit expectations, delete these lines entirely. If they still need expectations, add the appropriate status:
-imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm
-imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html
+imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm [ Pass ]
+imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html [ Pass ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm | |
| imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html | |
| imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm [ Pass ] | |
| imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html [ Pass ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/TestExpectations` around lines 1702 - 1703, The two entries
imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm and
imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html lack an
expectation status and will break the TestExpectations parser; open the
TestExpectations file and either remove these two lines if they should no longer
be explicitly listed, or append the correct expectation token (for example `[
Pass ]`, `[ Skip ]`, or `[ ImageOnlyFailure ]`) to each entry so they follow the
required "test-path expectation" format, ensuring the tokens are added exactly
after the path strings for the functions that parse expectations.
| # Tests are flaky since import. | ||
| imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.all.tentative.https.window.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.default.tentative.https.window.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.none.tentative.https.window.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/SharedWorker-extendedLifetime-named-module.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/SharedWorker-extendedLifetime.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/shared-worker-partitioned-cookies.tentative.https.html [ Skip ] | ||
| imported/w3c/web-platform-tests/workers/shared-worker-from-blob-url.window.html [ Skip ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding bug tracker references for flaky test skips.
These newly skipped tests are marked as "flaky since import" but lack webkit.org bug references. Filing tracking bugs helps ensure these tests are revisited and not permanently forgotten.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/TestExpectations` around lines 5995 - 6002, These skipped test
lines (e.g.,
imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.all.tentative.https.window.html,
imported/w3c/web-platform-tests/workers/SharedWorker-extendedLifetime.html,
etc.) need a webkit.org bug reference added so the skips are tracked; locate
those entries in LayoutTests/TestExpectations and, for each Skip line, append a
WebKit bug identifier or URL (for example "bug:
https://bugs.webkit.org/show_bug.cgi?id=NNNNNN" or "Bug NNNNNN") and commit the
updated expectation so each flaky skip has a corresponding bug reference.
|
|
||
| # This test hits an assert in debug | ||
| webkit.org/b/290133 [ Debug ] svg/dom/SVGGeometry-isPointInFill-with-null-path.html [ Skip ] | ||
| [ Debug ] svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding the same bug reference for consistency.
The related test SVGGeometry-isPointInFill-with-null-path.html above references webkit.org/b/290133. If this is the same underlying issue, adding the bug reference would improve traceability.
-[ Debug ] svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ]
+webkit.org/b/290133 [ Debug ] svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [ Debug ] svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ] | |
| webkit.org/b/290133 [ Debug ] svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/TestExpectations` at line 7766, The Skip annotation for the test
SVGGeometry-isPointInStroke-with-null-path.html should include the same bug
reference as the related test SVGGeometry-isPointInFill-with-null-path.html
(webkit.org/b/290133); update the expectation line for
SVGGeometry-isPointInStroke-with-null-path.html to append the bug URL (e.g. "[
Skip (bug: webkit.org/b/290133) ]" or the project's preferred bug notation) so
both tests consistently reference the same issue.
…r clang-cl Defaulted operator<=> inside a class template makes clang-cl emit MSVC STL's consteval _Literal_zero ctor as a regular call, referencing the _Literal_zero_is_expected poison symbol at link time. Spell out the relational operators on m_value directly under COMPILER(CLANG) && OS(WINDOWS).
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
LayoutTests/fast/text/font-face-empty-string.html (1)
24-24:⚠️ Potential issue | 🟡 MinorAdd an explicit check for
fontface.family = ''.Right now the test no longer executes the empty-string assignment, so it cannot catch regressions for that specific behavior.
Proposed patch
+shouldNotThrow("fontface.family = ''"); shouldNotThrow("fontface.family = '\"\"'");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/text/font-face-empty-string.html` at line 24, The test currently only exercises assignment of a string containing quotes; add an explicit check that assigns an actual empty string to the FontFace.family to catch regressions: invoke shouldNotThrow with the expression that sets fontface.family = '' (i.e., ensure the test runs shouldNotThrow("fontface.family = ''") or otherwise directly execute fontface.family = '' inside a shouldNotThrow wrapper) so the empty-string assignment is exercised; reference the existing shouldNotThrow call and the fontface.family assignment in the test to locate where to add this new assertion.JSTests/wasm/debugger/tests/jsc.py (1)
19-30:⚠️ Potential issue | 🟠 MajorAvoid
shell=Truefor the test runner command.This command is a single script path plus fixed flags. With
shell=True, a checkout path containing spaces will break the invocation, and the shell adds unnecessary parsing complexity. Pass an argv list tosubprocess.run()instead.Proposed fix
- cmd = f"{self.env.webkit_root}/Tools/Scripts/run-javascriptcore-tests --debug --no-build --testwasmdebugger" + cmd = [ + str(self.env.webkit_root / "Tools/Scripts/run-javascriptcore-tests"), + "--debug", + "--no-build", + "--testwasmdebugger", + ] @@ result = subprocess.run( cmd, - shell=True, timeout=120, capture_output=True, text=True, env=self.env.env, cwd=str(self.env.webkit_root),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/wasm/debugger/tests/jsc.py` around lines 19 - 30, The test runner uses subprocess.run(...) with shell=True and a single cmd string (cmd) which breaks on paths with spaces; change it to pass an argv list instead: replace the string cmd and the subprocess.run(...) call in jsc.py so that you build a list like [f"{self.env.webkit_root}/Tools/Scripts/run-javascriptcore-tests", "--debug", "--no-build", "--testwasmdebugger"] (or construct the script path and flags separately), remove shell=True, and pass that list as the first argument to subprocess.run while keeping timeout, capture_output, text, env=self.env.env, and cwd=str(self.env.webkit_root) unchanged. Ensure the script path is not wrapped in a shell and is passed as the first element of the argv list so paths with spaces are handled correctly.
♻️ Duplicate comments (31)
JSTests/microbenchmarks/regexp-forward-reference.js (1)
4-5: 🧹 Nitpick | 🔵 TrivialUse
constfor immutable regex bindings.On Line 4 and Line 5,
re1/re2are never reassigned, soconstis preferable for intent clarity. This was already flagged in an earlier review on the same lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/microbenchmarks/regexp-forward-reference.js` around lines 4 - 5, re1 and re2 are never reassigned, so change their declarations from let to const to express immutability; update the bindings for the RegExp literals referenced by re1 and re2 (the declarations currently using "let re1 = /\2(a)(b)/;" and "let re2 = /\k<y>(?<x>a)(?<y>b)/;") to use const instead.LayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash.html (1)
11-15:⚠️ Potential issue | 🟡 MinorUse
try/finallyaround internals toggle to prevent test-state leakage.If any operation in Lines 12-14 throws, Line 15 is skipped and immediate renderer destruction stays enabled for later tests.
Suggested fix
window.testRunner?.dumpAsText(); window.internals?.setImmediateRendererDestructionEnabled(true); -document.body.offsetHeight; -document.getElementById('target').prepend('A'); -document.body.offsetHeight; -window.internals?.setImmediateRendererDestructionEnabled(false); +try { + document.body.offsetHeight; + document.getElementById('target').prepend('A'); + document.body.offsetHeight; +} finally { + window.internals?.setImmediateRendererDestructionEnabled(false); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash.html` around lines 11 - 15, Wrap the enable/disable calls to window.internals?.setImmediateRendererDestructionEnabled around the operations in a try/finally so the flag is always reset: call setImmediateRendererDestructionEnabled(true) before performing document.body.offsetHeight and element.prepend (e.g. document.getElementById('target').prepend('A')), perform the operations inside the try, and call setImmediateRendererDestructionEnabled(false) in the finally block to guarantee cleanup even if document.body.offsetHeight or prepend throws.LayoutTests/editing/execCommand/break-out-of-list-resets-start-number.html (1)
2-6:⚠️ Potential issue | 🟡 MinorAdd
langand<title>to avoid HTMLHint failures.Line 2 lacks a
langattribute, and Lines 3-6 lack a<title>element.Suggested fix
-<html> +<html lang="en"> <head> +<title>break-out-of-list-resets-start-number</title> <script src="../editing.js"></script> <script src="../../resources/dump-as-markup.js"></script> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/editing/execCommand/break-out-of-list-resets-start-number.html` around lines 2 - 6, Add a lang attribute to the <html> element and add a <title> element inside the <head> to satisfy HTMLHint; specifically update the opening <html> tag to include an appropriate lang (e.g., lang="en") and insert a concise <title>...</title> node within the <head> section near the existing <script> tags so the document has both language metadata and a title.LayoutTests/editing/execCommand/outdent-list-resets-start-number.html (1)
2-6:⚠️ Potential issue | 🟡 MinorAdd missing document metadata to satisfy HTML lint.
Line 2 is missing
lang, and Lines 3-6 are missing a<title>. This can fail HTMLHint checks.Suggested fix
-<html> +<html lang="en"> <head> +<title>outdent-list-resets-start-number</title> <script src="../editing.js"></script> <script src="../../resources/dump-as-markup.js"></script> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/editing/execCommand/outdent-list-resets-start-number.html` around lines 2 - 6, Add the missing document metadata: add a lang attribute to the <html> element (e.g., lang="en") and include a <title> element inside the <head> (e.g., a short descriptive title for this test page) so the <html> and <head> elements in this file include proper language and title metadata.JSTests/stress/async-function-empty-resolved-promise.js (1)
47-51:⚠️ Potential issue | 🟠 MajorFix per-iteration disposal assertions inside the 1000-iteration stress loop.
Line 48 and Line 51 assert absolute
1, butrun()is called 1000 times (Line 81-Line 82). The counters are cumulative, so this fails from the second iteration onward.Suggested fix
async function run() { @@ - shouldBe(await onlyUsing(), undefined); - shouldBe(disposedSync, 1); + const syncBefore = disposedSync; + shouldBe(await onlyUsing(), undefined); + shouldBe(disposedSync, syncBefore + 1); @@ - shouldBe(await onlyAwaitUsing(), undefined); - shouldBe(disposedAsync, 1); + const asyncBefore = disposedAsync; + shouldBe(await onlyAwaitUsing(), undefined); + shouldBe(disposedAsync, asyncBefore + 1);Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/stress/async-function-empty-resolved-promise.js` around lines 47 - 51, The assertions compare disposedSync and disposedAsync to 1 even though run() is executed 1000 times, so the counters are cumulative and the checks fail after the first iteration; fix by either resetting the counters before each iteration (set disposedSync = 0 and disposedAsync = 0 just before calling run()) or by asserting the cumulative value using the loop index (e.g., expect disposedSync === iterationIndex+1 and disposedAsync === iterationIndex+1) when checking the results of onlyUsing() and onlyAwaitUsing(); update the assertions that reference disposedSync/disposedAsync accordingly and keep references to onlyUsing, onlyAwaitUsing and run so the correct checks are adjusted.LayoutTests/fast/css/expedia-group-animation-quirk.html (2)
1-12:⚠️ Potential issue | 🟡 MinorAdd explicit
<html>root and<title>in<head>.At Line 9-Line 12,
<head>has no<title>, and at Line 88</html>is closed without an opening<html>. This keeps the test document malformed and triggers lint/parser issues.Proposed patch
<!DOCTYPE html> +<html> <!-- Test for the Expedia Group menu animation quirk (https://webkit.org/b/312549, extending https://webkit.org/b/308116 / https://webkit.org/b/312212). @@ <head> +<title>Expedia Group animation quirk</title> <script src="../../resources/testharness.js"></script> <script src="../../resources/testharnessreport.js"></script> </head>Also applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css/expedia-group-animation-quirk.html` around lines 1 - 12, The document is missing an opening <html> root and a <title> inside the <head>, leaving the HTML malformed (file: fast/css/expedia-group-animation-quirk.html); fix by adding an explicit opening <html> tag immediately before the existing <head> and add a meaningful <title> element inside the <head> (e.g. "Expedia Group animation quirk test"), and verify the existing closing </html> matches the new opening tag so the document is well-formed.
68-73:⚠️ Potential issue | 🟡 MinorRequire both expected delay and duration values in assertions.
At Line 69 and Line 72, using
||allows partial matches to pass. The test should require both expected timings to prevent silent regressions.Proposed patch
const delays = style.animationDelay; - assert_true(delays.includes("0s") || delays.includes("0.06s"), + assert_true(delays.includes("0s") && delays.includes("0.06s"), "quirk should set correct animation delays (0s, 0.06s); got: " + delays); const durations = style.animationDuration; - assert_true(durations.includes("0.18s") || durations.includes("0.06s"), + assert_true(durations.includes("0.18s") && durations.includes("0.06s"), "quirk should set correct animation durations (0.18s, 0.06s); got: " + durations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css/expedia-group-animation-quirk.html` around lines 68 - 73, The assertions currently allow partial matches by using ||; update the checks so both expected timing strings must be present: for animation delays verify style.animationDelay includes "0s" AND includes "0.06s", and for animation durations verify style.animationDuration includes "0.18s" AND includes "0.06s" (use assert_true for each combined condition or separate asserts) — reference the variables delays, durations, style and the assert_true calls in the diff to locate where to change the logic.LayoutTests/accessibility/base-select-option-press-closes-popover.html (1)
2-11:⚠️ Potential issue | 🟡 MinorAdd missing document metadata (
langand<title>).Line 2 is missing
lang, and<head>is missing a<title>element. This mirrors the earlier unresolved review note.Suggested patch
-<html> +<html lang="en"> <head> +<title>base-select option press closes popover</title> <script src="../resources/accessibility-helper.js"></script> <script src="../resources/js-test.js"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/base-select-option-press-closes-popover.html` around lines 2 - 11, Add missing document metadata by setting the lang attribute on the <html> element (e.g., <html lang="en">) and adding a <title> element inside the <head> (e.g., <title>…) so the page includes language and title metadata; update the <html> start tag to include lang and insert a <title> element within the existing <head> block.LayoutTests/accessibility/element-path-with-css-transforms.html (4)
3-33:⚠️ Potential issue | 🟠 MajorAdd a
<title>element in<head>.The document head closes without a title, which fails HTML linting.
Proposed fix
<head> +<title>Element path with CSS transforms</title> <script src="../resources/accessibility-helper.js"></script> <script src="../resources/js-test.js"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/element-path-with-css-transforms.html` around lines 3 - 33, The head element in the test HTML is missing a <title>, causing HTML lint failures; add a descriptive <title> element (for example "element-path-with-css-transforms") inside the <head> of the document—place it before or after the existing <script> tags and before the <style> block so the document has a valid title tag.
52-53:⚠️ Potential issue | 🟠 MajorWait for all target elements before assertions.
Line 52 only waits for
item1;item2/item3checks can race path population.Proposed fix
await waitUntilIDHasPathWith("item1", 1, "Curve to"); + await waitUntilIDHasPathWith("item2", 1, "Curve to"); + await waitUntilIDHasPathWith("item3", 1, "Curve to");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/element-path-with-css-transforms.html` around lines 52 - 53, The test currently only awaits path population for "item1" using waitUntilIDHasPathWith which lets checks for "item2" and "item3" race; add explicit waits for those IDs before making assertions by calling waitUntilIDHasPathWith for "item2" and "item3" (matching the same expected path/text used for "item1") so all target elements have their paths populated prior to assertions.
2-2:⚠️ Potential issue | 🟡 MinorAdd a language attribute on the root element.
Line 2 should include
langto satisfy accessibility/lint requirements.Proposed fix
-<html> +<html lang="en">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/element-path-with-css-transforms.html` at line 2, The root HTML element is missing a lang attribute which fails accessibility/lint rules; edit the file's <html> tag (the root element) and add an appropriate language attribute (e.g., lang="en" or the project’s preferred language code), ensuring it is not empty and update any related test expectations if they assert the exact markup.
67-68:⚠️ Potential issue | 🟡 MinorInclude
bounds3in size-equality assertions.Current checks can miss a wrong path size for
item3.Proposed fix
output += expect("Math.abs(bounds1.width - bounds2.width) < 2", "true"); output += expect("Math.abs(bounds1.height - bounds2.height) < 2", "true"); + output += expect("Math.abs(bounds1.width - bounds3.width) < 2", "true"); + output += expect("Math.abs(bounds1.height - bounds3.height) < 2", "true");After this change, update
LayoutTests/accessibility/element-path-with-css-transforms-expected.txtto match the new output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/element-path-with-css-transforms.html` around lines 67 - 68, The current assertions only compare bounds1 and bounds2 sizes and can miss a mismatch for item3; update the two expect calls so they assert that Math.abs(bounds1.width - bounds2.width) < 2 && Math.abs(bounds1.width - bounds3.width) < 2 (and similarly for height) or otherwise include bounds3 in the width and height equality checks for bounds1/bounds2/bounds3; modify the lines referencing expect(...) for width and height to include bounds3, and then regenerate/update LayoutTests/accessibility/element-path-with-css-transforms-expected.txt to match the new output.LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html (1)
2-2:⚠️ Potential issue | 🟡 MinorAdd
langto the root<html>element.Line 2 still misses
lang, sohtml-lang-requireremains unresolved.Suggested fix
-<html> +<html lang="en">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html` at line 2, The root HTML element is missing a lang attribute; update the root <html> tag in the file to include a language attribute (for example set <html lang="en"> or the appropriate language code) so the html-lang-require check passes—modify the <html> element declaration (the root <html> tag) to include the lang="…" attribute.LayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.html (1)
3-3:⚠️ Potential issue | 🟡 MinorInvalid closing tag for void element
<embed>.In HTML5,
<embed>is a void element and must not include a closing</embed>tag. This issue was already flagged in a previous review.🔧 Proposed fix
-<embed id="embed" style="top: 200vmin; visibility: hidden;" type="application/pdf" src="about:blank"></embed> +<embed id="embed" style="top: 200vmin; visibility: hidden;" type="application/pdf" src="about:blank">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.html` at line 3, The HTML uses a non-void closing tag for the void element <embed> (the element with id="embed"); remove the invalid closing </embed> tag and ensure the element is written as a void element (e.g., <embed id="embed" ...> or self-closing if your formatter requires) so the embed element is not closed with a separate end tag.JSTests/stress/promise-prototype-finally-bug-312466.js (1)
42-42: 🧹 Nitpick | 🔵 TrivialPolish failure labels for readability.
The labels still contain awkward possessives (
this',it's), which makes failure output noisier than needed.✏️ Optional wording cleanup
- shouldThrow(`mimic this' constructor does not have [[Construct]]: ${name}`, () => { + shouldThrow(`mimic this.constructor does not have [[Construct]]: ${name}`, () => { @@ - shouldThrow(`mimic this' constructor[Symbol.species] is: ${name}`, () => { + shouldThrow(`mimic this.constructor[Symbol.species] is: ${name}`, () => { @@ - shouldThrow(`|this| is derived Promise instance but it's [Symbol.species] is weird: ${name}`, () => { + shouldThrow(`|this| is derived Promise instance but its [Symbol.species] is weird: ${name}`, () => {Also applies to: 68-68, 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/stress/promise-prototype-finally-bug-312466.js` at line 42, Update the failure message strings passed to shouldThrow to remove awkward possessives like "this'" and "it's" for clarity; locate the three calls to shouldThrow (e.g., the one with the message currently starting "mimic this' constructor does not have [[Construct]]: ${name}") and replace phrases like "mimic this' constructor" with "mimic this constructor" (or "the mimic constructor") and change any "it's" to "its" so all three labels (lines around where shouldThrow is called) read cleanly and consistently.LayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband.html (2)
2-4:⚠️ Potential issue | 🟡 MinorAdd missing
<html lang>and<title>metadata.This file still omits
langon<html>and<title>in<head>.Proposed fix
-<html> +<html lang="en"> <head> <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no"> +<title>scroll-to-smooth-during-rubberband</title> <script src="../../../resources/js-test.js"></script>Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband.html` around lines 2 - 4, The HTML file is missing language and title metadata; update the <html> element to include a lang attribute (e.g., <html lang="en">) and add a <title> element inside the <head> (e.g., <title>Scroll to Smooth During Rubberband</title>) so the document has proper language declaration and a descriptive title; modify the existing <html> tag and the <head> block accordingly (look for the <html> and <head> tags in this file and add the lang attribute and a new <title> element).
32-47:⚠️ Potential issue | 🟠 MajorAssert the rubberband negative-offset path was actually hit.
triggeredScrollToZerois never asserted, so the test can pass without exercising thewindow.pageYOffset < 0branch.Proposed fix
await UIHelper.dragFromPointToPoint(xOffset, 1120, xOffset, 1600, 0.05); await UIHelper.waitForTargetScrollAnimationToSettle(document.scrollingElement); + shouldBeTrue("triggeredScrollToZero"); shouldBe("window.pageYOffset", "0");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/scrolling/ios/scroll-to-smooth-during-rubberband.html` around lines 32 - 47, The test never verifies that the rubberband negative-offset branch ran because triggeredScrollToZero is never asserted; after awaiting UIHelper.waitForTargetScrollAnimationToSettle(document.scrollingElement) add an assertion that triggeredScrollToZero is true (e.g. using the existing test assertion helpers like shouldBe/shouldBeTrue against the variable name) so the test fails if the window.pageYOffset < 0 path was not hit, then keep the existing shouldBe("window.pageYOffset", "0") check.LayoutTests/fast/events/anchor-mouse-focusable-quirk.html (1)
5-8:⚠️ Potential issue | 🟡 MinorAdd missing document metadata (
langand<title>).As noted in the previous review, this file is missing
langattribute on the<html>element and a<title>element in<head>, which are flagged by static analysis.📋 Suggested fix
-<html> +<html lang="en"> <head> +<title>Anchor mouse-focusable quirk</title> <script src="../../resources/js-test.js"></script> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/events/anchor-mouse-focusable-quirk.html` around lines 5 - 8, Add the missing document metadata by setting the lang attribute on the HTML element and adding a title element inside the head: update the <html> tag to include a proper lang value (e.g., lang="en") and add a <title> element within the <head> (e.g., <title>anchor-mouse-focusable-quirk</title>) so that the document has both language metadata and a page title; modify the existing <html> and <head> elements in this file accordingly.LayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf.html (1)
70-74:⚠️ Potential issue | 🟡 MinorAdd a mutation assertion to actually verify liveness.
Line 71–Line 74 validate lookup only; they do not prove
childNodesupdates after DOM mutation.♻️ Proposed fix
test(function() { const nodeList = container.childNodes; - assert_equals(indexOf.call(nodeList, container.firstChild), 0); + const initialLength = nodeList.length; + const appended = document.createElement("div"); + container.appendChild(appended); + + assert_equals(nodeList.length, initialLength + 1, "live NodeList reflects append"); + assert_equals(indexOf.call(nodeList, appended), initialLength, "new node is discoverable"); + + container.removeChild(appended); assert_equals(indexOf.call(nodeList, outside), -1); }, "indexOf works on live ChildNodeList");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf.html` around lines 70 - 74, The test currently only checks indexOf lookups and doesn't assert that container.childNodes is live; after obtaining nodeList via container.childNodes and the initial asserts (indexOf.call(nodeList, container.firstChild) and indexOf.call(nodeList, outside)), mutate the DOM (e.g., append or remove a child from container using container.appendChild or container.removeChild on a known node) and then assert that indexOf.call(nodeList, that moved/added/removed node) reflects the mutation (index changed or becomes -1) to prove liveness of childNodes; reference container.childNodes, indexOf.call(nodeList, ...), container.firstChild and the node you mutate.LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html (1)
7-10:⚠️ Potential issue | 🔴 CriticalFix
AudioDecoderfeature detection precedence bug.
if (!"AudioDecoder" in window)is parsed incorrectly and won’t reliably detect absence ofAudioDecoder. This can execute unsupported-path code.Suggested fix
- if (!"AudioDecoder" in window) { + if (!("AudioDecoder" in window)) { window.testRunner?.notifyDone(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html` around lines 7 - 10, The feature-detection expression using "AudioDecoder" is parsed incorrectly; update the check around AudioDecoder so it properly tests existence (e.g., wrap the in-expression in parentheses or use typeof) and leave the early-exit logic intact: change the condition guarding window.testRunner?.notifyDone() and return so it reliably detects absence of the AudioDecoder global (referencing the "AudioDecoder" symbol and the existing window.testRunner?.notifyDone() call).JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js (3)
34-35:⚠️ Potential issue | 🟡 MinorPast review comment still applies: getReport() return value should be validated.
The previous review correctly identified that ignoring the agent report on line 35 can allow false positives if the worker fails or reports an unexpected state. The suggested fix of asserting the report equals
"done"remains valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js` around lines 34 - 35, The $.agent.getReport() return value is ignored which can hide worker failures; update the test to capture and validate the report from $.agent.getReport() (e.g., assert it equals "done") after $.agent.sleep(50) so the test fails when the worker reports anything other than the expected "done" value; reference the $.agent.sleep and $.agent.getReport calls in the change.
26-31:⚠️ Potential issue | 🟡 MinorPast review comment still applies: empty catch block swallows unexpected exceptions.
The previous review correctly identified that the empty
catch (e) {}on line 31 can hide real regressions. The suggested fix of catching only expected exception types (RangeError/TypeError) and rethrowing others remains valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js` around lines 26 - 31, The empty catch in the loop around ta.with / ta.toReversed / ta.toSorted swallows unexpected exceptions; change the catch to only handle the expected exception types (e.g., RangeError and TypeError) and rethrow any other errors so real regressions surface — inspect the try/catch surrounding calls to ta.with, ta.toReversed, and ta.toSorted and update it to if the caught error is a RangeError or TypeError, suppress it (or handle accordingly), otherwise rethrow the exception.
12-18:⚠️ Potential issue | 🟡 MinorPast review comment still applies: empty catch block swallows unexpected exceptions.
The previous review correctly identified that the empty
catch (e) {}on line 17 can hide real regressions. The suggested fix of catching only expected exception types (RangeError/TypeError) and rethrowing others remains valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js` around lines 12 - 18, The empty catch in the sab.grow loop swallows unexpected exceptions; update the try/catch around the loop that calls sab.grow (the for loop invoking sab.grow(sab.byteLength + 64)) to only swallow expected exceptions (e.g., RangeError and TypeError) and rethrow any other error types—inspect the caught error (e) and if it is not an instance of RangeError or TypeError, throw it again so unexpected failures are not hidden.JSTests/wasm/stress/fmin-fmax-constant-fold.js (1)
65-65: 🧹 Nitpick | 🔵 TrivialHoist computed expected float constants out of the hot loop (already noted previously).
The loop still recalculates denormal/rounded expected values every iteration; precomputing once keeps stress focused on Wasm execution rather than JS arithmetic overhead.
♻️ Proposed refactor
let instance = await instantiate(wat); +const F32_DENORM = Math.fround(Math.pow(2, -149)); +const F64_DENORM = Math.pow(2, -1074); +const F32_MIN_NORMAL = Math.fround(2.72); +const F32_MAX_NORMAL = Math.fround(3.14); for (let i = 0; i < wasmTestLoopCount; ++i) { @@ - assert.eq(instance.exports.f32_min_denorm(), -Math.fround(Math.pow(2, -149))); - assert.eq(instance.exports.f32_max_denorm(), Math.fround(Math.pow(2, -149))); - assert.eq(instance.exports.f64_min_denorm(), -Math.pow(2, -1074)); - assert.eq(instance.exports.f64_max_denorm(), Math.pow(2, -1074)); + assert.eq(instance.exports.f32_min_denorm(), -F32_DENORM); + assert.eq(instance.exports.f32_max_denorm(), F32_DENORM); + assert.eq(instance.exports.f64_min_denorm(), -F64_DENORM); + assert.eq(instance.exports.f64_max_denorm(), F64_DENORM); @@ - assert.eq(instance.exports.f32_min_normal(), Math.fround(2.72)); - assert.eq(instance.exports.f32_max_normal(), Math.fround(3.14)); + assert.eq(instance.exports.f32_min_normal(), F32_MIN_NORMAL); + assert.eq(instance.exports.f32_max_normal(), F32_MAX_NORMAL);Also applies to: 93-96, 113-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/wasm/stress/fmin-fmax-constant-fold.js` at line 65, The loop repeatedly recomputes the expected denormal/rounded float constants inside the hot loop that iterates with wasmTestLoopCount; compute those expected values once before entering the loop and reuse the precomputed variables inside the loop (do the same refactor for the other two loops that compute expected floats). Locate the hot loop using the wasmTestLoopCount loop header and move any calculations that produce expected float constants (denormal/rounded expected values) out to a small pre-loop setup block, then replace the in-loop expressions with references to those precomputed variables.LayoutTests/fast/forms/HTMLOptionElement_label02.html (1)
5-5:⚠️ Potential issue | 🟠 Major
label=""contradicts the asserted visible text.At Line 5, an explicit empty
labelpaired with visible text conflicts with the intent stated on Line 2 and can hide behavior regressions. Keep the option text empty (or makelabelnon-empty if intent changed), then rebaseline expected output.Proposed fix (empty-label behavior)
- <option label="">should display this</option> + <option label=""></option>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/forms/HTMLOptionElement_label02.html` at line 5, The option element in the test uses an explicit empty label attribute but includes visible text ("should display this"), which contradicts the test intent; update the <option label=""> element so its text node is empty (e.g., <option label=""></option>) or, if the intent changed, set a non-empty label value to match the visible text, and then rebaseline the expected output to reflect the chosen behavior; locate the offending element by looking for the <option label=""> entry in the test file and make the text vs. label consistent.LayoutTests/accessibility/left-right-line-range-display-table-in-flex.html (1)
2-17:⚠️ Potential issue | 🟡 MinorAdd missing document metadata (
langand<title>).Line 2 lacks
lang, and<head>lacks<title>, which is already being flagged by static analysis.Proposed fix
-<html> +<html lang="en"> <head> +<title>left-right-line-range-display-table-in-flex</title> <script src="../resources/accessibility-helper.js"></script> <script src="../resources/js-test.js"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/left-right-line-range-display-table-in-flex.html` around lines 2 - 17, The HTML is missing document metadata: add a lang attribute to the <html> element (e.g., lang="en") and add a <title> element inside the <head> (before or after the existing scripts/styles) so the document has proper language and title metadata; update the <html> tag and insert a <title> into the <head> containing an appropriate string.LayoutTests/fast/reflections/reflection-removed-crash-2.html (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
<!DOCTYPE html>to keep the regression test in standards mode.Line 1 currently starts with
<style>, which can trigger quirks-mode parsing and reduce test determinism.Suggested patch
+<!DOCTYPE html> <style> rt { translate: 23% 80cap 1svi; -webkit-box-reflect: below; } </style>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/reflections/reflection-removed-crash-2.html` at line 1, The test file starts with a <style> tag which triggers quirks-mode; prepend the HTML5 doctype declaration so the page runs in standards mode. Edit reflection-removed-crash-2.html and insert <!DOCTYPE html> as the very first line before the existing <style> tag (ensuring no BOM/whitespace precedes it) so the test is deterministic under standards-mode parsing.LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html (2)
18-26:⚠️ Potential issue | 🟠 MajorClose the remaining opened container
<div>tags before</body>.
#testArena,.comment-thread, and.commentare opened but not explicitly closed.Proposed fix
<div class="comment-text"> ...This test passes if it doesn't crash. </div> </div> + </div></div></div> </body>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html` around lines 18 - 26, The HTML leaves three opened containers unclosed: the `#testArena` div and its child .comment-thread and .comment; close them in proper nesting order before the closing </body> tag (i.e., add closing </div> for .comment, then .comment-thread, then `#testArena`) so the DOM is balanced and well-formed and any future additions remain correctly nested.
3-12:⚠️ Potential issue | 🟡 MinorAdd a
<title>inside<head>.
<head>is currently missing a required<title>, which keeps failing HTML lint.Proposed fix
<head> <meta charset="UTF-8"> + <title>text-wrap pretty line-break crash 6</title> <style>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html` around lines 3 - 12, The HTML head is missing a required <title> element; add a <title> tag inside the existing <head> (e.g., before or after the <style> block) with an appropriate short page title so the document passes HTML lint; locate the <head> block that contains the <meta charset="UTF-8"> and <style> and insert the <title> element there.LayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic.html (1)
2-7:⚠️ Potential issue | 🟡 MinorAdd
langand<title>in document head.This still misses required metadata and keeps triggering lint findings.
Proposed fix
-<html> +<html lang="en"> <head> <script src="../../../resources/js-test.js"></script> <script src="../../../resources/accessibility-helper.js"></script> <meta http-equiv="content-language" content="en"> +<title>ARIA relevant text live-region non-atomic test</title> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic.html` around lines 2 - 7, The document head is missing required metadata: add a lang attribute to the <html> element (e.g., <html lang="en">) and include a <title> element inside the <head> with an appropriate short title to satisfy accessibility/lint rules; update the file live-region-relevant-text-non-atomic.html by modifying the <html> tag to include lang and adding a <title> element inside the existing <head> block alongside the existing scripts and meta.LayoutTests/TestExpectations (1)
1702-1703:⚠️ Potential issue | 🔴 CriticalMissing expectation status — test runner will fail to parse these entries.
Lines 1702-1703 are missing their expectation status. The TestExpectations format requires each entry to end with an expectation like
[ Pass ],[ Skip ], or[ ImageOnlyFailure ].Either:
- Delete these lines if the tests should no longer have explicit expectations
- Add the appropriate expectation if they still need tracking
-imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm -imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html +imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm [ ImageOnlyFailure ] +imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html [ ImageOnlyFailure ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/TestExpectations` around lines 1702 - 1703, The two entries imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm and imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html in TestExpectations are missing expectation statuses and will break the parser; fix by either removing those lines if they should not have explicit expectations, or append the correct expectation token (e.g., [ Pass ], [ Skip ], [ ImageOnlyFailure ], etc.) to each entry so they conform to the TestExpectations format.
| if (SWIFT_REQUIRED) | ||
| cmake_policy(SET CMP0157 NEW) | ||
| enable_language(Swift) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In official CMake docs, which CMake version introduced policy CMP0157, and what is the behavior if cmake_policy(SET CMP0157 NEW) is used on versions where the policy is unknown?
💡 Result:
CMake policy CMP0157 was introduced in CMake version 3.29. If cmake_policy(SET CMP0157 NEW) is used on versions where the policy is unknown (earlier than 3.29), CMake issues a warning and ignores the command, treating the policy as unset. For CMP0157 specifically, when unset, CMake uses the OLD behavior without warning.
Citations:
- 1: https://cmake.org/cmake/help/latest/policy/CMP0157.html
- 2: https://cmake.org/cmake/help/latest/command/cmake_policy.html
Guard CMP0157 or raise minimum CMake version to 3.29.
Line 24 sets CMP0157 unconditionally while Line 9 allows CMake 3.20–3.28. CMake versions before 3.29 will silently ignore this policy and use the OLD behavior instead, causing unintended inconsistency across versions. Either guard the policy check or update the minimum required CMake version.
Proposed fix
if (SWIFT_REQUIRED)
- cmake_policy(SET CMP0157 NEW)
+ if (POLICY CMP0157)
+ cmake_policy(SET CMP0157 NEW)
+ endif ()
enable_language(Swift)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (SWIFT_REQUIRED) | |
| cmake_policy(SET CMP0157 NEW) | |
| enable_language(Swift) | |
| if (SWIFT_REQUIRED) | |
| if (POLICY CMP0157) | |
| cmake_policy(SET CMP0157 NEW) | |
| endif () | |
| enable_language(Swift) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 23 - 25, The CMP0157 policy is being set
unguarded inside the SWIFT_REQUIRED block (cmake_policy(SET CMP0157 NEW)) while
the project allows CMake 3.20–3.28; either raise the minimum CMake to 3.29 by
updating cmake_minimum_required to 3.29 so CMP0157 is supported, or guard the
policy call by checking POLICY CMP0157 before setting it and fall back to OLD
behavior for older CMake versions; update the SWIFT_REQUIRED branch that calls
cmake_policy(SET CMP0157 NEW) and/or the top-level cmake_minimum_required to
implement one of these fixes.
| if (!isNaN(result)) | ||
| throw new Error("expected NaN, got " + result); | ||
| // NaN is a value, not a hole, so 1 should be "in" arr. | ||
| // (This array escaped the sinking phase due to the NaN write, so it is a | ||
| // real runtime array and the in-operator checks its actual storage.) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, what are the exact results of isNaN(undefined)andNumber.isNaN(undefined)?
💡 Result:
isNaN(undefined) returns true and Number.isNaN(undefined) returns false. The global isNaN function coerces its argument to a number first. Since Number(undefined) is NaN, it returns true. Number.isNaN does not coerce; it only returns true if the value is of type Number and exactly NaN. undefined is not a Number, so it returns false. This behavior is unchanged in ECMAScript 2026/2027 and consistent across all JavaScript environments.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isnan
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isNaN
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isNaN
Use Number.isNaN instead of coercive isNaN and assert the in operator check.
Line 23 uses isNaN, which coerces its argument to a number. If result is undefined, isNaN(undefined) returns true, causing the assertion to pass incorrectly. Additionally, the comment on lines 25–27 explains that index 1 must be present in the array, but this is never verified.
Proposed fix
let result;
+let hasIndex1;
let breakAt = testLoopCount - 1;
if (breakAt % 2 !== 1) breakAt--;
for (let i = 0; i < testLoopCount; i++) {
@@
if (i === breakAt) {
result = arr[1];
+ hasIndex1 = 1 in arr;
break;
}
}
// breakAt is odd, so arr[1] = makeNaN() was executed. The value should be NaN.
-if (!isNaN(result))
+if (!Number.isNaN(result))
throw new Error("expected NaN, got " + result);
// NaN is a value, not a hole, so 1 should be "in" arr.
-// (This array escaped the sinking phase due to the NaN write, so it is a
-// real runtime array and the in-operator checks its actual storage.)
+if (!hasIndex1)
+ throw new Error("expected index 1 to be present");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/array-allocation-sink-double-nan-not-hole.js` around lines 23
- 27, Replace the coercive isNaN check with Number.isNaN for the variable result
(i.e., use Number.isNaN(result) instead of isNaN(result)) and add an explicit
in-operator assertion to verify index 1 exists on the test array (e.g., assert
or throw if (!(1 in arr))) so that you both correctly detect NaN and confirm the
comment's claim that index 1 is present; update the error messages for both
checks to include the variable names (result and arr) for clarity.
| function shouldBe(actual, expected) { | ||
| if (actual !== expected) | ||
| throw new Error(`Expected ${expected} but got ${actual}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve shouldBe diagnostics with an optional case label.
With many large-number assertions, labeling failures will speed up CI triage.
Proposed refactor
-function shouldBe(actual, expected) {
+function shouldBe(actual, expected, label = "") {
if (actual !== expected)
- throw new Error(`Expected ${expected} but got ${actual}`);
+ throw new Error(`${label ? `[${label}] ` : ""}Expected ${expected} but got ${actual}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/bigint-cached-mod-edge-cases.js` around lines 1 - 4, The
shouldBe function lacks contextual labels for failures; modify shouldBe(actual,
expected, label) to accept an optional third parameter (e.g., label = undefined)
and include that label in the thrown Error message (e.g., prepend or append
"[label]" when provided) so assertion failures for large-number comparisons in
shouldBe(...) report the case name; keep the existing strict comparison (actual
!== expected) and only change the error construction in shouldBe to include the
label when present.
|
|
||
| $.agent.start(` | ||
| $.agent.receiveBroadcast((sab, idx) => { | ||
| for (let i = 0; i < 50000; i++) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a named constant for agent iteration count.
The agent loop uses a hardcoded 50000 iterations while the main thread uses ITERATIONS_PER_ROUND (5000). Using a named constant would make the 10:1 ratio more explicit and maintainable.
♻️ Proposed refinement
At the top of the file:
const ROUNDS = 10;
const ITERATIONS_PER_ROUND = 5000;
+const AGENT_ITERATIONS = 50000;Then in the agent code:
- for (let i = 0; i < 50000; i++) {
+ for (let i = 0; i < ${AGENT_ITERATIONS}; i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js`
at line 12, The agent loop uses a hardcoded 50000 while the main thread
references ITERATIONS_PER_ROUND (5000); introduce a named constant (e.g.,
AGENT_ITERATIONS) at the top of the file and set it relative to
ITERATIONS_PER_ROUND (for clarity use AGENT_ITERATIONS = ITERATIONS_PER_ROUND *
10 or AGENT_ITERATIONS = 50000), then replace the literal 50000 in the agent for
loop with AGENT_ITERATIONS so the 10:1 ratio is explicit and maintainable;
update any comments/uses to reference AGENT_ITERATIONS and ensure
ITERATIONS_PER_ROUND remains the single source of truth for per-round
iterations.
| function stringify(m) { | ||
| return m === null ? "null" : JSON.stringify(Array.from(m)); | ||
| } | ||
|
|
||
| function test(re, input, expected) { | ||
| const actual = re.exec(input); | ||
| const actualStr = stringify(actual); | ||
| const expectedStr = stringify(expected); | ||
| if (actualStr !== expectedStr) | ||
| throw new Error(`FAIL: ${re} on ${JSON.stringify(input)}: expected ${expectedStr}, got ${actualStr}`); |
There was a problem hiding this comment.
Preserve undefined vs null in assertion output/comparison.
Line 21 collapses unmatched captures by serializing with JSON.stringify(Array.from(...)) (undefined becomes null). That can hide capture regressions in this stress suite.
Proposed fix
-function stringify(m) {
- return m === null ? "null" : JSON.stringify(Array.from(m));
-}
+function formatMatch(m) {
+ if (m === null)
+ return "null";
+ return `[${Array.from(m, (v) => v === undefined ? "<undefined>" : JSON.stringify(v)).join(", ")}]`;
+}
function test(re, input, expected) {
const actual = re.exec(input);
- const actualStr = stringify(actual);
- const expectedStr = stringify(expected);
- if (actualStr !== expectedStr)
- throw new Error(`FAIL: ${re} on ${JSON.stringify(input)}: expected ${expectedStr}, got ${actualStr}`);
+ if (actual === null || expected === null) {
+ if (actual !== expected)
+ throw new Error(`FAIL: ${re} on ${JSON.stringify(input)}: expected ${formatMatch(expected)}, got ${formatMatch(actual)}`);
+ return;
+ }
+
+ if (actual.length !== expected.length)
+ throw new Error(`FAIL: ${re} on ${JSON.stringify(input)}: expected length ${expected.length}, got ${actual.length}`);
+
+ for (let i = 0; i < expected.length; ++i) {
+ if (actual[i] !== expected[i]) {
+ throw new Error(`FAIL: ${re} on ${JSON.stringify(input)} at index ${i}: expected ${formatMatch(expected)}, got ${formatMatch(actual)}`);
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/yarr-jit-fixedcount-paren-context-free-on-skip.js` around
lines 20 - 29, The stringify helper and test compare use
JSON.stringify(Array.from(m)) which converts unmatched capture undefined to null
and hides regressions; update stringify (used by test and stringify) to preserve
undefined vs null by converting the match array into a form that distinguishes
undefined (e.g., map each element to a sentinel or an explicit string like
"<undefined>" or use JSON.stringify with a replacer) before serializing,
ensuring unmatched captures remain distinct when comparing actualStr and
expectedStr for the re.exec results.
| /* Subject :has() with descendant combinator and compound peer */ | ||
| .c2:has(.trigger) > .target { color: green; } | ||
| /* Subject :has() without compound peer */ | ||
| :has(> .trigger) > .target { color: green; } |
There was a problem hiding this comment.
Test isolation is broken; shared selector masks negative assertions.
The global rule at Line 10 (:has(> .trigger) > .target) can make unrelated cases pass, including scenarios that should turn non-green. This weakens the test signal (notably c8/c10). Scope that rule to its own case and fix expectations for negative outcomes.
Proposed fix
-:has(> .trigger) > .target { color: green; }
+.c3:has(> .trigger) > .target { color: green; }
-runTest(":has(> .trigger) without compound peer", null, function(container) {
+runTest(":has(> .trigger) without compound peer", "c3", function(container) {
var trigger = document.createElement("div");
trigger.className = "trigger";
container.appendChild(trigger);
}, true);
runTest(":not(:has(.trigger)) inside :not() with compound peer .c8", "c8", function(container) {
var trigger = document.createElement("div");
trigger.className = "trigger";
container.appendChild(trigger);
-}, true);
+}, false);
runTest(":is(.other :has(.trigger)) inside :is() with complex selector and compound peer .c10", "c10", function(container) {
var trigger = document.createElement("div");
trigger.className = "trigger";
container.appendChild(trigger);
-}, true);
+}, false);Also applies to: 93-97, 123-127, 135-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/selectors/has-invalidation-traversal-size.html` at line 10,
The global CSS rule ":has(> .trigger) > .target" is leaking into other test
cases and masking negative assertions; scope that rule to only its intended test
case by prefixing it with the test-case-specific container selector (e.g., add a
case-specific id or class like ".case-has-invalidation" so it becomes
".case-has-invalidation :has(> .trigger) > .target"), and then update the
expectations for the tests that assert negatives (the cases corresponding to the
other occurrences of the same rule) so they no longer rely on the global
rule—apply the same scoping fix for the other instances noted (the selectors
around the ranges analogous to 93-97, 123-127, 135-139).
| <html> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| <meta charset="utf-8"> | ||
| <head> | ||
| <style> | ||
| .text-representation { | ||
| white-space: pre-wrap; | ||
| } | ||
|
|
||
| .visually-hidden-input { | ||
| opacity: 0; | ||
| width: 0; | ||
| height: 0; | ||
| position: absolute; | ||
| } | ||
| </style> | ||
| <script src="../../resources/ui-helper.js"></script> | ||
| </head> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether repository lint configs enforce html-lang/title rules.
fd -HI ".htmlhintrc|htmlhint|lint" .
rg -n --hidden -S "html-lang-require|title-require|HTMLHint|htmlhint"Repository: oven-sh/WebKit
Length of output: 50371
🏁 Script executed:
cat -n LayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.html | head -30Repository: oven-sh/WebKit
Length of output: 1058
Add lang attribute to <html> and insert <title> in <head> to satisfy HTMLHint lint gates.
Line 3 lacks the lang attribute, and <head> (lines 6–20) has no <title> tag. The repository's .htmlhintrc enforces both html-lang-require and title-require rules, which will fail CI without these elements.
Suggested patch
-<html>
+<html lang="en">
@@
<head>
+<title>Hidden radio/checkbox text extraction</title>
<style>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <html> | |
| <meta name="viewport" content="width=device-width, initial-scale=1"> | |
| <meta charset="utf-8"> | |
| <head> | |
| <style> | |
| .text-representation { | |
| white-space: pre-wrap; | |
| } | |
| .visually-hidden-input { | |
| opacity: 0; | |
| width: 0; | |
| height: 0; | |
| position: absolute; | |
| } | |
| </style> | |
| <script src="../../resources/ui-helper.js"></script> | |
| </head> | |
| <html lang="en"> | |
| <meta name="viewport" content="width=device-width, initial-scale=1"> | |
| <meta charset="utf-8"> | |
| <head> | |
| <title>Hidden radio/checkbox text extraction</title> | |
| <style> | |
| .text-representation { | |
| white-space: pre-wrap; | |
| } | |
| .visually-hidden-input { | |
| opacity: 0; | |
| width: 0; | |
| height: 0; | |
| position: absolute; | |
| } | |
| </style> | |
| <script src="../../resources/ui-helper.js"></script> | |
| </head> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 3-3: An lang attribute must be present on elements.
(html-lang-require)
[error] 20-20: <title> must be present in tag.
(title-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@LayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.html`
around lines 3 - 20, Add a lang attribute to the root <html> tag (e.g.,
lang="en") and add a <title> element inside the <head> (e.g., <title>Debug Text
Extraction - Hidden Radio/Checkbox</title>) to satisfy html-lang-require and
title-require; update the existing <html> start tag to include the lang
attribute and insert a <title> as the first child of the <head> element near the
existing <meta> tags so the document passes HTMLHint checks.
| testRunner.dumpAsText(); | ||
| testRunner.waitUntilDone(); | ||
|
|
||
| const results = []; | ||
| for (let outputFormat of ["texttree", "html"]) { | ||
| const heading = document.createElement("h1"); | ||
| heading.textContent = `-- ${outputFormat}`; | ||
|
|
||
| const container = document.createElement("pre"); | ||
| container.classList.add("text-representation"); | ||
| let textContent = await UIHelper.requestDebugText({ | ||
| normalize: true, | ||
| includeRects: false, | ||
| includeURLs: false, | ||
| skipNearlyTransparentContent: true, | ||
| outputFormat, | ||
| }); | ||
|
|
||
| container.textContent = textContent; | ||
| results.push(heading); | ||
| results.push(container); | ||
| results.push(document.createElement("br")); | ||
| } | ||
|
|
||
| document.body.replaceChildren(...results); | ||
| testRunner.notifyDone(); | ||
| }); |
There was a problem hiding this comment.
Guard notifyDone() with finally to prevent hung test runs on async failure.
If UIHelper.requestDebugText(...) rejects, testRunner.notifyDone() is never reached and the test can time out.
Suggested patch
testRunner.dumpAsText();
testRunner.waitUntilDone();
- const results = [];
- for (let outputFormat of ["texttree", "html"]) {
- const heading = document.createElement("h1");
- heading.textContent = `-- ${outputFormat}`;
-
- const container = document.createElement("pre");
- container.classList.add("text-representation");
- let textContent = await UIHelper.requestDebugText({
- normalize: true,
- includeRects: false,
- includeURLs: false,
- skipNearlyTransparentContent: true,
- outputFormat,
- });
-
- container.textContent = textContent;
- results.push(heading);
- results.push(container);
- results.push(document.createElement("br"));
- }
-
- document.body.replaceChildren(...results);
- testRunner.notifyDone();
+ try {
+ const results = [];
+ for (let outputFormat of ["texttree", "html"]) {
+ const heading = document.createElement("h1");
+ heading.textContent = `-- ${outputFormat}`;
+
+ const container = document.createElement("pre");
+ container.classList.add("text-representation");
+ let textContent = await UIHelper.requestDebugText({
+ normalize: true,
+ includeRects: false,
+ includeURLs: false,
+ skipNearlyTransparentContent: true,
+ outputFormat,
+ });
+
+ container.textContent = textContent;
+ results.push(heading);
+ results.push(container);
+ results.push(document.createElement("br"));
+ }
+
+ document.body.replaceChildren(...results);
+ } finally {
+ testRunner.notifyDone();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testRunner.dumpAsText(); | |
| testRunner.waitUntilDone(); | |
| const results = []; | |
| for (let outputFormat of ["texttree", "html"]) { | |
| const heading = document.createElement("h1"); | |
| heading.textContent = `-- ${outputFormat}`; | |
| const container = document.createElement("pre"); | |
| container.classList.add("text-representation"); | |
| let textContent = await UIHelper.requestDebugText({ | |
| normalize: true, | |
| includeRects: false, | |
| includeURLs: false, | |
| skipNearlyTransparentContent: true, | |
| outputFormat, | |
| }); | |
| container.textContent = textContent; | |
| results.push(heading); | |
| results.push(container); | |
| results.push(document.createElement("br")); | |
| } | |
| document.body.replaceChildren(...results); | |
| testRunner.notifyDone(); | |
| }); | |
| testRunner.dumpAsText(); | |
| testRunner.waitUntilDone(); | |
| try { | |
| const results = []; | |
| for (let outputFormat of ["texttree", "html"]) { | |
| const heading = document.createElement("h1"); | |
| heading.textContent = `-- ${outputFormat}`; | |
| const container = document.createElement("pre"); | |
| container.classList.add("text-representation"); | |
| let textContent = await UIHelper.requestDebugText({ | |
| normalize: true, | |
| includeRects: false, | |
| includeURLs: false, | |
| skipNearlyTransparentContent: true, | |
| outputFormat, | |
| }); | |
| container.textContent = textContent; | |
| results.push(heading); | |
| results.push(container); | |
| results.push(document.createElement("br")); | |
| } | |
| document.body.replaceChildren(...results); | |
| } finally { | |
| testRunner.notifyDone(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@LayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.html`
around lines 48 - 74, The test can hang if UIHelper.requestDebugText(...)
rejects because testRunner.notifyDone() is only reached on success; wrap the
async work in a try/finally so notifyDone() is always called: put the loop that
calls UIHelper.requestDebugText (and the document.body.replaceChildren(...)
call) inside a try block and call testRunner.notifyDone() in the finally block
(keeping testRunner.dumpAsText() and testRunner.waitUntilDone() before the try),
referencing the existing symbols UIHelper.requestDebugText and
testRunner.notifyDone to locate where to add the try/finally.
| decoder.decode(chunk); | ||
| await decoder.flush(); | ||
| window.testRunner?.notifyDone(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && find . -name "resetting-audio-decoder-with-zero-size-crash.html" -type fRepository: oven-sh/WebKit
Length of output: 118
🏁 Script executed:
cd /tmp/repo && cat -n LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html | head -40Repository: oven-sh/WebKit
Length of output: 118
🏁 Script executed:
cd /tmp/repo && wc -l LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.htmlRepository: oven-sh/WebKit
Length of output: 118
🏁 Script executed:
git ls-files | grep resetting-audio-decoder-with-zero-size-crash.htmlRepository: oven-sh/WebKit
Length of output: 135
🏁 Script executed:
cat -n LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.htmlRepository: oven-sh/WebKit
Length of output: 874
🏁 Script executed:
git ls-files LayoutTests/fast/webcodecs/ | head -20Repository: oven-sh/WebKit
Length of output: 1054
🏁 Script executed:
cat LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash-expected.txtRepository: oven-sh/WebKit
Length of output: 76
🏁 Script executed:
cat LayoutTests/fast/webcodecs/resetting-audio-decoder-with-unsupported-codec-crash.htmlRepository: oven-sh/WebKit
Length of output: 933
🏁 Script executed:
cat LayoutTests/fast/webcodecs/resetting-video-decoder-with-unsupported-codec-crash.htmlRepository: oven-sh/WebKit
Length of output: 892
🏁 Script executed:
rg "testRunner\?\.notifyDone" LayoutTests/fast/webcodecs/ -B 5 -A 0 | head -60Repository: oven-sh/WebKit
Length of output: 4187
🏁 Script executed:
rg "await.*flush\(" LayoutTests/fast/webcodecs/ -B 3 -A 3Repository: oven-sh/WebKit
Length of output: 730
🏁 Script executed:
grep -r "try\|catch\|finally" LayoutTests/fast/webcodecs/*.html | head -20Repository: oven-sh/WebKit
Length of output: 197
🏁 Script executed:
cat LayoutTests/fast/webcodecs/audio-data-copy-to-crash.htmlRepository: oven-sh/WebKit
Length of output: 1733
🏁 Script executed:
rg "error:\s*_\s*=>" LayoutTests/fast/webcodecs/ -A 5Repository: oven-sh/WebKit
Length of output: 3618
Wrap decode/flush in try/finally to ensure notifyDone() runs.
If flush() rejects unexpectedly, the test will hang waiting for testRunner.waitUntilDone() to complete. Although the error callback handles decoding failures, promise rejections from invalid decoder state or other issues won't be caught, leaving the test without a termination signal.
Suggested fix
- decoder.decode(chunk);
- await decoder.flush();
- window.testRunner?.notifyDone();
+ try {
+ decoder.decode(chunk);
+ await decoder.flush();
+ } catch (_) {
+ // Crash/regression test: failure is acceptable, timeout is not.
+ } finally {
+ window.testRunner?.notifyDone();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| decoder.decode(chunk); | |
| await decoder.flush(); | |
| window.testRunner?.notifyDone(); | |
| }); | |
| try { | |
| decoder.decode(chunk); | |
| await decoder.flush(); | |
| } catch (_) { | |
| // Crash/regression test: failure is acceptable, timeout is not. | |
| } finally { | |
| window.testRunner?.notifyDone(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html`
around lines 20 - 23, Wrap the asynchronous calls to decoder.decode and
decoder.flush inside a try/finally so window.testRunner?.notifyDone() is
guaranteed to run even if decode or flush rejects; locate the block that calls
decoder.decode(chunk); await decoder.flush(); and change the control flow to
call decoder.decode and await decoder.flush inside a try, with
window.testRunner?.notifyDone() moved to the finally clause to ensure
termination regardless of rejections.
| const device = await adapter.requestDevice({ | ||
| requiredFeatures: ["shader-f16"], | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the silent-pass risk in this file:
# 1) shader-f16 is required
# 2) no feature guard exists before requestDevice
# 3) catch path does not call testFailed
rg -n -C3 'requiredFeatures:\s*\["shader-f16"\]|features\.has\("shader-f16"\)|main\(\)\.catch|testFailed' LayoutTests/fast/webgpu/ftoi.htmlRepository: oven-sh/WebKit
Length of output: 469
Add a capability guard to prevent silent test pass when shader-f16 is unavailable.
The test requires shader-f16 at device initialization, but lacks a feature check beforehand. If the adapter does not support shader-f16, the requestDevice() call fails, the catch block logs the error and calls notifyDone(), allowing the test to exit without running assertions. Add an explicit capability gate to skip the test properly on unsupported adapters.
Suggested fix
async function main() {
const adapter = await navigator.gpu.requestAdapter({});
+ if (!adapter?.features.has("shader-f16")) {
+ debug("Skipping: shader-f16 is unsupported on this adapter.");
+ return;
+ }
const device = await adapter.requestDevice({
requiredFeatures: ["shader-f16"],
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/webgpu/ftoi.html` around lines 74 - 76, Add an explicit
capability gate: before calling adapter.requestDevice(...) check
adapter.features.has("shader-f16"); if that returns false, mark the test as
skipped (e.g. call testRunner.skip("shader-f16 not supported") or otherwise log
a skip and return) so the code does not call adapter.requestDevice and does not
silently exit via the catch/notifyDone path.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
LayoutTests/fast/text/font-face-empty-string.html (1)
24-24:⚠️ Potential issue | 🟡 MinorAdd explicit coverage for
fontface.family = ''.This only asserts
fontface.family = '\"\"'. Since behavior changed for empty string assignment, add a directshouldNotThrow("fontface.family = ''")check to prevent regressions.Proposed test update
shouldNotThrow("fontface.family = '\"\"'"); +shouldNotThrow("fontface.family = ''");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LayoutTests/fast/text/font-face-empty-string.html` at line 24, Add an explicit test to cover assigning an empty string to the fontface.family property: update the test that currently calls shouldNotThrow("fontface.family = '\"\"'") to also call shouldNotThrow("fontface.family = ''") so the suite verifies that assigning an empty string does not throw; locate the invocation of shouldNotThrow in the file (the existing call with '\"\"') and add the new shouldNotThrow invocation immediately alongside it to ensure regression coverage.JSTests/complex/intl-default-timezone-backward-link.js (1)
1-10: 🧹 Nitpick | 🔵 TrivialThe test is already properly configured with
--useTemporal=1in complex.yaml.The file does use
Temporal.Now.timeZoneId()at line 9, and the harness (complex.yaml, lines 67–73) already provides the necessary--useTemporal=1flag. However, no files in theJSTests/complexdirectory currently have embedded//@ requireOptions("--useTemporal=1")directives; the codebase pattern relies on harness configuration instead. If independent test execution becomes a requirement, adding the directive would improve self-containment, but it is not essential given the current setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/complex/intl-default-timezone-backward-link.js` around lines 1 - 10, This test uses Temporal via Temporal.Now.timeZoneId() but lacks a self-contained directive; add the harness directive //@ requireOptions("--useTemporal=1") at the top of JSTests/complex/intl-default-timezone-backward-link.js so the test can run independently of complex.yaml; locate the file by the presence of shouldBe(...) and Temporal.Now.timeZoneId() and insert the requireOptions directive as the first line.JSTests/wasm/debugger/lib/utils.py (1)
15-57: 🧹 Nitpick | 🔵 TrivialConsider disabling ANSI colors for non-TTY output.
Unconditional escape codes can reduce CI log readability. Gate color usage on
sys.stdout.isatty()/sys.stderr.isatty(), or add aNO_COLORoverride.Suggested refactor
import sys +import os @@ class Colors: - GREEN = "\033[92m" - RED = "\033[91m" - YELLOW = "\033[93m" - BLUE = "\033[94m" - CYAN = "\033[96m" - RESET = "\033[0m" - BOLD = "\033[1m" - DIM = "\033[2m" + _enabled = ( + os.environ.get("NO_COLOR") is None + and sys.stdout.isatty() + and sys.stderr.isatty() + ) + GREEN = "\033[92m" if _enabled else "" + RED = "\033[91m" if _enabled else "" + YELLOW = "\033[93m" if _enabled else "" + BLUE = "\033[94m" if _enabled else "" + CYAN = "\033[96m" if _enabled else "" + RESET = "\033[0m" if _enabled else "" + BOLD = "\033[1m" if _enabled else "" + DIM = "\033[2m" if _enabled else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/wasm/debugger/lib/utils.py` around lines 15 - 57, The prints always inject ANSI escape sequences which can clutter non-TTY outputs; update the Colors class and logging helpers (Colors, log_pass, log_fail, log_info, log_warning, log_error, log_success, log_verbose, log_header) to disable color codes when output is not a TTY or when NO_COLOR is set: detect sys.stdout.isatty() and sys.stderr.isatty() and an environment override (e.g., os.environ.get("NO_COLOR")), and make Colors return empty strings for color constants when coloring is disabled so existing log_* functions can remain unchanged.JSTests/wasm/debugger/tests/tests.py (1)
158-185:⚠️ Potential issue | 🟡 MinorLine 158: the outer loop is currently a no-op.
patternsis rebuilt 10 times, but Lines 183-185 run after that loop finishes, so this step-instruction sequence executes once, not 10 times. Either indent Lines 183-185 into the outer loop or remove the unused repetition.Proposed fix
for _ in range(10): patterns = [ ["-> 0x400000000000018d <+30>: local.get 2"], ["-> 0x400000000000018f <+32>: i32.load 12"], ["-> 0x4000000000000192 <+35>: local.get 2"], ["-> 0x4000000000000194 <+37>: i32.load 8"], ["-> 0x4000000000000197 <+40>: i32.add"], ["-> 0x4000000000000198 <+41>: i32.store 4"], ["-> 0x400000000000019b <+44>: local.get 2"], ["-> 0x400000000000019d <+46>: i32.load 4"], ["-> 0x40000000000001a0 <+49>: return"], ["-> 0x40000000000001e0 <+61>: i32.store 0"], ["-> 0x40000000000001e3 <+64>: local.get 0"], ["-> 0x40000000000001e5 <+66>: i32.load 0"], ["-> 0x40000000000001e8 <+69>: local.set 1"], ["-> 0x40000000000001ea <+71>: local.get 0"], ["-> 0x40000000000001ec <+73>: i32.const 16"], ["-> 0x40000000000001ee <+75>: i32.add"], ["-> 0x40000000000001ef <+76>: global.set 0"], ["-> 0x40000000000001f5 <+82>: local.get 1"], ["-> 0x40000000000001f7 <+84>: return"], ["-> 0x4000000000000201"], ["-> 0x400000000000018b <+28>: local.get 2"], ] - - for pattern in patterns: - self.session.cmd("si") - self.session.cmd("dis", patterns=pattern) + for pattern in patterns: + self.session.cmd("si") + self.session.cmd("dis", patterns=pattern)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@JSTests/wasm/debugger/tests/tests.py` around lines 158 - 185, The outer for _ in range(10): loop rebuilds patterns but never exercises it because the inner loop that calls self.session.cmd("si") and self.session.cmd("dis", patterns=pattern) runs after the outer loop; fix by moving the inner loop (the for pattern in patterns: block that calls self.session.cmd("si") and self.session.cmd("dis", patterns=pattern)) inside the outer loop so the step/disassemble sequence runs 10 times, or remove the redundant outer loop if only one iteration is intended; locate the patterns variable and the calls to self.session.cmd("si") / self.session.cmd("dis", patterns=pattern) in tests.py to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@JSTests/complex/intl-default-timezone-etc-unknown.js`:
- Around line 1-4: The shouldBe function's error message only shows the actual
value; update it to include both actual and expected to improve diagnostics by
throwing a more informative Error (e.g., include "expected" and "actual" or a
formatted string) inside shouldBe so failed assertions show both values and help
identify which check failed.
In `@JSTests/stress/bigint-cached-mod-edge-cases.js`:
- Around line 308-330: The range-only assertions using shouldBe(x >= 0n && x <
p2, true) in the two divisor-switch blocks (variables p1, p2, x where you first
iterate modulo p1 then modulo p2) are too weak; replace those range checks with
deterministic exact-value assertions that compare x to the expected final BigInt
constant (as you did earlier in this file) so the test fails on a
stale-cache/mod-path regression; update the two shouldBe calls at the ends of
those blocks to assert x === <expectedBigInt> (compute the expected remainder
deterministically and use that literal).
In `@JSTests/stress/json-parse-jsstring-cache-coherence.js`:
- Around line 102-118: The test currently uses a fixed small iteration count
(20) in the blocks that build arrays values and keys and iterate checks, which
is too weak to force eviction of the JSON atom/string cache; replace the
hardcoded 20 with a larger, eviction-inducing value (e.g., at least twice the
cache capacity like 512 or compute from the known cache slot count) and apply
that variable to every loop and array construction (the for-loops that fill
values and keys, the construction of json, and the validation loops using
shouldBe) so the test reliably creates many distinct strings to trigger eviction
during JSON.parse.
In `@JSTests/stress/megamorphic-cache-super-property.js`:
- Around line 1-4: makeProtoObject currently creates a plain object and ignores
Object.create, so it doesn't produce a prototype-backed object; change the
function so it returns the object created by Object.create(obj) (or assign and
return that created object) instead of returning the original obj, ensuring
callers receive an object whose prototype has property x; update the function
makeProtoObject accordingly.
In `@JSTests/stress/regexp-noncapturing-group-backreference-count.js`:
- Around line 7-13: The catch in expectInvalid should verify the thrown
exception is a SyntaxError and contains "invalid backreference" before
continuing, and the final failure should throw an Error object instead of a
string; update the catch block in expectInvalid to check for (e instanceof
SyntaxError && e.message.includes("invalid backreference")) and replace the
final throw "Expected an exception for /" + pattern + "/" + mods with throw new
Error("Expected an exception for /" + pattern + "/" + mods) so errors follow the
test-suite's typed Error convention.
In `@JSTests/stress/temporal-timezone.js`:
- Line 1: The test currently skips entirely via the top-line directive "//@ skip
if $hostOS == "windows" || $hostOS == "linux"", which is too broad; instead
restrict skipping to only the platform-dependent assertions by either (a)
removing the global skip and wrapping the host-specific checks in conditional
guards that check $hostOS (e.g., around the parsing/canonicalization
assertions), or (b) split those host-variant assertions into a separate test
file with its own "//@ skip if ..." directive; update the file
Temporal-timezone.js accordingly so non-host-dependent tests still run while
only the truly platform-dependent assertions remain skipped.
In `@LayoutTests/accessibility/aria-hidden-focus-override.html`:
- Around line 11-14: The buttons in this test use implicit button elements which
can default to type="submit" in forms; update the three elements with ids
button1, button2, and outside to include an explicit attribute type="button" to
silence lint warnings and avoid accidental submit behavior if the markup
changes. Locate the <button id="button1">, <button id="button2"> and <button
id="outside"> elements and add type="button" to each tag.
In `@LayoutTests/fast/flexbox/deprecated-flex-box-writing-mode-shrink.html`:
- Line 41: The markup is missing explicit accessibility/type attributes which
causes lint noise: add an alt attribute to the <img> in the test (e.g.,
alt="placeholder image" or alt="decorative image" if purely decorative), add a
title attribute to the <iframe> (e.g., title="test iframe") to satisfy
accessibility requirements, and add an explicit type attribute to the <button>
(e.g., type="button") to avoid implicit submit behavior; update the <img>,
<iframe>, and <button> elements accordingly in the file.
In `@LayoutTests/fast/harness/full_results.json`:
- Around line 184-206: The JSON summary's num_regressions field is inconsistent
with the actual number of "report": "REGRESSION" entries; update the
num_regressions value to 10 so it matches the ten REGRESSION entries (search for
the "num_regressions" key and verify against occurrences of "report":
"REGRESSION" in the JSON before saving).
In `@LayoutTests/fast/selectors/has-invalidation-traversal-size.html`:
- Line 2: The HTML file has structural issues: add a lang attribute to the
<html> element (e.g., <html lang="en">), create a proper <head> section
containing a <title> element, and remove or pair the unmatched closing </script>
tag (the stray </script> near the end) so all <script> tags are balanced; ensure
the document has a valid <head> and matching script tags to eliminate
parser/lint errors.
---
Outside diff comments:
In `@JSTests/complex/intl-default-timezone-backward-link.js`:
- Around line 1-10: This test uses Temporal via Temporal.Now.timeZoneId() but
lacks a self-contained directive; add the harness directive //@
requireOptions("--useTemporal=1") at the top of
JSTests/complex/intl-default-timezone-backward-link.js so the test can run
independently of complex.yaml; locate the file by the presence of shouldBe(...)
and Temporal.Now.timeZoneId() and insert the requireOptions directive as the
first line.
In `@JSTests/wasm/debugger/lib/utils.py`:
- Around line 15-57: The prints always inject ANSI escape sequences which can
clutter non-TTY outputs; update the Colors class and logging helpers (Colors,
log_pass, log_fail, log_info, log_warning, log_error, log_success, log_verbose,
log_header) to disable color codes when output is not a TTY or when NO_COLOR is
set: detect sys.stdout.isatty() and sys.stderr.isatty() and an environment
override (e.g., os.environ.get("NO_COLOR")), and make Colors return empty
strings for color constants when coloring is disabled so existing log_*
functions can remain unchanged.
In `@JSTests/wasm/debugger/tests/tests.py`:
- Around line 158-185: The outer for _ in range(10): loop rebuilds patterns but
never exercises it because the inner loop that calls self.session.cmd("si") and
self.session.cmd("dis", patterns=pattern) runs after the outer loop; fix by
moving the inner loop (the for pattern in patterns: block that calls
self.session.cmd("si") and self.session.cmd("dis", patterns=pattern)) inside the
outer loop so the step/disassemble sequence runs 10 times, or remove the
redundant outer loop if only one iteration is intended; locate the patterns
variable and the calls to self.session.cmd("si") / self.session.cmd("dis",
patterns=pattern) in tests.py to apply the change.
In `@LayoutTests/fast/text/font-face-empty-string.html`:
- Line 24: Add an explicit test to cover assigning an empty string to the
fontface.family property: update the test that currently calls
shouldNotThrow("fontface.family = '\"\"'") to also call
shouldNotThrow("fontface.family = ''") so the suite verifies that assigning an
empty string does not throw; locate the invocation of shouldNotThrow in the file
(the existing call with '\"\"') and add the new shouldNotThrow invocation
immediately alongside it to ensure regression coverage.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Line 24: Line 24 unconditionally sets policy CMP0157; wrap the
cmake_policy(SET CMP0157 NEW) call in a guard that checks if POLICY CMP0157 is
defined (e.g., if(POLICY CMP0157) then cmake_policy(SET CMP0157 NEW) endif()) so
older CMake (<3.29) won't error, or alternatively raise cmake_minimum_required
to 3.29 if you prefer to require that policy; reference the CMP0157 symbol and
the cmake_policy call when making the change.
In `@JSTests/microbenchmarks/regexp-forward-reference.js`:
- Around line 4-5: The regex variables re1 and re2 are declared with let but
never reassigned; change their declarations to use const to make them
immutable—update the bindings for re1 (the pattern /\2(a)(b)/) and re2 (the
pattern /\k<y>(?<x>a)(?<y>b)/) to use const so the intent of no reassignment is
explicit.
In `@JSTests/stress/async-function-empty-resolved-promise.js`:
- Around line 47-51: The test incorrectly expects disposedSync/disposedAsync to
equal 1 even though run() performs 1000 iterations; instead, reset the disposal
counters at the start of each iteration or assert against the current iteration
count. Modify the loop inside run() so that before calling onlyUsing() /
onlyAwaitUsing() you reset disposedSync = 0 and disposedAsync = 0 (or,
alternatively, change the assertions to compare disposedSync/disposedAsync to
the iteration index + 1), ensuring the checks around the symbols onlyUsing,
onlyAwaitUsing, disposedSync, disposedAsync, and run reflect per-iteration
disposal rather than a global total.
In
`@JSTests/stress/growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js`:
- Line 12: Replace the hardcoded loop bound 50000 with a named constant to
express the 10:1 relationship to ITERATIONS_PER_ROUND; add a constant like
AGENT_ITERATIONS = ITERATIONS_PER_ROUND * 10 and use it in the loop condition
(the for loop that currently reads "for (let i = 0; i < 50000; i++)"), so the
intent is explicit and maintenance stays aligned with ITERATIONS_PER_ROUND.
- Around line 13-17: The current try/catch around sab.grow(...) (and the similar
block at lines 27-31) swallows all exceptions; change the catch to only absorb
the expected resize/prototype-operation errors (e.g., RangeError or the specific
error type thrown by SharedArrayBuffer.prototype.grow and prototype method
calls) and rethrow any other exceptions so unexpected failures are visible;
locate the try blocks wrapping sab.grow(...) and the prototype-operation
try/catch and replace the empty catch { } with logic that checks the exception
type/name/message and only suppresses known, expected errors, otherwise rethrow
the exception.
- Around line 34-35: The test currently calls $.agent.getReport() and discards
its return value; change this to assert the agent report equals the expected
string "done" so worker behavior is validated—locate the calls to
$.agent.sleep(50) and $.agent.getReport() in
growable-sharedarraybuffer-parallel-grow-during-prototype-methods.js and replace
the discarded call with an assertion that the payload returned by
$.agent.getReport() is "done" (use the project’s test assertion helper already
used elsewhere in the file).
In `@JSTests/stress/intl-segmenter-word-iswordlike-number.js`:
- Around line 21-29: Extract the repeated cursor/iteration/assert loop into a
single helper function (e.g., assertSegmentResults or verifySegmentLoop) that
accepts the Segmenter instance (segmenter), the input string (input), and the
expected results array (results); move the body that sets cursor = 0, iterates
"for (let { segment, index, isWordLike } of segmenter.segment(input))", and
calls shouldBe checks for index, index+segment.length, segment, and isWordLike,
and the final shouldBe(cursor, results.length) into that helper, then replace
each of the four duplicated blocks (the occurrences around lines 21-29, 43-50,
64-71, 89-96) with calls to the helper; keep the helper in the same test file
and ensure it uses the same shouldBe helper in scope.
In `@JSTests/stress/promise-prototype-finally-bug-312466.js`:
- Line 42: The failure-label strings use awkward possessives like "this'" and
"it's" which should be cleaned up; update the message passed to shouldThrow
(e.g., the template string in shouldThrow(`mimic this' constructor does not have
[[Construct]]: ${name}`, ...)) to a clearer possessive form such as "mimic this
constructor's" or rephrase to "the constructor of this mimic" and make the same
change for the other occurrences of the same pattern used elsewhere in the file
(the other shouldThrow calls at the same test sites).
In `@JSTests/wasm/debugger/lib/environment.py`:
- Around line 73-79: The validate logic must ensure that a fallback "lldb" is
actually on PATH: in the code paths using _find_lldb_path() and the block that
sets self.lldb_path = "lldb", update validate() (or the same check around
self.lldb_path) to call shutil.which("lldb") (or equivalent PATH lookup) when
self.lldb_path == "lldb" and treat it as not found if which returns None; log a
warning/error and return False instead of reporting success so DebugSession
won't later fail to spawn LLDB. Also adjust the success log (log_success for
LLDB found) to only run when the actual executable path is verified.
In `@JSTests/wasm/debugger/lib/session.py`:
- Around line 156-165: The cmd() function currently writes commands to the
shared LLDB session and then calls _wait(), but _wait() can match output
produced before the current command; fix this by fencing command output: before
writing the actual command in cmd(), emit a unique sentinel/fence token (e.g.,
based on self._name and a monotonic counter or uuid) to _lldb.stdin and flush,
then write the real command; update _wait() (or add a new helper parameter) so
it discards any queued output until it sees that sentinel and only considers
lines after the sentinel when matching patterns (use the existing
_DEFAULT_PATTERNS and pass the fence through the call from cmd() to _wait());
ensure the sentinel is unique per cmd() invocation so commands are properly
isolated.
In `@JSTests/wasm/debugger/resources/swift-wasm/fatal-error-test/main.js`:
- Around line 43-52: The loop calls instance.exports.trigger_fatal_error which
may be undefined, causing a swallowed TypeError and an infinite loop; add a
fail-fast check after retrieving triggerFatalError (from
instance.exports.trigger_fatal_error) and if it's falsy throw or print a clear
error and exit the process (or break the loop) so the harness does not loop
forever when the export is missing.
In `@JSTests/wasm/debugger/test-wasm-debugger.py`:
- Around line 97-100: The test runner currently instantiates env =
WebKitEnvironment(Path(__file__), build_config) but never validates it; call
env.validate() immediately after constructing the WebKitEnvironment (before
scheduling or creating all_tests) and abort (raise or sys.exit with a clear
error) if validation fails so missing tools like jsc/LLDB are detected early;
reference the WebKitEnvironment instance (env) and its validate() method when
adding this check.
In `@JSTests/wasm/debugger/tests/jsc.py`:
- Around line 35-36: The current except block in execute() swallows
subprocess.TimeoutExpired by only calling log_warning("JavaScriptCore test timed
out but continuing"), which makes callers treat a timed-out run as a pass;
change this to surface the failure by either re-raising the TimeoutExpired
(raise) or returning a non-zero/failure status so the suite fails; update the
except subprocess.TimeoutExpired handler in execute() (and remove or keep the
log_warning call if desired) so the timeout is propagated to callers instead of
being treated as success.
In `@JSTests/wasm/multi-memory-spec-harness/sync_index.js`:
- Around line 180-184: The thrown Error message uses template placeholders
inside single quotes so interpolation doesn't occur; update the throw in the
catch block (the line creating new Error('WebAssembly.Module ctor unexpectedly
throws ${typeof e}: ${e}${e.stack}')) to use a template literal (backticks) or
string concatenation so ${typeof e}, ${e} and ${e.stack} are evaluated and
included in the error message.
In `@JSTests/wasm/stress/fmin-fmax-constant-fold.js`:
- Around line 93-96: Precompute the expected denormal constants once before the
loop and replace repeated Math.pow/Math.fround calls inside the assertions with
those variables; e.g., compute expectedF32Min = -Math.fround(Math.pow(2, -149)),
expectedF32Max = Math.fround(Math.pow(2, -149)), expectedF64Min = -Math.pow(2,
-1074), expectedF64Max = Math.pow(2, -1074) and then use
expectedF32Min/expectedF32Max/expectedF64Min/expectedF64Max in the calls to
instance.exports.f32_min_denorm(), f32_max_denorm(), f64_min_denorm(), and
f64_max_denorm().
In `@LayoutTests/accessibility/aria-hidden-deep-dom-no-crash.html`:
- Around line 2-6: The document header is missing the lang attribute on the
<html> element and a <title> element in the <head>; update the <html> start tag
to include an appropriate lang attribute (e.g., lang="en") and add a <title>
element inside the <head> (before or after the script tags) to provide a
document title for accessibility and linting.
In `@LayoutTests/accessibility/aria-hidden-focus-not-overridden-when-caught.html`:
- Around line 2-6: The HTML document is missing required metadata: add a lang
attribute to the <html> element and include a <title> element inside the <head>;
update the <html> tag to include an appropriate language code (e.g., lang="en")
and add a concise <title> element within the <head> so the file
(accessibility/aria-hidden-focus-not-overridden-when-caught.html) passes HTML
lint checks.
In `@LayoutTests/accessibility/aria-hidden-focus-override.html`:
- Around line 2-6: The HTML page is missing a lang attribute and a title element
which fails lint checks; update the <html> tag to include an appropriate lang
(e.g., lang="en") and add a <title> element inside the <head> (e.g., <title>ARIA
Hidden Focus Override</title>) so the document has language metadata and a page
title; modify the existing <html> and <head> elements in this file accordingly.
In `@LayoutTests/accessibility/base-select-option-border-radius-path.html`:
- Around line 2-19: Add a lang attribute to the <html> tag and include a <title>
element inside the <head> to satisfy HTML lint rules: update the <html> start
tag to include an appropriate language code (e.g., lang="en") and add a
descriptive <title> element within the <head> block above the stylesheet/scripts
so the page has a valid language declaration and a document title.
- Around line 44-48: The test currently only waits for opt1 with
waitUntilIDHasPathWith("opt1", 1, "Curve to") before asserting path segments for
opt1/opt2/opt3, which can cause races for opt2/opt3; add waits for the other
options by calling waitUntilIDHasPathWith("opt2", 1, "Curve to") and
waitUntilIDHasPathWith("opt3", 1, "Curve to") (or a loop over
["opt1","opt2","opt3"]) before the expect(...) assertions so all paths are
present prior to asserting pathSegmentCountOfID.
In `@LayoutTests/accessibility/base-select-option-press-closes-popover.html`:
- Around line 2-11: Add missing HTML metadata: set the root <html> element's
lang attribute (e.g., lang="en") and add a <title> element inside the <head>
(before the closing </head>) to provide document language and title metadata;
update the <html> tag and the <head> section in this file (the existing <html>
element and the <head> block) accordingly.
In `@LayoutTests/accessibility/element-path-with-css-transforms.html`:
- Around line 67-68: The current assertions only compare bounds1 and bounds2
sizes, missing bounds3; update the assertions that use bounds1 and bounds2 (the
two lines calling expect with Math.abs(bounds1.width - bounds2.width) < 2 and
Math.abs(bounds1.height - bounds2.height) < 2) to also assert that bounds3
matches the others (e.g., add comparisons Math.abs(bounds1.width -
bounds3.width) < 2 and Math.abs(bounds2.width - bounds3.width) < 2 for width and
the equivalent two for height) so item3's size regressions are detected.
- Around line 52-57: The test currently only calls
waitUntilIDHasPathWith("item1", 1, "Curve to") before asserting path segments
for item1, item2, and item3, which can cause races; update the setup to wait for
all items by calling waitUntilIDHasPathWith for "item2" and "item3" as well (or
iterate over ["item1","item2","item3"]) so each element's AX path is cached
before the expect(...) assertions run.
- Around line 3-33: The head is missing a title element which triggers a lint
error; add a <title> element inside the existing <head> (e.g., using the test
name "element-path-with-css-transforms" or similar descriptive text) so the
document has a proper title node before the head closes, keeping it placed
before or after the existing script/style tags as appropriate.
- Line 2: Add a lang attribute to the root html element to satisfy accessibility
linting: update the root <html> tag (the document's top-level html element) to
include an appropriate language code (e.g., lang="en") so the file
element-path-with-css-transforms.html has a valid language declaration.
In `@LayoutTests/accessibility/left-right-line-range-display-table-in-flex.html`:
- Around line 2-17: The HTML is missing document metadata: add a lang attribute
to the <html> element (e.g., lang="en") and include a <title> element inside the
<head> (e.g., <title>Left-right line range display table in flex</title>) so
lint checks pass; update the file's top tag and the <head> block where the
scripts/styles are referenced to include these additions.
In
`@LayoutTests/accessibility/mac/live-regions/live-region-relevant-text-non-atomic.html`:
- Line 25: The code uses the bare identifier accessibilityController which can
throw ReferenceError when the global is absent; update the guard to check the
global safely (e.g., use typeof window !== "undefined" && typeof
window.accessibilityController !== "undefined" or check typeof
accessibilityController !== "undefined") before referencing it so the
conditional becomes a safe existence check for accessibilityController.
- Around line 2-7: The HTML document is missing required metadata: add a lang
attribute to the <html> element (e.g., lang="en") and include a <title> element
inside the <head> with an appropriate page title to satisfy accessibility and
static-analysis checks; update the existing <html> start tag to include the lang
attribute and insert a concise <title> element in the <head> section near the
existing meta and script tags.
In `@LayoutTests/css3/flexbox/flexitem.html`:
- Line 139: The <img> element with attributes data-expected-height="600" and
style="flex-shrink:1;height:1000px;min-height:min-content;" is missing an alt
attribute and should be marked decorative; add alt="" to that <img> element so
it satisfies the alt-require rule while keeping the rest of the tag unchanged.
In `@LayoutTests/editing/execCommand/break-out-of-list-resets-start-number.html`:
- Around line 2-6: The HTML document is missing required metadata: add a lang
attribute to the <html> element and include a <title> element inside the <head>
to satisfy lint rules; update the existing <html> start tag to include an
appropriate language value (e.g., lang="en") and add a concise
<title>...</title> node inside the <head> block (above or below the existing
<script> tags) so the document has both language and title metadata.
In `@LayoutTests/editing/execCommand/outdent-list-resets-start-number.html`:
- Around line 2-6: Add a language attribute to the root <html> element and
include a <title> inside the <head>; specifically, update the <html> tag to
include a suitable lang value (e.g., lang="en") and add a concise <title>
element in the <head> section (e.g., <title>Outdent list resets start
number</title>) so the document satisfies linting and accessibility
requirements.
In
`@LayoutTests/editing/spelling/grammar-correction-shown-clicking-middle-of-word.html`:
- Around line 21-23: The test enables continuous spell checking via
window.internals.setContinuousSpellCheckingEnabled(true) without restoring the
prior state; modify the test to read and save the current state (e.g., call
window.internals.continuousSpellCheckingEnabled or
getContinuousSpellCheckingEnabled if available) before changing it, then restore
that saved value in a teardown/cleanup step (after assertions or via an
add_cleanup handler) so the callsites
internals.setContinuousSpellCheckingEnabled(...) in this file (and the similar
block at lines 51-54) do not leak state to other tests.
- Around line 24-25: runTest() accesses the global internals object; add an
explicit early-exit at the start of runTest to handle environments where
internals is undefined by checking if (!internals) and then performing a clean
exit (call finish() if available, or return) before any use of internals
(references in runTest and at the locations handling selection/state). This
prevents throws outside WebKitTestRunner and ensures the test exits cleanly.
In
`@LayoutTests/fast/css-generated-content/first-letter-stale-checked-ptr-crash.html`:
- Around line 11-15: Wrap the calls to
window.internals?.setImmediateRendererDestructionEnabled(true) and
...setImmediateRendererDestructionEnabled(false) in a try/finally so the false
call always runs; specifically, enable immediate renderer destruction, perform
the DOM operations (document.body.offsetHeight,
getElementById('target').prepend('A'), document.body.offsetHeight) inside the
try block, and call
window.internals?.setImmediateRendererDestructionEnabled(false) in the finally
block to guarantee cleanup even if an exception is thrown.
In `@LayoutTests/fast/css/expedia-group-animation-quirk.html`:
- Around line 1-12: The document currently omits an explicit <html> root element
and lacks a <title> inside the <head>; update the top-level markup to include an
explicit <html> element wrapping the existing <head> (and future <body>) and add
a descriptive <title> element inside the <head> (e.g., matching the test
purpose) so the page no longer relies on implicit root insertion; locate the
DOCTYPE and existing <head> and script tags in the diff to apply these changes.
- Around line 68-73: The current assertions use || allowing partial matches;
change them to require both expected values by checking that
style.animationDelay (delays) includes both "0s" and "0.06s" and that
style.animationDuration (durations) includes both "0.18s" and "0.06s", updating
the assert messages to show both expected values if the check fails; locate and
modify the checks referencing delays, durations and
style.animationDelay/style.animationDuration to perform two includes checks
combined with && instead of the existing || logic.
In
`@LayoutTests/fast/css3-text/css3-text-wrap/text-wrap-pretty-line-break-crash-6.html`:
- Around line 3-12: Head element in this file is missing a <title>, causing
title-require lint failures; add a <title> element inside the existing <head>
(next to the <meta charset="UTF-8"> and <style>) with a brief descriptive string
(e.g., "text-wrap-pretty-line-break-crash-6") so the document satisfies the
title requirement.
- Around line 18-26: The HTML in the snippet leaves unclosed container <div>s
(the ones with id="testArena", class="comment-thread", and class="comment")
before the closing </body>, which can cause crashes; fix by adding the
corresponding closing </div> tags in the correct nesting order to close the <div
id="testArena">, the inner <div class="comment-thread"> and <div
class="comment"> (matching the opening tags shown) so all opened containers are
properly closed before </body>.
In `@LayoutTests/fast/dom/NodeList/nodelist-array-prototype-indexOf.html`:
- Around line 70-74: The test only checks initial indexOf results; make it
validate liveness by mutating the DOM and re-checking indexOf on the same
nodeList instance: after obtaining nodeList = container.childNodes and the
initial asserts, append or insert a new child (or move an existing child) on
container (e.g., container.appendChild(newNode) or
container.insertBefore(newNode, someSibling)) and then assert
indexOf.call(nodeList, newNode) returns the expected index and that previously
indexed nodes' indexes update accordingly; reference nodeList, container,
indexOf.call, container.firstChild and outside to locate where to insert the
mutation and new assertions.
In
`@LayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-deep-expected.html`:
- Around line 13-14: Update the pass-condition paragraph to match the rendered
HTML: the <p> element that currently reads "Test passes if there is a filled
green square and no red." should be changed to reflect two boxes (e.g., "Test
passes if there are two filled green squares and no red.") to match the two <div
class="box"> elements present in the markup; alternatively, if the intent was a
single box, remove one of the <div class="box"> elements so the text and
rendered output agree.
In `@LayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-deep.html`:
- Line 31: Update the pass-condition text inside the paragraph that currently
reads "Test passes if there is a filled green square and no red." to reflect
there are two boxes; change it to a plural form such as "Test passes if there
are two filled green squares and no red." so the wording matches the rendered
expectation described elsewhere in the test.
In
`@LayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item-expected.html`:
- Around line 13-14: Update the pass-condition paragraph to match the actual
rendered output: change the descriptive text in the <p> element (the
pass-condition sentence) so it refers to two squares/boxes (or otherwise matches
the presence of the two <div class="box"> elements) instead of saying one
square; edit the sentence text in the same file near the <p> element that
precedes the two <div class="box"> nodes so the pass-condition accurately
reflects the rendered result.
In `@LayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item.html`:
- Line 31: Update the pass-condition paragraph text to match the rendered output
by changing the singular phrase "a filled green square" to "two filled green
squares" so the sentence describes two green boxes (make the same edit where the
pass-condition string is defined in
LayoutTests/fast/flexbox/quirks-percentage-height-in-flex-item.html to match the
wording on the other line).
In `@LayoutTests/fast/forms/HTMLOptionElement_label02.html`:
- Line 5: The test's markup is inconsistent: the <option> element currently has
label="" but also contains visible text "should display this"; update the
element so the attribute and content match the test intent — either remove the
inner text when testing an empty label (keep label="") or set the label
attribute to "should display this" if the test expects that visible text; locate
the element with the exact snippet 'option label=""' and make the attribute or
inner text change accordingly to resolve the contradiction.
In `@LayoutTests/fast/harness/results.html`:
- Around line 302-318: In testNameAndVariant, the code currently splits on '?'
then '#' sequentially which can drop suffixes (e.g. "foo.html#frag?x"); change
the logic to find the earliest separator index between '?' and '#' (treating
missing indexes as absent), then perform a single split at that index so the
variant is everything from that separator to the end and testName is the prefix;
update references in testNameAndVariant to use this earliest-separator logic.
In `@LayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.html`:
- Line 3: The file contains an invalid closing tag for the void element <embed>
(the element with id "embed"); remove the trailing </embed> entirely so the
element is written only as a single self-contained start tag (e.g. <embed
id="embed" ...>), ensuring no </embed> remains in
LayoutTests/fast/pdf-plugin-destruction-dispatches-print-event-crash.html and
the provided grep check (rg '</embed>') returns no results.
In `@LayoutTests/fast/reflections/reflection-removed-crash-2.html`:
- Line 1: The test reflection-removed-crash-2.html is missing an explicit
doctype; prepend <!DOCTYPE html> as the very first line of the file (before the
existing <style> tag/content) so the page runs in standards mode and avoids
quirks-mode dependency.
In
`@LayoutTests/fast/scrolling/scroll-anchoring/scroll-anchoring-during-rubberband.html`:
- Around line 2-4: Add the missing document metadata by setting a language on
the root html element and adding a title inside the head: update the <html> tag
to include a lang attribute (e.g., lang="en") and add a <title> element within
the <head> (e.g., <title>Scroll Anchoring Test</title>) so the HTML checks for
the <html> tag and the <head> section are satisfied.
- Around line 31-35: The test assumes UIHelper exists and awaits
UIHelper.renderingUpdate(), which can throw when UIHelper is undefined; update
the load handler to first check that window.UIHelper and a callable
UIHelper.renderingUpdate exist (e.g., typeof UIHelper.renderingUpdate ===
'function') before awaiting it, then proceed to the existing
internals.unconstrainedScrollTo call (keep the existing window.internals guard
and internals.unconstrainedScrollTo usage).
In `@LayoutTests/fast/selectors/focus-visible-script-focus-from-click.html`:
- Line 2: The root html element is missing a lang attribute which triggers the
html-lang-require check; update the root <html> tag to include an appropriate
lang attribute (e.g., lang="en" or the document's language) so the validator
passes, ensuring you modify the opening <html> element shown in the diff.
In `@LayoutTests/fast/selectors/has-invalidation-traversal-size.html`:
- Line 10: The unscoped global rule ":has(> .trigger) > .target" leaks into
other tests and masks negative cases; restrict this rule so it only applies
inside the "without compound peer" test case (the runTest call named ":has(>
.trigger) without compound peer") and update the expectations for the other
cases (e.g. the tests referencing .c8 and .c10 such as ":not(:has(.trigger))
inside :not() with compound peer .c8" and ":is(.other :has(.trigger)) inside
:is() with complex selector and compound peer .c10") by flipping their negative
assertions so they reflect the scoped rule rather than the previous global
behavior. Ensure the selector is wrapped or prefixed to target only the specific
test container for the without-compound-peer case and verify the related
occurrences at the other mentioned blocks (lines around the second and third
runTest instances) are updated accordingly.
In
`@LayoutTests/fast/text-extraction/debug-text-extraction-hidden-radio-checkbox.html`:
- Around line 44-74: The async load handler calls UIHelper.requestDebugText(...)
and then testRunner.notifyDone(), but if requestDebugText rejects notifyDone is
never called; wrap the await loop in a try/finally (or surround the core async
work with try and call testRunner.notifyDone() in a finally block) so notifyDone
is always invoked on completion or error; reference the load event listener
callback, the use of UIHelper.requestDebugText, and testRunner.notifyDone to
locate and update the handler.
- Around line 3-7: The HTML document is missing required metadata: add a lang
attribute to the <html> element (e.g., <html lang="en">) and include a <title>
element inside the <head> to satisfy lint/HTMLHint rules; update the <html>
start tag to include the appropriate language code and insert a concise
<title>...</title> within the existing <head> element.
In
`@LayoutTests/fast/webcodecs/resetting-audio-decoder-with-zero-size-crash.html`:
- Around line 7-10: The feature-detection condition uses the wrong operator
precedence: replace the faulty expression if (!"AudioDecoder" in window) with a
correct guard such as if (!("AudioDecoder" in window)) or if (typeof
AudioDecoder === "undefined") so the test exits on platforms without the API;
update the block that calls window.testRunner?.notifyDone() accordingly to
ensure immediate termination when AudioDecoder is absent.
- Around line 20-22: The test currently calls decoder.decode(chunk) and await
decoder.flush() then window.testRunner?.notifyDone(); but if decode() throws or
flush() rejects the notifyDone() is never called; wrap the decode/flush sequence
in a try/catch/finally (or attach a .catch) so that
window.testRunner?.notifyDone() is invoked regardless of success or failure,
ensuring any exceptions are either logged or rethrown after notifyDone;
reference the existing decoder.decode, decoder.flush, and
window.testRunner?.notifyDone() calls when making the change.
In `@LayoutTests/fast/webgpu/ftoi.html`:
- Around line 74-76: Before calling adapter.requestDevice with requiredFeatures
["shader-f16"], add a capability gate that checks the adapter object and that
adapter.features.has("shader-f16") is true; only call
adapter.requestDevice({...}) when that check passes, otherwise call notifyDone()
or testFailed(...) to avoid the test silently exiting on rejection; update the
code around adapter/requestDevice and any catch blocks to reflect this guard and
ensure assertions run only when the feature is supported.
In `@LayoutTests/TestExpectations`:
- Line 7766: The test expectation for
SVGGeometry-isPointInStroke-with-null-path.html in LayoutTests/TestExpectations
is missing the bug reference; update the "[ Debug ]
svg/dom/SVGGeometry-isPointInStroke-with-null-path.html [ Skip ]" entry to carry
the same bug link "webkit.org/b/290133" as the paired
SVGGeometry-isPointInFill-with-null-path.html so both debug skips reference the
same issue.
- Around line 1702-1703: The two test entries
"imported/w3c/web-platform-tests/css/css-flexbox/align-content-007.htm" and
"imported/w3c/web-platform-tests/css/css-flexbox/align-self-016.html" are
missing expectation tokens; update each line in TestExpectations to append an
appropriate status token (e.g., [ Pass ], [ Skip ], [ ImageOnlyFailure ], etc.)
so the file parses correctly, choosing the correct token for each test and
keeping the original test path intact.
- Around line 5995-6002: The skipped tests listed (e.g.,
imported/w3c/web-platform-tests/workers/same-site-cookies/first-party.all.tentative.https.window.html,
imported/w3c/web-platform-tests/workers/SharedWorker-extendedLifetime.html,
imported/w3c/web-platform-tests/workers/shared-worker-from-blob-url.window.html,
etc.) lack tracking bug references; update each "[ Skip ]" expectation in
LayoutTests/TestExpectations to include either an individual WebKit bug number
or a single shared tracking-bug reference (e.g., "[ Skip : crbug.com/XXXX ]" or
"[ Skip : bugXXXXXX ]") so they are tracked, and if they share a root cause use
the same tracking bug for all listed entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| function shouldBe(actual, expected) { | ||
| if (actual !== expected) | ||
| throw new Error('bad value: ' + actual); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve assertion failure diagnostics in shouldBe.
The current error message only prints actual, which slows down debugging when multiple checks exist.
Suggested diff
function shouldBe(actual, expected) {
if (actual !== expected)
- throw new Error('bad value: ' + actual);
+ throw new Error(`expected ${expected}, got ${actual}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/complex/intl-default-timezone-etc-unknown.js` around lines 1 - 4, The
shouldBe function's error message only shows the actual value; update it to
include both actual and expected to improve diagnostics by throwing a more
informative Error (e.g., include "expected" and "actual" or a formatted string)
inside shouldBe so failed assertions show both values and help identify which
check failed.
| // Switch divisors at same n=2 size. | ||
| { | ||
| const p1 = (1n << 128n) - 159n; | ||
| const p2 = (1n << 128n) - 173n; | ||
| let x = (1n << 200n) + 1n; | ||
| for (let i = 0; i < 150; i++) | ||
| x = (x * x) % p1; | ||
| for (let i = 0; i < 150; i++) | ||
| x = (x * x) % p2; | ||
| shouldBe(x >= 0n && x < p2, true); | ||
| } | ||
|
|
||
| // Switch divisors across sizes: n=2 then n=4. | ||
| { | ||
| const p1 = (1n << 128n) - 159n; | ||
| const p2 = 2n ** 255n - 19n; | ||
| let x = (1n << 200n) + 1n; | ||
| for (let i = 0; i < 150; i++) | ||
| x = (x * x) % p1; | ||
| for (let i = 0; i < 150; i++) | ||
| x = (x * x) % p2; | ||
| shouldBe(x >= 0n && x < p2, true); | ||
| } |
There was a problem hiding this comment.
Cache-invalidation checks need exact-value assertions, not only range checks.
At Line 317 and Line 329, asserting x >= 0n && x < p2 is too weak for this scenario. A stale-cache/mod-path bug can still yield an in-range but incorrect remainder, so these checks may miss the regression the block is targeting. Please assert deterministic final constants (as done in earlier sections) for both divisor-switch cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/bigint-cached-mod-edge-cases.js` around lines 308 - 330, The
range-only assertions using shouldBe(x >= 0n && x < p2, true) in the two
divisor-switch blocks (variables p1, p2, x where you first iterate modulo p1
then modulo p2) are too weak; replace those range checks with deterministic
exact-value assertions that compare x to the expected final BigInt constant (as
you did earlier in this file) so the test fails on a stale-cache/mod-path
regression; update the two shouldBe calls at the ends of those blocks to assert
x === <expectedBigInt> (compute the expected remainder deterministically and use
that literal).
| // 11. many distinct (first,last,len) keys evicting cached values | ||
| { | ||
| let values = []; | ||
| let keys = []; | ||
| for (let i = 0; i < 20; ++i) { | ||
| values.push("v" + String.fromCharCode(65 + i) + "x"); | ||
| keys.push("v" + String.fromCharCode(97 + i) + "x"); | ||
| } | ||
| let json = '['; | ||
| for (let i = 0; i < 20; ++i) | ||
| json += '"' + values[i] + '",{"' + keys[i] + '":0},"' + keys[i] + '",'; | ||
| json += '0]'; | ||
| let r = JSON.parse(json); | ||
| for (let i = 0; i < 20; ++i) { | ||
| shouldBe(r[i * 3], values[i], "many-evict value " + i); | ||
| shouldBe(r[i * 3 + 2], keys[i], "many-evict key-as-value " + i); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating JSONAtomStringCache sources..."
fd -i 'JSONAtomStringCache' Source
echo
echo "Inspecting likely size/capacity constants and cache structure..."
files=$(fd -i 'JSONAtomStringCache.*\.(h|hpp|cpp|mm)' Source)
if [ -n "${files}" ]; then
rg -n -C3 'cache|capacity|size|m_cache|slot|constexpr|JSONAtomStringCache' ${files}
fiRepository: oven-sh/WebKit
Length of output: 13873
Eviction stress may become too weak with fixed count 20.
The cache has capacity 256 slots (per JSONAtomStringCache.h:39), and with only 20 iterations creating ~40 distinct strings, collision is unlikely and eviction may not occur. The test comment explicitly names "evicting cached values" as the intent, but a fixed count risks silently losing that pressure if cache capacity changes.
Suggested hardening diff
- for (let i = 0; i < 20; ++i) {
+ const collisionCount = 128;
+ for (let i = 0; i < collisionCount; ++i) {
values.push("v" + String.fromCharCode(65 + i) + "x");
keys.push("v" + String.fromCharCode(97 + i) + "x");
}
@@
- for (let i = 0; i < 20; ++i)
+ for (let i = 0; i < collisionCount; ++i)
json += '"' + values[i] + '",{"' + keys[i] + '":0},"' + keys[i] + '",';
@@
- for (let i = 0; i < 20; ++i) {
+ for (let i = 0; i < collisionCount; ++i) {
shouldBe(r[i * 3], values[i], "many-evict value " + i);
shouldBe(r[i * 3 + 2], keys[i], "many-evict key-as-value " + i);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 11. many distinct (first,last,len) keys evicting cached values | |
| { | |
| let values = []; | |
| let keys = []; | |
| for (let i = 0; i < 20; ++i) { | |
| values.push("v" + String.fromCharCode(65 + i) + "x"); | |
| keys.push("v" + String.fromCharCode(97 + i) + "x"); | |
| } | |
| let json = '['; | |
| for (let i = 0; i < 20; ++i) | |
| json += '"' + values[i] + '",{"' + keys[i] + '":0},"' + keys[i] + '",'; | |
| json += '0]'; | |
| let r = JSON.parse(json); | |
| for (let i = 0; i < 20; ++i) { | |
| shouldBe(r[i * 3], values[i], "many-evict value " + i); | |
| shouldBe(r[i * 3 + 2], keys[i], "many-evict key-as-value " + i); | |
| } | |
| // 11. many distinct (first,last,len) keys evicting cached values | |
| { | |
| let values = []; | |
| let keys = []; | |
| const collisionCount = 128; | |
| for (let i = 0; i < collisionCount; ++i) { | |
| values.push("v" + String.fromCharCode(65 + i) + "x"); | |
| keys.push("v" + String.fromCharCode(97 + i) + "x"); | |
| } | |
| let json = '['; | |
| for (let i = 0; i < collisionCount; ++i) | |
| json += '"' + values[i] + '",{"' + keys[i] + '":0},"' + keys[i] + '",'; | |
| json += '0]'; | |
| let r = JSON.parse(json); | |
| for (let i = 0; i < collisionCount; ++i) { | |
| shouldBe(r[i * 3], values[i], "many-evict value " + i); | |
| shouldBe(r[i * 3 + 2], keys[i], "many-evict key-as-value " + i); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/json-parse-jsstring-cache-coherence.js` around lines 102 -
118, The test currently uses a fixed small iteration count (20) in the blocks
that build arrays values and keys and iterate checks, which is too weak to force
eviction of the JSON atom/string cache; replace the hardcoded 20 with a larger,
eviction-inducing value (e.g., at least twice the cache capacity like 512 or
compute from the known cache slot count) and apply that variable to every loop
and array construction (the for-loops that fill values and keys, the
construction of json, and the validation loops using shouldBe) so the test
reliably creates many distinct strings to trigger eviction during JSON.parse.
| function makeProtoObject(val) { | ||
| let obj = { x: val }; | ||
| Object.create(obj); | ||
| return obj; |
There was a problem hiding this comment.
makeProtoObject currently does not create a prototype-based object.
Object.create(obj) is unused, and the function returns obj directly. If the intent is to exercise prototype-chain reads/shapes, this helper is not doing that.
Proposed fix
function makeProtoObject(val) {
- let obj = { x: val };
- Object.create(obj);
- return obj;
+ let proto = { x: val };
+ return Object.create(proto);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function makeProtoObject(val) { | |
| let obj = { x: val }; | |
| Object.create(obj); | |
| return obj; | |
| function makeProtoObject(val) { | |
| let proto = { x: val }; | |
| return Object.create(proto); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/megamorphic-cache-super-property.js` around lines 1 - 4,
makeProtoObject currently creates a plain object and ignores Object.create, so
it doesn't produce a prototype-backed object; change the function so it returns
the object created by Object.create(obj) (or assign and return that created
object) instead of returning the original obj, ensuring callers receive an
object whose prototype has property x; update the function makeProtoObject
accordingly.
| } catch (e) { | ||
| if (e.message.includes("invalid backreference")) | ||
| continue; | ||
| throw e; | ||
| } | ||
|
|
||
| throw "Expected an exception for /" + pattern + "/" + mods; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification:
# 1) Find similar assertions in stress tests.
# 2) Compare message-matching and throw style conventions.
rg -n -C2 'invalid backreference|throw "Expected|throw new Error' JSTests/stressRepository: oven-sh/WebKit
Length of output: 50372
🏁 Script executed:
head -30 JSTests/stress/regexp-noncapturing-group-backreference-count.jsRepository: oven-sh/WebKit
Length of output: 645
Strengthen exception handling in expectInvalid to match codebase patterns
Line 8 lacks a SyntaxError type check, and Line 13 throws a string instead of an Error object. These deviate from the consistent error-handling pattern used throughout JSTests/stress tests, where exceptions are typed and reported as Error objects.
Suggested fix
} catch (e) {
- if (e.message.includes("invalid backreference"))
+ if (e instanceof SyntaxError && /backreference/i.test(String(e.message)))
continue;
throw e;
}
- throw "Expected an exception for /" + pattern + "/" + mods;
+ throw new Error("Expected a SyntaxError for /" + pattern + "/" + mods);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/regexp-noncapturing-group-backreference-count.js` around lines
7 - 13, The catch in expectInvalid should verify the thrown exception is a
SyntaxError and contains "invalid backreference" before continuing, and the
final failure should throw an Error object instead of a string; update the catch
block in expectInvalid to check for (e instanceof SyntaxError &&
e.message.includes("invalid backreference")) and replace the final throw
"Expected an exception for /" + pattern + "/" + mods with throw new
Error("Expected an exception for /" + pattern + "/" + mods) so errors follow the
test-suite's typed Error convention.
| @@ -1,3 +1,4 @@ | |||
| //@ skip if $hostOS == "windows" || $hostOS == "linux" | |||
There was a problem hiding this comment.
Platform skip is too broad and drops high-value coverage.
Skipping the entire test on Linux/Windows hides regressions in parsing/canonicalization assertions that are not host-timezone-dependent. Please narrow the skip to only truly platform-variant assertions (or split those into a separate guarded test).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@JSTests/stress/temporal-timezone.js` at line 1, The test currently skips
entirely via the top-line directive "//@ skip if $hostOS == "windows" || $hostOS
== "linux"", which is too broad; instead restrict skipping to only the
platform-dependent assertions by either (a) removing the global skip and
wrapping the host-specific checks in conditional guards that check $hostOS
(e.g., around the parsing/canonicalization assertions), or (b) split those
host-variant assertions into a separate test file with its own "//@ skip if ..."
directive; update the file Temporal-timezone.js accordingly so
non-host-dependent tests still run while only the truly platform-dependent
assertions remain skipped.
| <button id="button1">Button 1</button> | ||
| <button id="button2">Button 2</button> | ||
| </div> | ||
| <button id="outside">Outside</button> |
There was a problem hiding this comment.
Set explicit type="button" on all test buttons.
This clears the lint warnings and prevents default-submit semantics from leaking in if markup evolves.
Proposed fix
- <button id="button1">Button 1</button>
- <button id="button2">Button 2</button>
+ <button type="button" id="button1">Button 1</button>
+ <button type="button" id="button2">Button 2</button>
</div>
-<button id="outside">Outside</button>
+<button type="button" id="outside">Outside</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button id="button1">Button 1</button> | |
| <button id="button2">Button 2</button> | |
| </div> | |
| <button id="outside">Outside</button> | |
| <button type="button" id="button1">Button 1</button> | |
| <button type="button" id="button2">Button 2</button> | |
| </div> | |
| <button type="button" id="outside">Outside</button> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 11-11: The type attribute must be present on elements.
(button-type-require)
[warning] 12-12: The type attribute must be present on
elements.(button-type-require)
[warning] 14-14: The type attribute must be present on
elements.(button-type-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/accessibility/aria-hidden-focus-override.html` around lines 11 -
14, The buttons in this test use implicit button elements which can default to
type="submit" in forms; update the three elements with ids button1, button2, and
outside to include an explicit attribute type="button" to silence lint warnings
and avoid accidental submit behavior if the markup changes. Locate the <button
id="button1">, <button id="button2"> and <button id="outside"> elements and add
type="button" to each tag.
| </div> | ||
|
|
||
| <div class="webkit-box"> | ||
| <img src="data:image/svg+xml,<svg width='300' height='100' xmlns='http://www.w3.org/2000/svg'></svg>" |
There was a problem hiding this comment.
Add explicit accessibility/type attributes to avoid avoidable lint noise.
Line 41 (<img>), Line 50 (<iframe>), and Line 54 (<button>) are missing alt, title, and type respectively.
Suggested patch
- <img src="data:image/svg+xml,<svg width='300' height='100' xmlns='http://www.w3.org/2000/svg'></svg>"
+ <img alt=""
+ src="data:image/svg+xml,<svg width='300' height='100' xmlns='http://www.w3.org/2000/svg'></svg>"
data-expected-width="50">
- <iframe data-expected-width="50"></iframe>
+ <iframe title="legacy flexbox shrink test frame" data-expected-width="50"></iframe>
- <button data-expected-width="50">XXXXXXX</button>
+ <button type="button" data-expected-width="50">XXXXXXX</button>Also applies to: 50-50, 54-54
🧰 Tools
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/flexbox/deprecated-flex-box-writing-mode-shrink.html` at
line 41, The markup is missing explicit accessibility/type attributes which
causes lint noise: add an alt attribute to the <img> in the test (e.g.,
alt="placeholder image" or alt="decorative image" if purely decorative), add a
title attribute to the <iframe> (e.g., title="test iframe") to satisfy
accessibility requirements, and add an explicit type attribute to the <button>
(e.g., type="button") to avoid implicit submit behavior; update the <img>,
<iframe>, and <button> elements accordingly in the file.
| "imported": { | ||
| "w3c": { | ||
| "web-platform-tests": { | ||
| "css": { | ||
| "css-backgrounds": { | ||
| "box-shadow-radius-generated.html?width=300&height=60&spread=30&radius=0px%200px%2030px%2030px": { | ||
| "reftest_type": [ | ||
| "==" | ||
| ], | ||
| "report": "REGRESSION", | ||
| "expected": "PASS", | ||
| "actual": "IMAGE", | ||
| "image_diff_percent": 0.28308165, | ||
| "image_difference": { | ||
| "max_difference": 255, | ||
| "total_pixels": 1792 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
from pathlib import Path
path = Path("LayoutTests/fast/harness/full_results.json")
text = path.read_text(encoding="utf-8")
reported = len(re.findall(r'"report"\s*:\s*"REGRESSION"', text))
m = re.search(r'"num_regressions"\s*:\s*(\d+)', text)
num_regressions = int(m.group(1)) if m else None
print(f'report=\"REGRESSION\" entries: {reported}')
print(f'num_regressions field: {num_regressions}')
print("MATCH" if reported == num_regressions else "MISMATCH")
PYRepository: oven-sh/WebKit
Length of output: 124
Update num_regressions to match actual REGRESSION entry count.
The fixture contains 10 "report": "REGRESSION" entries but num_regressions is set to 7. This internal inconsistency will cause assertion failures in any parser or summary logic that validates count correctness. Update num_regressions to 10.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/harness/full_results.json` around lines 184 - 206, The JSON
summary's num_regressions field is inconsistent with the actual number of
"report": "REGRESSION" entries; update the num_regressions value to 10 so it
matches the ten REGRESSION entries (search for the "num_regressions" key and
verify against occurrences of "report": "REGRESSION" in the JSON before saving).
| @@ -0,0 +1,143 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
p = Path("LayoutTests/fast/selectors/has-invalidation-traversal-size.html")
s = p.read_text()
print("has <html lang=...>:", bool(re.search(r"<html\\b[^>]*\\blang\\s*=", s, re.I)))
print("has <title>...</title>:", bool(re.search(r"<title>.*?</title>", s, re.I|re.S)))
print("script open tags:", len(re.findall(r"<script\\b", s, re.I)))
print("script close tags:", len(re.findall(r"</script>", s, re.I)))
PYRepository: oven-sh/WebKit
Length of output: 156
🏁 Script executed:
cat -n LayoutTests/fast/selectors/has-invalidation-traversal-size.html | head -50Repository: oven-sh/WebKit
Length of output: 2145
🏁 Script executed:
tail -20 LayoutTests/fast/selectors/has-invalidation-traversal-size.htmlRepository: oven-sh/WebKit
Length of output: 706
🏁 Script executed:
grep -n "<script\|</script" LayoutTests/fast/selectors/has-invalidation-traversal-size.htmlRepository: oven-sh/WebKit
Length of output: 123
Fix HTML structure errors (lang/title/extra closing script tag).
Line 2 is missing lang, the <head> section lacks <title>, and line 141 has an unmatched </script>. Keep the test document valid to avoid parser/lint noise.
Proposed fix
-<html>
+<html lang="en">
<head>
+<title>:has() invalidation traversal size</title>
<style>
@@
</script>
-</script>
</body>
</html>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <html> | |
| <html lang="en"> | |
| <head> | |
| <title>:has() invalidation traversal size</title> | |
| <style> | |
| ... | |
| </script> | |
| </body> | |
| </html> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 2-2: An lang attribute must be present on elements.
(html-lang-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LayoutTests/fast/selectors/has-invalidation-traversal-size.html` at line 2,
The HTML file has structural issues: add a lang attribute to the <html> element
(e.g., <html lang="en">), create a proper <head> section containing a <title>
element, and remove or pair the unmatched closing </script> tag (the stray
</script> near the end) so all <script> tags are balanced; ensure the document
has a valid <head> and matching script tags to eliminate parser/lint errors.
Preview Builds
|
… ucal_getIanaTimeZoneID on Bun macOS importModule's new attribute extraction (407d0fe) calls ScriptFetchParameters::create(Type) which loses the original with.type string when parseType() returns HostDefined for Bun's custom loaders (toml/text/file/etc). Look the string back up from the attributes map for that case. IntlObject: U_ICU_VERSION_MAJOR_NUM is 74 from the bundled headers, but Bun on macOS links against system libicucore whose version follows the deployment target. Disable the ucal_getIanaTimeZoneID branch under OS(DARWIN) && USE(BUN_JSC_ADDITIONS); falls through to ucal_getCanonicalTimeZoneID.
…for the host hook
retrieveImportAttributesFromDynamicImportOptions: skip the
supportedImportAttributes throw under BUN_JSC_ADDITIONS — Node (and Bun)
accept arbitrary keys; the host's importModuleDynamically callback decides.
ScriptFetchParameters: add m_attributes (UncheckedKeyHashMap) so the host
hook can reconstruct the original {with} object.
JSModuleLoader::importModule: populate fetchParams->setAttributes() with the
full map (creating fetchParams even when type is None but attributes is not
empty).
…ters The 3-arg WasmBoundsCheck form added for memory64 generates a Branch64 of (ptrPlusImm < pointer) to detect wrap-around. For Int32 pointers (memory32) this is both unnecessary — Move32 zero-extends so the 64-bit sum cannot overflow — and unsafe: B3 leaves Int32 Tmp upper bits undefined, so the 64-bit compare can spuriously fire. This caused OOB traps in OMG-tier code when running in BoundsChecking mode (Windows, or useWasmFastMemory=false), e.g. with @electric-sql/pglite. Restrict the overflow guard to ptr->type() == Int64 and add a regression test that passes a wrapped i64 with dirty high bits as a memory32 index.
Merges upstream WebKit main @ aac4aed (565 commits since d550dd3).
Conflict resolution policy:
#include "Foo.h"quote-style for JSC headers (reject upstream's<JavaScriptCore/Foo.h>angle form)USE(BUN_JSC_ADDITIONS)blocksENABLE_OFFLINE_ASM_ALT_ENTRYkeeps(OS(DARWIN) && USE(BUN_JSC_ADDITIONS))on the newPLATFORM(COCOA)formAdopted upstream removals:
jsCast<T*>/jsDynamicCast<T*>→uncheckedDowncast<T>/dynamicDowncast<T>— migrated all Bun-only call sitesJSScriptFetcher/JSScriptFetchParametersremoved —loadModuleSyncsignature changed toRefPtr<ScriptFetchParameters>&&, RefPtr<ScriptFetcher>&&JSScriptFetcherType/JSScriptFetchParametersTyperemoved;EmbedderArrayLikeTypeaddedBuild fixes:
QueuedTask/InternalMicrotaskfor the sync-queue Bun additionsJSCJSValueInlines.hbeforeHeapSubspaceTypes.h(unified-source bundle shift from BunV8HeapSnapshotBuilder.cpp)TopExceptionScope.h(same)jsCast<JSGlobalObject*>(init.owner)(new API static_asserts against same-type casts)Bun-side changes required (in oven-sh/bun WebKit#29393):
JSType.zig: remove 2 entries, renumber 23+ down by 2ZigGlobalObject.cpp/ModuleLoader.cpp: update moduleLoader hook signatures toRefPtr<>formsrc/bun.js/bindings/**/*.cpp:jsCast<T*>→uncheckedDowncast<T>migration