Photo settings#2
Closed
jyavenard wants to merge 2 commits into
Closed
Conversation
https://bugs.webkit.org/show_bug.cgi?id=262466 rdar://116322614 Reviewed by NOBODY (OOPS!). Add ImageCapture.getPhotoSettings and implement support in MockRealtimeVideoSource and AVVideoCaptureSource. * LayoutTests/fast/mediastream/image-capture-get-photo-settings-expected.txt: Added. * LayoutTests/fast/mediastream/image-capture-get-photo-settings.html: Added. * Source/WebCore/DerivedSources.make: * Source/WebCore/Modules/mediastream/ImageCapture.cpp: (WebCore::ImageCapture::getPhotoSettings): * Source/WebCore/Modules/mediastream/ImageCapture.h: * Source/WebCore/Modules/mediastream/ImageCapture.idl: * Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp: (WebCore::MediaStreamTrack::getPhotoSettings const): * Source/WebCore/Modules/mediastream/MediaStreamTrack.h: * Source/WebCore/Modules/mediastream/PhotoSettings.idl: Added. * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp: (WebCore::MediaStreamTrackPrivate::getPhotoSettings): * Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h: * Source/WebCore/platform/mediastream/PhotoSettings.h: Added. * Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp: (WebCore::RealtimeMediaSource::getPhotoSettings): * Source/WebCore/platform/mediastream/RealtimeMediaSource.h: (WebCore::PhotoSettingsOrError::PhotoSettingsOrError): (WebCore::PhotoSettingsOrError::operator bool const): * Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h: * Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm: (WebCore::toFillLightMode): (WebCore::AVVideoCaptureSource::getPhotoSettings): * Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp: (WebCore::MockRealtimeVideoSource::getPhotoSettings): (WebCore::MockRealtimeVideoSource::settingsDidChange): * Source/WebCore/platform/mock/MockRealtimeVideoSource.h: * Source/WebKit/Scripts/webkit/messages.py: (headers_for_type): * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: * Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp: (WebKit::UserMediaCaptureManagerProxy::SourceProxy::getPhotoSettings): (WebKit::UserMediaCaptureManagerProxy::getPhotoSettings): * Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.h: * Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.messages.in: * Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.cpp: (WebKit::RemoteRealtimeMediaSource::getPhotoSettings): * Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.h: * Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSourceProxy.cpp: (WebKit::RemoteRealtimeMediaSourceProxy::getPhotoSettings): * Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSourceProxy.h:
a4bf884 to
2e98e8e
Compare
2e98e8e to
e3abeff
Compare
eric-carlson
pushed a commit
that referenced
this pull request
May 3, 2024
…/setrequestheader-case-insensitive.htm is a constant failure (attempt #2) https://bugs.webkit.org/show_bug.cgi?id=273498 rdar://127299045 Reviewed by Anne van Kesteren and Sam Sneddon. Second attempt. This change modifies the test such that it now only compares the relevant header substrings, instead of matching the entire header content. * LayoutTests/imported/w3c/web-platform-tests/xhr/setrequestheader-case-insensitive-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/setrequestheader-case-insensitive.htm: Canonical link: https://commits.webkit.org/278282@main
eric-carlson
pushed a commit
that referenced
this pull request
May 13, 2024
…n site-isolation rdar://127515199 https://bugs.webkit.org/show_bug.cgi?id=273715 Unreviewed test gardening. * LayoutTests/platform/mac-site-isolation/TestExpectations: Canonical link: https://commits.webkit.org/278367@main
eric-carlson
pushed a commit
that referenced
this pull request
Jun 14, 2024
…volume scrubber on a video player https://bugs.webkit.org/show_bug.cgi?id=275469 <rdar://129080145> Reviewed by Antti Koivisto. 1. In EventHandler::mouseDragged we dispatch the "mouse move" event 2. JS triggers some mutation which makes the tree dirty 3. later in EventHandler::handleMouseMoveEvent() we call EventHandler::handleMouseDraggedEvent() (tree is dirty) which, through a few layers of functions calls VisiblePosition::canonicalPosition() 4. VisiblePosition::canonicalPosition() needs a clean tree so it calls Document::updateLayout() which is turn destroys some renderers (see #2) 5. In-between EventHandler::handleMouseDraggedEvent() and VisiblePosition::canonicalPosition(), we CheckPtr a renderer which gets destroyed at WebKit#4. The fix (what we normally do with cases like this) is to make sure we clean the tree before entering VisiblePosition. * Source/WebCore/page/EventHandler.cpp: (WebCore::EventHandler::handleMouseDraggedEvent): Canonical link: https://commits.webkit.org/280013@main
eric-carlson
pushed a commit
that referenced
this pull request
Jul 11, 2024
…terpolate https://bugs.webkit.org/show_bug.cgi?id=275993 rdar://130704075 Reviewed by Matt Woodrow. We had three separate issues that would lead us to visually animate when one of the values in a given interval is a non-invertible matrix: 1. The method that determines whether it's possible to interpolate between two `transform` values would only account for `matrix()` values and not `matrix3d()`. 2. The `transform` property animation wrapper would not implement the `canInterpolate()` method and would thus always indicate that two `transform` values could be interpolated. This caused CSS Transitions to run even when the values would not a discrete interpolation. 3. Even if we correctly determined that two `transform` values should yield discrete interpolation, we would delegate an accelerated animation to Core Animation and that animation's behavior would differ an visibly interpolate. In this patch, we fill all three issues. First, we introduce a new `TransformOperations::containsNonInvertibleMatrix()` method which will check whether a `matrix()` or `matrix3d()` value that is not invertible is contained in the list of transform operations. We now use this function in `TransformOperations::shouldFallBackToDiscreteAnimation()` to address issue #1. Then, we add a `canInterpolate()` implementation to `AcceleratedTransformOperationsPropertyWrapper` which calls in the now-correct `TransformOperations::shouldFallBackToDiscreteAnimation()` to address issue #2. Finally, we add a new flag on `BlendingKeyframes` to determine whether a keyframe contains a `transform` value with a non-invertible matrix and we consult that flag in `KeyframeEffect::canBeAccelerated()` to determine whether an animation should be delegated to Core Animation, addressing issue #3. We add new WPT tests to check the correct interpolation behavior of `transform` when a non-invertible `matrix3d()` value is used, that no CSS Transition can be started with such a value, and finally that no animation is visibly run to catch the Core Animation case. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-interpolation-007-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-interpolation-007.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-non-invertible-discrete-interpolation-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-non-invertible-discrete-interpolation-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-non-invertible-discrete-interpolation.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-non-invertible-no-transition-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-non-invertible-no-transition.html: Added. * Source/WebCore/animation/BlendingKeyframes.cpp: (WebCore::BlendingKeyframes::analyzeKeyframe): * Source/WebCore/animation/BlendingKeyframes.h: (WebCore::BlendingKeyframes::hasDiscreteTransformInterval const): * Source/WebCore/animation/CSSPropertyAnimation.cpp: * Source/WebCore/animation/KeyframeEffect.cpp: (WebCore::KeyframeEffect::canBeAccelerated const): * Source/WebCore/platform/graphics/transforms/TransformOperations.cpp: (WebCore::TransformOperations::containsNonInvertibleMatrix const): (WebCore::TransformOperations::shouldFallBackToDiscreteAnimation const): * Source/WebCore/platform/graphics/transforms/TransformOperations.h: Canonical link: https://commits.webkit.org/280466@main
eric-carlson
pushed a commit
that referenced
this pull request
Jul 11, 2024
https://bugs.webkit.org/show_bug.cgi?id=276288 Reviewed by Antti Koivisto. RenderSlider::computeIntrinsicLogicalWidths should check against _logical_ width value. * LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/flex-item-compressible-002-expected.txt: * Source/WebCore/rendering/RenderSlider.cpp: (WebCore::RenderSlider::computeIntrinsicLogicalWidths const): Canonical link: https://commits.webkit.org/280718@main
eric-carlson
pushed a commit
that referenced
this pull request
Aug 16, 2024
…text run https://bugs.webkit.org/show_bug.cgi?id=277716 rdar://133309470 Reviewed by Matthieu Dubet. This patch implements the processing of text-autospace: ideogram-alpha only within an element. We don't yet handle element boundaries here. Although we pass SpacingState context from one ComplexTextController to another, we do that here in a limited way, only for measuring text for layout and for painting. There are other places in code which this will be necessary, for example, for handling element boundaries. 1. During the construction of ComplexTextController, we call ::adjustGlyphsAndAdvances which already iterates through glyphs and adjust spacing for other reasons. Now we process each pair of characters related to these glyphs here, adding the spacing necessary before the "current" character. For that reason, the SpacingState stores information about the previous character of a run. We also save the measured spacing in a new parallel vector m_textAutoSpaceSpacings. At this phase we can only manipulate a glyph advance, however, for adding space "before" a glyph, we need to move the glyph to the logical right, which is done later on ::advance. 2. ComplexTextController::advance is called for both layout and painting, but during painting it has access to a GlyphBuffer and it add glyphs into it. We are introducing a new GlyphBuffer::add function that also takes the glyph's origin, so we can manipulate the origin as necessary by adding the previous calculated spacing. 3. Doing #1 and #2 is already enough for painting the extra spacing between relevant characters according to their classes. Howeverm the width measured during layout would be broken because IFC splits text content into inlineTextItem(s) and measure the width of each item independently. This means that we already have to handle SpacingState passing here, otherwise we are not able to handle spacing between characters on the boundary of different InlineTextItem. * LayoutTests/imported/w3c/web-platform-tests/css/css-text/text-autospace/text-autospace-ideogram-alpha-001-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-text/text-autospace/text-autospace-ideogram-alpha-001-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-text/text-autospace/text-autospace-ideogram-alpha-001.html: Added. * Source/WTF/wtf/text/CharacterProperties.h: (WTF::isPunctuation): (WTF::isOpeningPunctuation): (WTF::isClosingPunctuation): (WTF::isOfScriptType): (WTF::eastAsianWidth): (WTF::isEastAsianFullWidth): (WTF::isCJKSymbolOrPunctuation): * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/layout/formattingContexts/inline/InlineItemsBuilder.cpp: (WebCore::Layout::InlineItemsBuilder::computeInlineTextItemWidths): * Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp: (WebCore::Layout::TextUtil::width): * Source/WebCore/layout/formattingContexts/inline/text/TextUtil.h: (WebCore::Layout::TextUtil::width): * Source/WebCore/platform/graphics/ComplexTextController.cpp: (WebCore::ComplexTextController::ComplexTextController): (WebCore::ComplexTextController::advance): (WebCore::ComplexTextController::adjustGlyphsAndAdvances): (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/ComplexTextController.h: (WebCore::ComplexTextController::ComplexTextRun::textAutospaceSize const): * Source/WebCore/platform/graphics/FontCascade.cpp: (WebCore::FontCascade::width const): (WebCore::FontCascade::codePath const): * Source/WebCore/platform/graphics/GlyphBuffer.h: (WebCore::GlyphBuffer::add): * Source/WebCore/platform/graphics/TextRun.cpp: * Source/WebCore/platform/graphics/TextRun.h: * Source/WebCore/platform/graphics/WidthCache.h: (WebCore::WidthCache::add): (WebCore::WidthCache::invalidateCacheForTextSpacing): * Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/skia/ComplexTextControllerSkia.cpp: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/text/TextSpacing.cpp: Added. (WebCore::TextAutospace::shouldApplySpacing const): (WebCore::TextAutospace::textAutospaceSize): (WebCore::TextSpacing::isIdeograph): (WebCore::TextSpacing::isNonIdeographicNumeral): (WebCore::TextSpacing::characterClass): * Source/WebCore/platform/text/TextSpacing.h: (WebCore::TextAutospace::hasIdeographAlpha const): (WebCore::TextAutospace::hasIdeographNumeric const): Canonical link: https://commits.webkit.org/282192@main
eric-carlson
pushed a commit
that referenced
this pull request
Aug 25, 2024
…text run https://bugs.webkit.org/show_bug.cgi?id=277716 rdar://133309470 Reviewed by Matthieu Dubet. We are relanding this patch as its first version was reverted due to performance reasons. On the current iteration we are avoiding classifying characters when not needed (text-autospace: no-autospace). We also won't keep the parralel vector for the added spacing in such a case. Original patch description: This patch implements the processing of text-autospace: ideogram-alpha only within an element. We don't yet handle element boundaries here. Although we pass SpacingState context from one ComplexTextController to another, we do that here in a limited way, only for measuring text for layout and for painting. There are other places in code which this will be necessary, for example, for handling element boundaries. 1. During the construction of ComplexTextController, we call ::adjustGlyphsAndAdvances which already iterates through glyphs and adjust spacing for other reasons. Now we process each pair of characters related to these glyphs here, adding the spacing necessary before the "current" character. For that reason, the SpacingState stores information about the previous character of a run. We also save the measured spacing in a new parallel vector m_textAutoSpaceSpacings. At this phase we can only manipulate a glyph advance, however, for adding space "before" a glyph, we need to move the glyph to the logical right, which is done later on ::advance. 2. ComplexTextController::advance is called for both layout and painting, but during painting it has access to a GlyphBuffer and it add glyphs into it. We are introducing a new GlyphBuffer::add function that also takes the glyph's origin, so we can manipulate the origin as necessary by adding the previous calculated spacing. 3. Doing #1 and #2 is already enough for painting the extra spacing between relevant characters according to their classes. Howeverm the width measured during layout would be broken because IFC splits text content into inlineTextItem(s) and measure the width of each item independently. This means that we already have to handle SpacingState passing here, otherwise we are not able to handle spacing between characters on the boundary of different InlineTextItem. * Source/WTF/wtf/text/CharacterProperties.h: (WTF::isPunctuation): (WTF::isOpeningPunctuation): (WTF::isClosingPunctuation): (WTF::isOfScriptType): (WTF::eastAsianWidth): (WTF::isEastAsianFullWidth): (WTF::isCJKSymbolOrPunctuation): * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/layout/formattingContexts/inline/InlineItemsBuilder.cpp: (WebCore::Layout::InlineItemsBuilder::computeInlineTextItemWidths): * Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp: (WebCore::Layout::TextUtil::width): * Source/WebCore/layout/formattingContexts/inline/text/TextUtil.h: (WebCore::Layout::TextUtil::width): * Source/WebCore/platform/graphics/ComplexTextController.cpp: (WebCore::ComplexTextController::ComplexTextController): (WebCore::ComplexTextController::advance): (WebCore::ComplexTextController::adjustGlyphsAndAdvances): (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/ComplexTextController.h: (WebCore::ComplexTextController::ComplexTextRun::textAutospaceSize const): * Source/WebCore/platform/graphics/FontCascade.cpp: (WebCore::FontCascade::width const): (WebCore::FontCascade::codePath const): * Source/WebCore/platform/graphics/GlyphBuffer.h: (WebCore::GlyphBuffer::add): * Source/WebCore/platform/graphics/TextRun.cpp: * Source/WebCore/platform/graphics/TextRun.h: * Source/WebCore/platform/graphics/WidthCache.h: (WebCore::WidthCache::add): (WebCore::WidthCache::invalidateCacheForTextSpacing): * Source/WebCore/platform/graphics/coretext/ComplexTextControllerCoreText.mm: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/graphics/skia/ComplexTextControllerSkia.cpp: (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun): * Source/WebCore/platform/text/TextSpacing.cpp: Added. (WebCore::TextAutospace::shouldApplySpacing const): (WebCore::TextAutospace::textAutospaceSize): (WebCore::TextSpacing::isIdeograph): (WebCore::TextSpacing::isNonIdeographicNumeral): (WebCore::TextSpacing::characterClass): * Source/WebCore/platform/text/TextSpacing.h: (WebCore::TextAutospace::hasIdeographAlpha const): (WebCore::TextAutospace::hasIdeographNumeric const): Canonical link: https://commits.webkit.org/282511@main
eric-carlson
pushed a commit
that referenced
this pull request
Sep 27, 2024
…r_ overflow https://bugs.webkit.org/show_bug.cgi?id=279486 Reviewed by Antti Koivisto. Let's call 1. lastHyphenPosition when we are dealing with the non-overflowing runs (this is when we can't break the overflowing part of the content and try to break runs _before_ the overflowing point). Since these runs are not overflowing, we should simply pick the last hyphenation position. 2. firstHyphenPosition when even the first hyphenation would produce overflowing content (e.g. minimum-content with computation) 3. hyphenPositionBefore when dealing with normal overflowing breaking (neither #1 nor #2) * LayoutTests/fast/inline/overflowing-content-with-hypens-expected.html: Added. * LayoutTests/fast/inline/overflowing-content-with-hypens.html: Added. * Source/WebCore/layout/formattingContexts/inline/InlineContentBreaker.cpp: (WebCore::Layout::firstTextRunIndex): (WebCore::Layout::InlineContentBreaker::processOverflowingContent const): (WebCore::Layout::limitBeforeValue): (WebCore::Layout::limitAfterValue): (WebCore::Layout::hasEnoughContentForHyphenation): (WebCore::Layout::firstHyphenPosition): (WebCore::Layout::lastHyphenPosition): (WebCore::Layout::hyphenPositionBefore): (WebCore::Layout::InlineContentBreaker::tryBreakingTextRun const): (WebCore::Layout::InlineContentBreaker::tryHyphenationAcrossOverflowingInlineTextItems const): (WebCore::Layout::hyphenPosition): Deleted. Canonical link: https://commits.webkit.org/283528@main
eric-carlson
pushed a commit
that referenced
this pull request
Sep 27, 2024
…ter follows to the same value https://bugs.webkit.org/show_bug.cgi?id=279570 rdar://135851156 Reviewed by Keith Miller. Let's consider the following FTL graph. BB#0 @0 = NewObject() Jump #1 BB#1 PutByOffset(@0, 0, @x) Jump #2 BB#2 ... @z = ... @1 = GetByOffset(@x, 0) Branch(@1, #3, WebKit#4) BB#3 PutByOffset(@0, 0, @z) Jump WebKit#5 BB#4 PutByOffset(@0, 0, @z) Jump WebKit#5 BB#5 Jump #2 Now, we would like to eliminate @0 object allocation. And we are computing SSA for pointers of fields of the that object which gets eliminated. Consider about @x's fields' SSA. PutByOffset becomes Def and GetByOffset becomes Use. And the same field will get the same SSA variable. So we first puts Defs and compute Phis based on that. In ObjectAllocationSinking phase, we had a fast path when the both SSA variable is following to the same value. Let's see BB#5. Because BB#3 and BB#4 defines Defs, dominance frontier BB#5 will need to introduce Phi. But interestingly, both SSA variable is following to the same @z. As a result, we were not inserting Phi for this case. But this is wrong. Inserted Phi is a Def, and based on that, we will further introduce Phis with that. If we omit inserting Phi in BB#5, we will not insert Phi into BB#2 while BB#2 will merge BB#1's Def And BB#5's Phi's Def. As a result, in BB#2, we think this variable is following to BB#1's Def. But that's wrong and BB#5's Phi exists. This patch removes this fast path to fix the issue. * JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js: Added. (Queue): (Queue.prototype.enqueue): (Queue.prototype.dequeue): (i.queue.dequeue): * Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp: Canonical link: https://commits.webkit.org/283558@main
eric-carlson
pushed a commit
that referenced
this pull request
Nov 12, 2024
https://bugs.webkit.org/show_bug.cgi?id=282741 rdar://139412312 Reviewed by Youenn Fablet. Pass colorspace information to the created CVPixelBuffer. We add utility methods to construct the colorspace data from the vpcC box and VPx bytestream should the information not be provided on construction. We prefer colorspace information from this source order given colorspace > description data (vpcC) > inband bytestream. Added tests verifying that black are pure black, and yellow are almost pure yellow with 601 videos and video range. Technically they should be exactly pure (255, 255, 0), however, compression artifacts with the source makes it not so. Fly-by #1: m_isClosed can be accessed concurrently on the decoder's or caller's workqueue. Make it atomic. Fly-by #2: Make relevant members const and add annotation about where some members can be accessed from. * LayoutTests/media/content/test-h264-601-videorange.mp4: Added. * LayoutTests/media/content/test-vp8-601-videorange.webm: Added. * LayoutTests/media/content/test-vp9-601-videorange.webm: Added. * LayoutTests/media/media-source/media-source-vp8-hiddenframes.html: We can reduce the fuzz range now that both the VT decoder (mac) will return the same colours as VideoDecoder (ios family) * LayoutTests/media/media-video-fullrange.html: Wait a maximum of 500ms for the promise to be resolved as the rVFC callback may not always be called. * LayoutTests/media/media-video-videorange-expected.txt: Added. * LayoutTests/media/media-video-videorange.html: Added. * LayoutTests/media/media-vp8-hiddenframes.html: We can reduce the fuzz range now that both the VT decoder (mac) will return the same colours as VideoDecoder (ios family) * LayoutTests/platform/mac-wk1/TestExpectations: * LayoutTests/platform/wpe/TestExpectations: * Source/WebCore/Modules/webcodecs/WebCodecsVideoDecoder.cpp: (WebCore::createVideoDecoderConfig): * Source/WebCore/platform/VideoDecoder.h: * Source/WebCore/platform/graphics/VP9Utilities.cpp: (WebCore::vPCodecConfigurationRecordFromVPXByteStream): (WebCore::convertToPlatformVideoColorPrimaries): (WebCore::convertToPlatformVideoTransferCharacteristics): (WebCore::convertToPlatformVideoMatrixCoefficients): (WebCore::colorSpaceFromVPCodecConfigurationRecord): (WebCore::vpcCFromVPXByteStream): Deleted. * Source/WebCore/platform/graphics/VP9Utilities.h: * Source/WebCore/platform/graphics/cocoa/CMUtilities.h: * Source/WebCore/platform/graphics/cocoa/CMUtilities.mm: (WebCore::convertToCMTransferFunction): Add transfer value for BT601 (smpte170m) which is the same as 709.2 transfer. (WebCore::attachColorSpaceToPixelBuffer): * Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.mm: Move methods to VP9Utilities.cpp (WebCore::convertToMatrixCoefficients): (WebCore::createVideoInfoFromVPCodecConfigurationRecord): (WebCore::convertToPlatformVideoColorPrimaries): Deleted. (WebCore::convertToPlatformVideoTransferCharacteristics): Deleted. (WebCore::convertToPlatformVideoMatrixCoefficients): Deleted. * Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h: * Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm: (WebCore::WebCoreDecompressionSession::decodeSampleInternal): Retrieve colorspace from CMSampleBuffer and give it to the decoder initialization. (WebCore::WebCoreDecompressionSession::enqueueDecodedSample): Fly-by: the last video frame in a webm doesn't have a duration. A logic error would have caused to never notify the listener that the frame at currentTime had been decoded, leading to the play() promise to never be resolved (nor rVFC callback to be called) (WebCore::WebCoreDecompressionSession::initializeVideoDecoder): * Source/WebCore/platform/libwebrtc/LibWebRTCVPXVideoDecoder.cpp: (WebCore::LibWebRTCVPXInternalVideoDecoder::decode): (WebCore::LibWebRTCVPXInternalVideoDecoder::LibWebRTCVPXInternalVideoDecoder): (WebCore::LibWebRTCVPXInternalVideoDecoder::createPixelBuffer): (WebCore::LibWebRTCVPXInternalVideoDecoder::Decoded): Canonical link: https://commits.webkit.org/286474@main
eric-carlson
pushed a commit
that referenced
this pull request
Dec 10, 2024
…layout dependent state https://bugs.webkit.org/show_bug.cgi?id=283395 Reviewed by Antti Koivisto. There are isSkippedContentRoot functions atm. 1. WebCore::isSkippedContentRoot(style, element) 2. and RenderObject::isSkippedContentRoot (see ContentVisibilityForceLayoutScope, for cases when we need to look inside c-v subtrees for geometry) and returns false when we are supposed to ignore content-visibility. This is always scoped to a layout frame (as opposed to painting, hittesting etc) The codebase is sprinkled with isSkippedContentRoot() calls, some of which exercise #1 while others call into #2 in a seemingly random fashion (e.g. even painting calls the "let's consult the ignore bit" variant). This patch replaces these 2 functions with 1. LocalFrameViewLayoutContext::isSkippedContentRootForLayout() 2. WebCore::isSkippedContentRoot(renderer) Where during layout we call layoutContext().isSkippedContentRootForLayout() (surprisingly small number) and the rest simply calls #2. (Note, there's a highly specific, 3rd use case in StyleAdjuster, which should be moved out to a place where we could use the WebCore::isSkippedContentRoot(renderer) variant). * Source/WebCore/dom/Document.cpp: (WebCore::CallbackForContainIntrinsicSize): (WebCore::Document::caretPositionFromPoint): * Source/WebCore/editing/TextIterator.cpp: (WebCore::TextIterator::advance): * Source/WebCore/page/LocalFrameViewLayoutContext.cpp: (WebCore::LocalFrameViewLayoutContext::isSkippedContentForLayout const): (WebCore::LocalFrameViewLayoutContext::isSkippedContentRootForLayout const): * Source/WebCore/page/LocalFrameViewLayoutContext.h: * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::simplifiedLayout): (WebCore::RenderBlock::layoutPositionedObject): (WebCore::RenderBlock::paintContents): (WebCore::RenderBlock::adjustBorderBoxRectForPainting): (WebCore::RenderBlock::paintRectToClipOutFromBorder): (WebCore::RenderBlock::paintExcludedChildrenInBorder): * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::layoutBlockChildren): * Source/WebCore/rendering/RenderBox.cpp: (WebCore::RenderBox::foregroundIsKnownToBeOpaqueInRect const): (WebCore::RenderBox::explicitIntrinsicInnerWidth const): (WebCore::RenderBox::explicitIntrinsicInnerHeight const): * Source/WebCore/rendering/RenderElement.cpp: (WebCore::RenderElement::styleWillChange): (WebCore::RenderElement::layoutIfNeeded): (WebCore::RenderElement::isSkippedContentRoot const): Deleted. * Source/WebCore/rendering/RenderElement.h: (WebCore::RenderObject::isSkippedContentRoot const): Deleted. * Source/WebCore/rendering/RenderElementInlines.h: (WebCore::RenderElement::shouldApplyInlineSizeContainment const): (WebCore::RenderElement::shouldApplySizeContainment const): (WebCore::RenderElement::shouldApplySizeOrInlineSizeContainment const): (WebCore::isSkippedContentRoot): * Source/WebCore/rendering/RenderGrid.cpp: (WebCore::RenderGrid::layoutPositionedObject): * Source/WebCore/rendering/RenderObject.cpp: (WebCore::RenderObject::isSkippedContentForLayout const): Deleted. * Source/WebCore/rendering/RenderObject.h: * Source/WebCore/rendering/RenderObjectInlines.h: (WebCore::RenderObject::layoutContext const): * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::RenderReplaced::paint): * Source/WebCore/rendering/RenderWidget.cpp: (WebCore::RenderWidget::paint): * Source/WebCore/rendering/style/RenderStyle.h: * Source/WebCore/rendering/style/RenderStyleInlines.h: (WebCore::doesSizeContainmentApplyByStyle): (WebCore::isSkippedContentRoot): Deleted. * Source/WebCore/rendering/updating/RenderTreeUpdater.cpp: (WebCore::RenderTreeUpdater::updateElementRenderer): * Source/WebCore/style/StyleAdjuster.cpp: (WebCore::Style::Adjuster::adjust const): Canonical link: https://commits.webkit.org/286858@main
eric-carlson
pushed a commit
that referenced
this pull request
Feb 3, 2025
…in-size-replaced-003*.html tests are constantly crashing. https://bugs.webkit.org/show_bug.cgi?id=286168# <rdar://problem/143150963> Reviewed by Antti Koivisto. 1. when a replaced element's intrinsic size changes we invalidate the associated IFC object 2. setNeedsLayoutIfNeededAfterIntrinsicSizeChange (as the name implies) is supposed to mark the renderer dirty only when its intrinsic size changed but the logic seems to have evolved to cover no-size-change cases as well. When other, unrelated changes trigger partial layout on IFC, we may skip this dirty renderer (#2). (e.g. video is on the first line, while partial layout start at the second line). Coming back from such partial layouts we may find some renderers still dirty -> ASSERT. This patch covers cases when base class decides to dirty video renderers. * LayoutTests/platform/ios/TestExpectations: * Source/WebCore/rendering/RenderVideo.cpp: (WebCore::RenderVideo::imageChanged): Canonical link: https://commits.webkit.org/289342@main
eric-carlson
pushed a commit
that referenced
this pull request
Feb 13, 2025
…pector rdar://98891055 https://bugs.webkit.org/show_bug.cgi?id=283092 Reviewed by Ryosuke Niwa and BJ Burg. There currently exists a message WebInspectorUIProxy::OpenLocalInspectorFrontend, which the web process sends to the UI process to show Web Inspector for the current web page. This introduces security risks as a compromised website may find its way to send arbitrary messages to the UI process, opening Web Inspector and weakening the web content sandbox. The reason this message exists is because there are useful ways the web process needs to open Web Inspector with initiative. Normally, Web Inspector is opened via one of the Develop menu's items, which is controlled by the UI process. However, Web Inspector can also be opened without being prompted by the UI process first, in these places: 1. In a web page's context menu, the "Inspect Element" option 2. Inside Web Inspector, if the Debug UI is enabled, on the top right corner, a button to open inspector^2 3. In WebKitTestRunner, via the TestRunner::showWebInspector function This patch makes it so that web process can no longer send a message to a UI process to open Web Inspector. This means web process cannot open Web Inspector at will -- it must be either due to the UI process's demand, or it's in one of the above three cases. More details below. I have tested that this change preserves the above three special cases and does prevent the web page from opening Web Inspector at will. - Cases #1 and #2 can be tested from the UI. - Case #3 can be tested with a WebKit test involving Web Inspector. I ran the test LayoutTests/inspector/console/js-completions.html, where I saw the test crashing without special treatment for this case. - To verify that the web page can't open Web Inspector, I followed the reproduction steps from the Radar and saw Web Inspector no longer opens, and opening the external URL also failed as expected. * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.messages.in: * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h: * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp: (WebKit::WebInspectorUIProxy::connect): - If the UI process wants to open Web Inspector, it sends a WebInspector::Show command to the web process. This patch makes that command take an async reply, so that the anticipated WebInspectorUIProxy::OpenLocalInspectorFrontend message from the web process can now be delivered through that async reply instead. This ensures that OpenLocalInspectorFrontend can only be done when initiated from the UI process (due to user interaction). (WebKit::WebInspectorUIProxy::markAsUnderTest): (WebKit::WebInspectorUIProxy::openLocalInspectorFrontend): (WebKit::WebInspectorUIProxy::closeFrontendPageAndWindow): - To avoid relying on the web process for potentially sensitive parameters, I reworked and removed the canAttach and underTest arguments from openLocalInspectorFrontend. These two values are now stored and managed in the UI process instead, instead of being passed from the web process all the time. - For canAttach, I noticed that the WebInspectorUIProxyMac::platformCanAttach method already implements the same logic as the web process's WebInspector::canAttachWindow. I filed https://webkit.org/b/283435 as a follow-up to clean up the webProcessCanAttach parameter, the canAttachWindow function in the web process, and potentially the m_attached field too, which all become obsolete due to this change. - I couldn't figure out what the `if (m_attached)` in canAttachWindow check does, and to me it had no effect, as this function is not called while inspector is open. - For underTest, I'm now letting the test runner directly set the flag on the WebInspectorUIProxy, as part of my fix to address case #3 from above. (WebKit::WebInspectorUIProxy::showConsole): (WebKit::WebInspectorUIProxy::showResources): (WebKit::WebInspectorUIProxy::showMainResourceForFrame): (WebKit::WebInspectorUIProxy::togglePageProfiling): - As the web process can longer call OpenLocalInspectorFrontend, call show/connect/openLocalInspectorFrontend here in the UI process instead. (WebKit::WebInspectorUIProxy::requestOpenLocalInspectorFrontend): - To preserve the open inspector^2 button (case #2 from above), we still maintain this message, but we ignore it unless it's for opening inspector^2, thus renaming the message as a request. This is all assuming that the Web Inspector is not a compromised web process, so we allow that message from it to come through. * Source/WebKit/WebProcess/Inspector/WebInspector.messages.in: * Source/WebKit/WebProcess/Inspector/WebInspector.h: * Source/WebKit/WebProcess/Inspector/WebInspector.cpp: (WebKit::WebInspector::show): - The Show message now takes an async reply, which is used to replace sending WebInspectorUIProxy::OpenLocalInspectorFrontend later. (WebKit::WebInspector::showConsole): (WebKit::WebInspector::showResources): (WebKit::WebInspector::showMainResourceForFrame): (WebKit::WebInspector::startPageProfiling): (WebKit::WebInspector::stopPageProfiling): - Calling inspectorController()->show() no longer does anything, since it's now the UI process's job to show Web Inspector first, for these functions to merely switch to the appropriate tabs. * Source/WebKit/WebProcess/Inspector/WebInspector.cpp: (WebKit::WebInspector::openLocalInspectorFrontend): * Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp: (WebKit::WebInspectorClient::openLocalFrontend): - Adapt to the command's reworked version. - This is maintained to allow the opening of inspector^2 from the web process (case #2 from above). For opening inspector^1, this message will be ignored by the UI process. * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::contextMenuItemSelected): - When the "Inspect Element" context menu item is selected (case #1 from above), since the web process may not be privileged to open Web Inspector, handle the showing of inspector here in UI process. * Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp: (WTR::TestRunner::showWebInspector): * Tools/WebKitTestRunner/TestInvocation.cpp: (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): * Source/WebKit/UIProcess/API/C/WKPagePrivate.h: * Source/WebKit/UIProcess/API/C/WKPage.cpp: (WKPageShowWebInspectorForTesting): - Preserve letting the WebKitTestRunner open Web Inspector (case #3 from above). - Adapt to the change that we now also let the UI process know about the underTest flag for case #3, rather than letting UI process rely on the value reported by the web process. * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h: * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp: (WKBundlePageShowInspectorForTest): Deleted. - No longer used due to my special fix for case #3. Originally-landed-as: 283286.537@safari-7620-branch (694a9b5). rdar://144667626 Canonical link: https://commits.webkit.org/290260@main
eric-carlson
pushed a commit
that referenced
this pull request
Feb 22, 2025
https://bugs.webkit.org/show_bug.cgi?id=288102 rdar://145222010 Reviewed by Yusuke Suzuki. Added the notion of a string list to a parsed RegExp that is in the form of /^(?:break|case|which|do|for)/ with an optional trailing $. Such a RegExp will not backtrack and therefore we can streamline the code we emit for such a pattern. This change involves recognizing beginning of string anchored alternations of strings while parsing and then treating the generation of JIT code differently for these patterns. This includes changing how conditional branching works, specifically that instead of the "fall through on match" for each term, to a "jump on match" for the whole alternation. The current code generated for the "case" elternative is: 8:Term PatternCharacter checked-offset:(3) 'c' <156> 0x11381430c: add w1, w1, #2 <160> 0x113814310: cmp w1, w2 <164> 0x113814314: b.hi 0x113814444 -> <468> 10:Term PatternCharacter checked-offset:(4) 'c' <168> 0x113814318: sub x17, x0, WebKit#4 <172> 0x11381431c: ldr w17, [x17, x1] <176> 0x113814320: movz w16, #0x6163 <180> 0x113814324: movk w16, #0x6573, lsl WebKit#16 -> 0x65736163 <184> 0x113814328: cmp w17, w16 <188> 0x11381432c: b.ne 0x113814444 -> <468> 11:Term PatternCharacter checked-offset:(4) 'a' already handled 12:Term PatternCharacter checked-offset:(4) 's' already handled 13:Term PatternCharacter checked-offset:(4) 'e' already handled 14:NestedAlternativeNext minimum-size:(5),checked-offset:(5) <192> 0x113814330: movz x16, #0x4444 <196> 0x113814334: movk x16, #0x1381, lsl WebKit#16 <200> 0x113814338: movk x16, #0x8001, lsl WebKit#32 <204> 0x11381433c: movk x16, #0xc973, lsl WebKit#48 -> 0x113814444 JIT PC <208> 0x113814340: stur x16, [sp, WebKit#8] <212> 0x113814344: b 0x113814404 -> <404> With some additional backtracking code: 9:NestedAlternativeNext minimum-size:(4),checked-offset:(4) <468> 0x113814444: sub w1, w1, #2 <472> 0x113814448: b 0x113814348 -> <216> With this change, the processing of "case" becomes: 9:StringListAlternativeNext minimum-size:(4),checked-offset:(4) <132> 0x12a8285c4: sub w1, w1, #1 <136> 0x12a8285c8: cmp w1, w2 <140> 0x12a8285cc: b.hi 0x12a8285e8 -> <168> 10:Term PatternCharacter checked-offset:(4) 'c' <144> 0x12a8285d0: sub x17, x0, WebKit#4 <148> 0x12a8285d4: ldr w17, [x17, x1] <152> 0x12a8285d8: movz w16, #0x6163 <156> 0x12a8285dc: movk w16, #0x6573, lsl WebKit#16 -> 0x65736163 <160> 0x12a8285e0: cmp w17, w16 <164> 0x12a8285e4: b.eq 0x12a82866c -> <300> 11:Term PatternCharacter checked-offset:(4) 'a' already handled 12:Term PatternCharacter checked-offset:(4) 's' already handled 13:Term PatternCharacter checked-offset:(4) 'e' already handled 14:StringListAlternativeNext minimum-size:(5),checked-offset:(5) With no backtracking code. We are able to eliminate one branch and the saving of the continuation PC for backtracking. The code size to process these string list RegExp is reduces. For the example RegExp above, the prior version created 1940 bytes (485 instructions) of code while the code created with this 1392 bytes (345 instructions) of code, a nearly 30% reduction in code. This change is a ~18% progression on the new regexp-keyword-parsing microbenchmark: Baseline YarrStringList regexp-keyword-parsing 136.7065+-0.9807 ^ 116.0161+-1.1791 ^ definitely 1.1783x faster <geometric> 136.7065+-0.9807 ^ 116.0161+-1.1791 ^ definitely 1.1783x faster * JSTests/microbenchmarks/regexp-keyword-parsing.js: Added. (arrayToString): (objectToString): (dumpValue): (compareArray): (compareGroups): (testRegExp): (testRegExpSyntaxError): (let.re.break.case.catch.continue.debugger.default.else.finally.if): (let.re1.break.case.catch.continue.debugger.default.else.finally.if): * JSTests/stress/regexp-parsing-tokens.js: Added. (arrayToString): (objectToString): (dumpValue): (compareArray): (compareGroups): (testRegExp): (testRegExpSyntaxError): * Source/JavaScriptCore/yarr/YarrJIT.cpp: * Source/JavaScriptCore/yarr/YarrPattern.cpp: (JSC::Yarr::YarrPatternConstructor::atomParenthesesEnd): (JSC::Yarr::YarrPatternConstructor::checkForTerminalParentheses): (JSC::Yarr::PatternAlternative::dump): (JSC::Yarr::PatternTerm::dump): * Source/JavaScriptCore/yarr/YarrPattern.h: (JSC::Yarr::PatternTerm::PatternTerm): (JSC::Yarr::PatternAlternative::PatternAlternative): Canonical link: https://commits.webkit.org/290791@main
eric-carlson
pushed a commit
that referenced
this pull request
Mar 3, 2025
https://bugs.webkit.org/show_bug.cgi?id=288102 rdar://145222010 Reviewed by Yusuke Suzuki. Added the notion of a string list to a parsed RegExp that is in the form of /^(?:break|case|which|do|for)/ with an optional trailing $. Such a RegExp will not backtrack and therefore we can streamline the code we emit for such a pattern. This change involves recognizing beginning of string anchored alternations of strings while parsing and then treating the generation of JIT code differently for these patterns. This includes changing how conditional branching works, specifically that instead of the "fall through on match" for each term, to a "jump on match" for the whole alternation. Fixed a bug in the original version where we weren't properly checking the nested alternatives to see if they only contain fixed single count PatternCharacter terms. The current code generated for the "case" elternative is: 8:Term PatternCharacter checked-offset:(3) 'c' <156> 0x11381430c: add w1, w1, #2 <160> 0x113814310: cmp w1, w2 <164> 0x113814314: b.hi 0x113814444 -> <468> 10:Term PatternCharacter checked-offset:(4) 'c' <168> 0x113814318: sub x17, x0, WebKit#4 <172> 0x11381431c: ldr w17, [x17, x1] <176> 0x113814320: movz w16, #0x6163 <180> 0x113814324: movk w16, #0x6573, lsl WebKit#16 -> 0x65736163 <184> 0x113814328: cmp w17, w16 <188> 0x11381432c: b.ne 0x113814444 -> <468> 11:Term PatternCharacter checked-offset:(4) 'a' already handled 12:Term PatternCharacter checked-offset:(4) 's' already handled 13:Term PatternCharacter checked-offset:(4) 'e' already handled 14:NestedAlternativeNext minimum-size:(5),checked-offset:(5) <192> 0x113814330: movz x16, #0x4444 <196> 0x113814334: movk x16, #0x1381, lsl WebKit#16 <200> 0x113814338: movk x16, #0x8001, lsl WebKit#32 <204> 0x11381433c: movk x16, #0xc973, lsl WebKit#48 -> 0x113814444 JIT PC <208> 0x113814340: stur x16, [sp, WebKit#8] <212> 0x113814344: b 0x113814404 -> <404> With some additional backtracking code: 9:NestedAlternativeNext minimum-size:(4),checked-offset:(4) <468> 0x113814444: sub w1, w1, #2 <472> 0x113814448: b 0x113814348 -> <216> With this change, the processing of "case" becomes: 9:StringListAlternativeNext minimum-size:(4),checked-offset:(4) <132> 0x12a8285c4: sub w1, w1, #1 <136> 0x12a8285c8: cmp w1, w2 <140> 0x12a8285cc: b.hi 0x12a8285e8 -> <168> 10:Term PatternCharacter checked-offset:(4) 'c' <144> 0x12a8285d0: sub x17, x0, WebKit#4 <148> 0x12a8285d4: ldr w17, [x17, x1] <152> 0x12a8285d8: movz w16, #0x6163 <156> 0x12a8285dc: movk w16, #0x6573, lsl WebKit#16 -> 0x65736163 <160> 0x12a8285e0: cmp w17, w16 <164> 0x12a8285e4: b.eq 0x12a82866c -> <300> 11:Term PatternCharacter checked-offset:(4) 'a' already handled 12:Term PatternCharacter checked-offset:(4) 's' already handled 13:Term PatternCharacter checked-offset:(4) 'e' already handled 14:StringListAlternativeNext minimum-size:(5),checked-offset:(5) With no backtracking code. We are able to eliminate one branch and the saving of the continuation PC for backtracking. The code size to process these string list RegExp is reduces. For the example RegExp above, the prior version created 1940 bytes (485 instructions) of code while the code created with this 1392 bytes (345 instructions) of code, a nearly 30% reduction in code. This change is a ~18% progression on the new regexp-keyword-parsing microbenchmark: Baseline YarrStringList regexp-keyword-parsing 136.7065+-0.9807 ^ 116.0161+-1.1791 ^ definitely 1.1783x faster <geometric> 136.7065+-0.9807 ^ 116.0161+-1.1791 ^ definitely 1.1783x faster * JSTests/microbenchmarks/regexp-keyword-parsing.js: Added. (arrayToString): (objectToString): (dumpValue): (compareArray): (compareGroups): (testRegExp): (testRegExpSyntaxError): (let.re.break.case.catch.continue.debugger.default.else.finally.if): (let.re1.break.case.catch.continue.debugger.default.else.finally.if): * JSTests/stress/regexp-parsing-tokens.js: Added. (arrayToString): (objectToString): (dumpValue): (compareArray): (compareGroups): (testRegExp): (testRegExpSyntaxError): * Source/JavaScriptCore/yarr/YarrJIT.cpp: * Source/JavaScriptCore/yarr/YarrPattern.cpp: (JSC::Yarr::YarrPatternConstructor::atomParenthesesEnd): (JSC::Yarr::YarrPatternConstructor::checkForTerminalParentheses): (JSC::Yarr::PatternAlternative::dump): (JSC::Yarr::PatternTerm::dump): * Source/JavaScriptCore/yarr/YarrPattern.h: (JSC::Yarr::PatternTerm::PatternTerm): (JSC::Yarr::PatternAlternative::PatternAlternative): Canonical link: https://commits.webkit.org/290982@main
eric-carlson
pushed a commit
that referenced
this pull request
Mar 3, 2025
https://bugs.webkit.org/show_bug.cgi?id=264576 rdar://114997939 Reviewed by BJ Burg. (This work was done in collaboration with Razvan and was based on his draft at WebKit@377f3e1.) This commit enables automatically inspecting and pausing the ServiceWorkerDebuggable. The idea is similar to the same functionalities with the JSContext/JSGlobalObjectDebuggable. The general flow is: 1. When the debuggable is first created, we optionally mark it as inspectable. 2. As soon as the debuggable is marked inspectable, its main thread (the thread that it was created on) gets blocked. 3. When the auto-launched Web Inspector frontend finishes initializing, it notifies the backend. - It's important for the debuggable to wait for this signal because a genuine auto-inspection must appear attached to the debuggable before it begins execution, respecting any breakpoints set early on in its script (where auto-pausing is basically a breakpoint before line 1). 4. The backend unpauses the blocked debuggable. If auto-pausing was requested, tell the debugger agent to pause. The service worker begins executing script unless its worker thread was specified to start in the WorkerThreadStartMode::WaitForInspector. During that waiting period, the worker thread can perform tasks sent into its debugging run loop, until it's signaled to stop waiting and continue to execute the script like normal. This commit makes use of that interface to make the service worker pause (when justified, i.e. developerExtrasEnabled) before running the above flow resembling auto-inspecting a JSContext. * Source/WebCore/workers/service/context/ServiceWorkerThread.cpp: (WebCore::threadStartModeFromSettings): (WebCore::ServiceWorkerThread::ServiceWorkerThread): - When there is potentially a remote inspector that would like to auto-inspect, make it so that the thread waits on start before executing its script. * Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h: * Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp: (WebCore::ServiceWorkerThreadProxy::ServiceWorkerThreadProxy): (WebCore::ServiceWorkerThreadProxy::threadStartedRunningDebuggerTasks): - Setting inspectability is step #1 in the above flow. - In step #2, calling `debuggable->setInspectable(true)` might block already, but we don't want that until the worker thread is setup and have the run loop be in debug mode, so we do that in a callback instead. - In step WebKit#4, when connection to the inspector completes or fails, the setInspectable call only returns then, so we unblock the worker thread to resume code execution. * Source/WebCore/inspector/agents/worker/WorkerDebuggerAgent.h: * Source/WebCore/inspector/WorkerInspectorController.h: * Source/WebCore/inspector/WorkerInspectorController.cpp: (WebCore::WorkerInspectorController::frontendInitialized): (WebCore::WorkerInspectorController::connectFrontend): (WebCore::WorkerInspectorController::disconnectFrontend): (WebCore::WorkerInspectorController::createLazyAgents): (WebCore::WorkerInspectorController::ensureDebuggerAgent): * Source/WebCore/workers/service/context/ServiceWorkerDebuggable.cpp: (WebCore::ServiceWorkerDebuggable::connect): * Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.h: * Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp: (WebCore::ServiceWorkerInspectorProxy::connectToWorker): - Mimic the logic for auto-inspecting a JSContext/JSGlobalObjectDebuggable. * Source/JavaScriptCore/inspector/protocol/Inspector.json: - Step #3 in the above flow, notify the backend when frontend completes setting up. * Source/WebCore/workers/service/context/ServiceWorkerDebuggable.h: - Allow service workers to be auto-inspected. (This is checked at https://github.com/rcaliman-apple/WebKit/blob/eng/Web-Inspector-Automatically-connect-Web-Inspector-to-ServiceWorker/Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.cpp#L95) * Source/WTF/wtf/PlatformEnableCocoa.h: - Add feature flag just in case. Canonical link: https://commits.webkit.org/291167@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 2, 2025
https://bugs.webkit.org/show_bug.cgi?id=290862 <rdar://147215658> Reviewed by Antti Koivisto. "Reusing block" type mutations (see RenderTreeBuilder::Inline::splitFlow) followed by float removal may lead to an unexpected state where we have a float to remove, but we have already destroyed m_floatingObjects, causing us to incorrectly assume that the float no longer belongs here (markSiblingsWithFloatsForLayout) and, therefore, does not need to be removed from sibling blocks (in case it is intrusive). What happens here is: 1. tree mutation makes an anon block reused (pre block) 2. a float is removed from said anon block's subtree At #1 we call removeFloatingObjects() which simply clears and destroys m_floatingObjects on the anon block. Now at #2, when we try to remove this float from sibling block containers by calling RenderBlockFlow::markSiblingsWithFloatsForLayout, and we consult m_floatingObjects to see if there's any float associated with the block and we early return as we had already cleared this set at #1. This patch ensures that when markSiblingsWithFloatsForLayout is called with a valid float, we always try to clean up sibling content. * LayoutTests/fast/block/float-remove-after-block-collapse-crash-expected.txt: Added. * LayoutTests/fast/block/float-remove-after-block-collapse-crash.html: Added. * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Change for (siblings) for (set items) to for (set items) for (siblings) so that the 'for (siblings)' logic can be moved to a lambda and used when there's a valid incoming float. Canonical link: https://commits.webkit.org/293094@main
webkit-commit-queue
pushed a commit
that referenced
this pull request
Apr 3, 2025
…n addFloatsToNewParent https://bugs.webkit.org/show_bug.cgi?id=290898 <rdar://143296265> Reviewed by Antti Koivisto. In this patch 1. we let m_floatingObjects go stale on the skipped root (we already do that for the skipped subtree by not running layout) 2. we descend into skipped subtrees while cleaning up floats even when m_floatingObjects is stale/empty Having up-to-date m_floatingObjects on the skipped root, while stale m_floatingObjects on the skipped subtree can lead to issues when (#1) a previously intrusive float (#2) becomes non-intrusive and (#3) eventually gets deleted prevents us from being able to cleanup m_floatingObjects in skipped subtree(s). at #1 m_floatingObjects is populated with the intrusive float (both skipped root and renderers in skipped subtree) and at #2 since we only run layout on the skipped root, m_floatingObjects gets updated by removing this previously intrusive float (skipped subtree becomes stale) and at #3 we don't descend into the skipped subtree to cleanup m_floatingObjects since the skipped root does not have this float anymore (removed at #2). * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Canonical link: https://commits.webkit.org/293119@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 28, 2025
…n addFloatsToNewParent https://bugs.webkit.org/show_bug.cgi?id=290898 <rdar://149579273> Reviewed by Antti Koivisto. In this patch 1. we let m_floatingObjects go stale on the skipped root (we already do that for the skipped subtree by not running layout) 2. we descend into skipped subtrees while cleaning up floats even when m_floatingObjects is stale/empty Having up-to-date m_floatingObjects on the skipped root, while stale m_floatingObjects on the skipped subtree can lead to issues when (#1) a previously intrusive float (#2) becomes non-intrusive and (#3) eventually gets deleted prevents us from being able to cleanup m_floatingObjects in skipped subtree(s). at #1 m_floatingObjects is populated with the intrusive float (both skipped root and renderers in skipped subtree) and at #2 since we only run layout on the skipped root, m_floatingObjects gets updated by removing this previously intrusive float (skipped subtree becomes stale) and at #3 we don't descend into the skipped subtree to cleanup m_floatingObjects since the skipped root does not have this float anymore (removed at #2). * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Canonical link: https://commits.webkit.org/293889@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 28, 2025
…rsor smoothly rdar://148762897 https://bugs.webkit.org/show_bug.cgi?id=292042 Reviewed by Devin Rousso. This is a combination of two issues: 1. If the WKInspectorWKWebView has safeAreaInsets set and therefore _obscuredContentInsets configured, adjusting its dimensions does not take that into account. Assuming there is a left inset configured to be X and the new docked inspector width requested by the frontend is W, the frontend is not aware of the extra X pixels needed when rendering the WKInspectorWKWebView (because the frontend lives inside that web view), which then needs to be (W + X) pixels wide instead. 2. The setAttachedWindowHeight/Width commands in the backend take an `unsigned int` as the new dimension's type, but the frontend computes in floating point (JavaScript's Number), which always gets rounded down when the new dimension gets passed in and coerced to an int. This makes the new view just slightly smaller than required over the course of dragging. * Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm: (WebKit::WebInspectorUIProxy::platformSetAttachedWindowHeight): (WebKit::WebInspectorUIProxy::platformSetAttachedWindowWidth): - Fix issue #1. * Source/WebInspectorUI/UserInterface/Base/Main.js: - Fix issue #2. - Adding the rounding on resulting dimension alone was sufficient to fix the "always-shrinking" bug. However, resizing still didn't feel completely smooth; the inspector wouldn't shrink by itself, but it still felt like having some lag following the moving cursor. I took a simpler approach to compute the delta, where we use the original dimension as the standard instead of the last frame, which turns out to be extremely smooth and exactly what we wanted. Tested the fix manually on various inspector zoom factors from 60% to 240%, combined with or without an _obscuredContentInsets applied on the WKInspectorWKWebView. Canonical link: https://commits.webkit.org/294123@main
eric-carlson
pushed a commit
that referenced
this pull request
May 28, 2025
…ngs' rdar://151332172 Reviewed by Jonathan Bedard and Ryosuke Niwa. * Source/WebKit/WebProcess/Model/WebModelPlayerProvider.cpp: Canonical link: https://commits.webkit.org/294927@main
eric-carlson
pushed a commit
that referenced
this pull request
May 28, 2025
https://bugs.webkit.org/show_bug.cgi?id=293337 rdar://151740794 Reviewed by Yijia Huang and Justin Michaud. The current MovHintRemoval's analysis looks weird. We should just do liveness analysis globally and use this information for MovHint removal. 1. "Use" is a node which may exit. When exit happens, we should keep all use of live locals at this bytecode exit location alive. 2. "Def" is MovHint. We kill the locals here. And doing fixpoint analysis and using this information to remove MovHint. Also, pruning Availability in OSRAvailabilityAnalysisPhase via bytecode liveness is wrong: they need to keep live nodes from DFG for example. 0: PutHint @x, PROP(@y) 1: OSR exit point #1 (here, loc0 is not alive) 2: -- Pruning happens -- 3: MovHint @x, loc0 4: OSR exit point #2 (here, loc0 is alive) In this case pruning point will remove (0)'s heap availability since @x is not alive from bytecode at (1), but this is wrong as we need this in (4). In this patch, we remove pruneByLiveness in DFGOSRAvailabilityAnalysisPhase. This pruning should happen by the user of DFGOSRAvailabilityAnalysisPhase instead, and it is already happening (see FTLLowerToB3's pruneByLiveness in exit site, which is right. And ObjectAllocationSinking is pruning with CombinedLiveness, this is right since it also accounts Node's liveness in addition to bytecode's liveness.). Let's just make availability just compute the availability for all things, and then we prune some of unnecessary ones at each use of this information. * Source/JavaScriptCore/dfg/DFGMovHintRemovalPhase.cpp: * Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp: (JSC::DFG::OSRAvailabilityAnalysisPhase::run): Canonical link: https://commits.webkit.org/295369@main
eric-carlson
pushed a commit
that referenced
this pull request
May 28, 2025
https://bugs.webkit.org/show_bug.cgi?id=293456 Reviewed by Antti Koivisto. 1. setPreferredLogicalWidthsDirty was introduced at 17615@main modeled after setNeedsLayout 2. As with setNeedsLayout, setPreferredLogicalWidthsDirty has the ability to only mark the current renderer dirty (as opposed to the default behavior of marking ancestors as well) 3. Initially (17615@main) MarkOnlyThis was passed in only on the RenderView (as it has no parent) 4. Later at 17621@main, MarkOnlyThis was used to fix a specific bug where an absolute positioned box with percent padding had incorrect size after viewport resize. Here is what happened there: RenderView RenderBlockFlow (html) RenderBlockFlow (body) RenderBlockFlow (absolute positioned box with % padding) - absolute positioned box uses shrink-to-fit sizing when width is auto. The final width is the combination of its min/max widths and the available space. - % padding is resolved against the containing block's width. Now with viewport resize, where the absolute positioned box's containing block is the RenderView - min/max _content_ values stay the same but - the viewport's new size affects the padding value so the box's final min/max values do change. Min/max values (aka preferred width) are cached on the renderers and we don't recompute them unless PreferredLogicalWidthsDirty bit is set to true (similar to needsLayout bit). We mark renderers dirty in two distinct places: #1 when content/style changes before layout or #2 during layout as we figure we need to invalidate more content In many cases (e.g. resize) in order to evaluate the extent of the damage up front and mark all affected renderers dirty would require a full tree walk, so instead we rely on layout to mark additional renderers dirty as needed. ...which is how we fixed the viewport resize bug in 17621@main. if (RelayoutChildren::Yes && renderer.preferredWidthDependsOnAvailableSpace()) renderer.setPreferredLogicalWidthsDirty(true, MarkOnlyThis) - check during layout if the current renderer's final width depends on the available space - if it does, mark the preferred widths dirty This ensures that by the time we get to computeLogicalWidth() -where we compute the final size of the renderer- preferredLogicalWidths bit is already dirty. It guarantees that we re-compute our min/max values, allowing the new padding value to be incorporated. Now consider a scenario where this positioned box has fixed width (e.g. width: 100px) - we still mark preferred widths dirty (we have to) but - in computeLogicalWidth(), we will not re-compute them as we simply take the fixed width (instead of running the shrink-to-fit logic) - and preferredWidths stays dirty So just because we have a box with preferredWidthDependsOnAvailableSpace(), it does not necessarily mean we run shrink-to-fit sizing and as a result we may not clear the dirty bit. ...and this is where setPreferredLogicalWidthsDirty differs from setNeedsLayout. Whenever we call setNeedsLayout(MarkOnlyThis), it is always followed by a layoutIfNeeded() call, clearing the needsLayout bit, while preferredWidths may remain dirty. While it is crucial that no needsLayout bit is set as returning from layout, preferredWidths bits can stay dirty throughout the entire lifetime of a renderer if they are never used. The reason why having a stuck dirty bit is problematic though, is because we rely on them when marking ancestor chain dirty. The default behavior of both needsLayout and preferredLogicalWidthsDirty is MarkContainingBlockChain (when a renderer changes it's likely that parent changes too). With MarkContainingBlockChain, we climb the ancestor chain and mark containers dirty, unless we find an already dirty container. This performance optimization helps to avoid to walk the ancestor chain every time a renderer is marked dirty, but it also assumes that if a renderer is dirty all its ancestors are dirty as well. So now JS mutates some style and we try to mark our ancestor chain dirty to let our containers know that at the subsequent layout they need to re-compute their min/max values if their sizing relies on them. ...but in setPreferredLogicalWidthsDirty, we bail out too early when we come across this stuck renderer and never mark the parents dirty. So maybe 17621@main should have picked MarkContainingBlockChain and not MarkOnlyThis to fully invalidate the ancestor chain. Before considering that let's take a look at how min/max values are used. In block layout we first visit the parent, compute its width and descend into the children and pass in the parent width as available space. If the parent's width depends on the size of the children (e.g. width: min-content), we simply ask the children for their min/max widths. There's a special "preferred width" codepath in our block layout implementation. This codepath computes min/max widths and caches them on the renderers. Important to note that this happens _before_ running layout on the child renderers. (this may sound like some form of circular dependency, but CSS spec is clear about how to resolve cases where child min/max widths depend on the final size of the parent width) What it means is by the time we run layout on a renderer, the parent may have already "forced" the renderer to re-compute the stale min/max widths. So now imagine we are in the renderer's layout code now and come across this line if (RelayoutChildren::Yes && renderer.preferredWidthDependsOnAvailableSpace()) renderer.setPreferredLogicalWidthsDirty(true, MarkOnlyThis) This makes us to re-compute the min/max values even if they are clean (and this is the result of not being able to effectively run invalidation up front, before layout) With MarkOnlyThis, the worst case scenario (beside the sticky bit bug) is that we may end up running min/max computation twice; first triggered by our parent followed by this line above. However, with MarkContainingBlockChain, we would keep re-computing valid and clean min/max values at every layout on the ancestors as well. (as ancestors would see their dirty min/max values at the next layout the first time and then they would re-compute them, followed by us marking them dirty again and so on) While MarkContainingBlockChain is normally what we do as changing min/max values on the child may affect the ancestors too, it is too late to use it at layout due to block layout's "preferred width first -> layout second" order. The fundamental issue here is that we can't tell if the renderer's min/max values got cleared in the current layout frame by ancestors running preferred with computation on their subtree. If we could, we would either 1, not call renderer.setPreferredLogicalWidthsDirty(true, MarkOnlyThis) at all i.e. min/max values are really really clear, so let's just reuse them 2, or call it by passing in MarkContainingBlockChain (and go with #1 at subsequent layout if applicable) TLDR; While it's okay for preferredWidths to stay dirty across layouts, when a renderer is dirty all its ancestors have to be dirty. Calling setPreferredLogicalWidthsDirty() with MarkOnlyThis during layout is also fine as long as we managed to clear it before finishing layout. Here let's just fix the sticky bit by making sure ancestor chain is always fully marked. - add a rare bit to indicate if we used MarkOnlyThis on this renderer - adjust the "if this renderer dirty its parent must be dirty" logic by consulting the MarkOnlyThis bit. * LayoutTests/fast/dynamic/percent-padding-with-shrink-to-fit-parent-expected.html: Added. * LayoutTests/fast/dynamic/percent-padding-with-shrink-to-fit-parent.html: Added. * Source/WebCore/rendering/RenderObject.cpp: (WebCore::RenderObject::setPreferredLogicalWidthsDirty): (WebCore::RenderObject::invalidateContainerPreferredLogicalWidths): * Source/WebCore/rendering/RenderObject.h: Canonical link: https://commits.webkit.org/295501@main
eric-carlson
pushed a commit
that referenced
this pull request
Jun 30, 2025
…eleted and re-entered into an input https://bugs.webkit.org/show_bug.cgi?id=294558 <rdar://problem/154094432> Reviewed by Antti Koivisto. 1. RenderTextControlSingleLine's inner renderer is vertically centered. 2. RenderTextControlSingleLine's inner renderer provides the baseline position for the input box. 3. The baseline position is passed to inline layout and is used to align other content on the line (when baseline alignment applies). As text is appended to the input box: 1. An empty RenderText is constructed and inserted into the tree. 2. InsertIntoTextNodeCommand::doApply() is called. 3. RenderText is populated by calling setText. In step #2, layout is prematurely run on the content (see passwordEchoEnabled) before it is populated in step #3. This layout generates an empty inline display content with a 0px-tall line box, which gets vertically centered. This "centered line" then becomes the baseline for the rest of the content. In step #3, another layout is run on the input box, this time with populated content, but the layout is limited to the input only, so adjacent content does not get alignment treatment. This is how the offset occurs (result of the premature layout). There are a few issues here: 1. We should not run layout on the empty content before RenderText is populated (webkit.org/b/294880). 2. RenderTextControlSingleLine should not center the inner renderer when it has no content (webkit.org/b/294881). 3. IFC should not report valid line boxes when there’s no content at all (i.e., when no display boxes are generated). This patch addresses issue #3. * LayoutTests/fast/editing/incorrect-baseline-for-input-adjacent-content-expected.html: Added. * LayoutTests/fast/editing/incorrect-baseline-for-input-adjacent-content.html: Added. * Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp: (WebCore::LayoutIntegration::LineLayout::firstLineBox const): (WebCore::LayoutIntegration::LineLayout::lastLineBox const): Canonical link: https://commits.webkit.org/296569@main
eric-carlson
pushed a commit
that referenced
this pull request
Aug 1, 2025
https://bugs.webkit.org/show_bug.cgi?id=296470 rdar://79416560 Reviewed by Tim Horton. Added support for tooltips on Catalyst. This includes: - Linking UIKitMacHelper - Declaring needed protocols and interfaces - Implementing toolTipChanged The bulk of the functionality is in -[WKContentView _toolTipChanged:], which creates a UIToolTipInteraction if it doesn't exist. If a UIToolTipInteraction exists, we update the UIToolTipInteraction with the tooltip's new string. Notice, _toolTipChanged's has a windowChangedKeyState call and uses a UINSSharedApplicationDelegate. This is for updating the tooltip after the initial creation. Implementing UIToolTipInteractionDelegate's toolTipInteraction:configurationAtPoint: is not enough to update the tooltip because... 1. UIView's addInteraction and removeInteraction are only for separate views. That is, if there are tooltips A and B on the same view, whichever tooltip is triggered first will stay no matter where you hover within the view. The interaction is not removed until you move your mouse to a different view. 2. There is no API to update (text or show/hide) the tooltip while it is displayed. To work around #2, we need to indirectly trigger AppKit's _displayToolTipIfNecessaryIgnoringTime, which is the logic to update the tooltip, but through UIKit. See below for more details. * Source/WTF/wtf/PlatformHave.h: * Source/WebKit/Configurations/WebKit.xcconfig: * Source/WebKit/Platform/spi/ios/UIKitSPI.h: These files above have necessary declarations that are used for working around #2, adding a HAVE macro, and linking UIKitMacHelper. * Source/WebKit/UIProcess/ios/PageClientImplIOS.mm: (WebKit::PageClientImpl::toolTipChanged): Calls -[WKContentView _toolTipChanged:], which contains the logic for supporting tooltips. * Source/WebKit/UIProcess/ios/WKContentViewInteraction.h: * Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm: (-[WKContentView _toolTipChanged:]): This is where the support is implemented. To circumvent #2, we first use UINSSharedApplicationDelegate, which is a UINSApplicationDelegate, then obtaining a UINSWindow. UINSWindow has a UINSSceneView that has a NSViewDynamicToolTipManager property. The NSViewDynamicToolTipManager can be used to call windowChangedKeyState. We call windowChangedKeyState to trigger a chain of function calls that will eventually trigger the update tooltip function. The chain is as follows: windowChangedKeyState calls _restartMovementTracking which calls _displayToolTipIfNecessaryIgnoringTime, which updates the tooltip. (-[WKContentView toolTipInteraction:configurationAtPoint:]): Canonical link: https://commits.webkit.org/297878@main
eric-carlson
pushed a commit
that referenced
this pull request
Aug 17, 2025
https://bugs.webkit.org/show_bug.cgi?id=296902 rdar://157510912 Reviewed by Yusuke Suzuki. Currently, we support both aligned allocations (e.g. bmalloc_allocate_with_alignment) and zeroed allocations (e.g. bmalloc_allocate_zeroed); however, we do not support simultaneously-aligned-and-zeroed allocations (e.g. bmalloc_allocate_zeroed_with_alignment). This patch implements those. The consumer of the API can just zero it themselves, but libpas is careful to optimize out that zeroing operation if it knows it’s not necessary, e.g. if the page was newly mmap’d (c.f. pas_allocation_result_zero). This comes up when allocating wasm memory, as we basically 1. Ask for a huge allocation 2. Mmap over it to ensure it’s zero This is probably not itself a huge performance problem, but it does show up when I tried to switch that #2 over to madvise(MADV_ZERO): normally this would be preferable because this subsequent mmap would fragment the backing vm-objects (and acquire more locks), but in the case that we’re just replacing the entire vm-object anyways the first downside goes away, after which presumably the actual effort of going page-by-page and making sure they’re zeroed begins to dominate. Creating this new bmalloc_allocate_zeroed_with_alignment family of functions will thus allow us to avoid that unnecessary mmap and unblock migrating it to use madvise(MADV_ZERO) instead. Canonical link: https://commits.webkit.org/298428@main
eric-carlson
pushed a commit
that referenced
this pull request
Aug 24, 2025
https://bugs.webkit.org/show_bug.cgi?id=297724 rdar://157024791 Reviewed by Antti Koivisto. 1. out-of-flow boxes participate first in in-flow layout as if they were in-flow boxes where we compute their static position. This static position becomes their final position when inset (left, right, top, bottom) is auto. 2. as a second step, as we reach the out-of-flow box's containing block we run layout again and compute the final position (this might just be what we computed at #1 in case of auto inset) Now we mark the out-of-flow box dirty at #1 and expect #2 to clear the box by moving it to its final position. However in case of subtree layout where the layout root has an out-of-flow descendant while the containing block is an ancestor of the layout root, #2 will never happen (we bail out of layout before reaching the containing block). "setNeedsLayout" was added at 254969@main to mimic what legacy line layout did. However starting from 262470@main, we already move the box at #1 meaning that #2 does not need to happen if the box is statically positioned only. (If the out-of-flow box was not-statically positioned, subtree layout would not start "below" its containing block) * LayoutTests/TestExpectations: * Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp: (WebCore::LayoutIntegration::LineLayout::updateRenderTreePositions): Canonical link: https://commits.webkit.org/299021@main
webkit-commit-queue
pushed a commit
that referenced
this pull request
Sep 26, 2025
https://bugs.webkit.org/show_bug.cgi?id=299504 rdar://161294228 Reviewed by Yijia Huang. Previous one 300327@main worked for the same basic block's load, but it didn't work for the load in dominators. This patch updates CSE rules to make immutable load elimination work with dominators' load. Like, BB#0 @0: Load(@x, immutable) @1: CCall(...) # potentially clobber everything Branch ... #1, #2 BB#1 @2: CCall(...) # potentially clobber everything Jump #3 BB#2 @3: CCall(...) # potentially clobber everything Jump #3 BB#3 @4: Load(@x, immutable) ... Then @4 should be replaced with Identity(@0) as dominator BB#0 is having immutable load @0 matching to @4. Tests: Source/JavaScriptCore/b3/testb3_1.cpp Source/JavaScriptCore/b3/testb3_8.cpp * Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp: * Source/JavaScriptCore/b3/testb3.h: * Source/JavaScriptCore/b3/testb3_1.cpp: (run): * Source/JavaScriptCore/b3/testb3_8.cpp: (testLoadImmutableDominated): (testLoadImmutableNonDominated): Canonical link: https://commits.webkit.org/300562@main
eric-carlson
pushed a commit
that referenced
this pull request
Oct 15, 2025
…on-in-child.html is failing https://bugs.webkit.org/show_bug.cgi?id=299628 rdar://161203486 Reviewed by Basuke Suzuki. Suppose we have a mainframe and an iframe and this series of navigations happens: 1. iframe fragment navigates to "/#a" 2. main frame fragment navigates to "/#1" 3. main frame fragment navigates to "/#2" 4. main frame fragment navigates to "/#3" 5. iframe goes back 6. iframe fragment navigates to "/#b" After Step 5, the UI Process b/f list should be: A) mainframe - URL - ItemID A ** iframe - URL - ItemID A B) mainframe - URL - ItemID B ** iframe - URL/#a - ItemID B C) mainframe - URL/#1 - ItemID C ** iframe - URL/#a - ItemID C D) mainframe - URL/#2 - ItemID D ** iframe - URL/#a - ItemID D E) mainframe - URL/#3 - ItemID E ** iframe - URL/#a - ItemID E The mainframe's Navigation object's m_entries should be: A) mainframe - URL - ItemID A C) mainframe - URL/#1 - ItemID C D) mainframe - URL/#2 - ItemID D E) mainframe - URL/#3 - ItemID E <--- current index The iframe's Navigation object's m_entries should be: A) ** iframe - URL - ItemID A <--- current index E) ** iframe - URL/#a - ItemID E According to this layout test, after Step 6: The mainframe's Navigation object's m_entries should be: A) mainframe - URL - ItemID A <--- current index The iframe's Navigation object's m_entries should be: A) ** iframe - URL - ItemID A F) ** iframe - URL/#b - ItemID F <--- current index So when a subframe has a PUSH same-document navigation and disposes of any forward entries, any parent frame must do the same. This test was failing because the parent frame was not disposing of its forward entries. To fix this, we add a new function to recusively dispose of all forward entries in any parent frames when a subframe has a PUSH same-document navigation. We use the ItemID to determine what entry must stay and then dispose of any entries that come after that one. * LayoutTests/imported/w3c/web-platform-tests/navigation-api/per-entry-events/dispose-for-navigation-in-child-expected.txt: * Source/WebCore/page/LocalDOMWindowProperty.cpp: (WebCore::LocalDOMWindowProperty::protectedFrame const): * Source/WebCore/page/LocalDOMWindowProperty.h: * Source/WebCore/page/Navigation.cpp: (WebCore::Navigation::updateNavigationEntry): (WebCore::Navigation::disposeOfForwardEntriesInParents): Call recursivelyDisposeOfForwardEntriesInParents on the main frame, which will traverse down the frame tree, and for each frame until we reach the subframe that actually navigated, dispose of any forward entries. (WebCore::Navigation::recursivelyDisposeOfForwardEntriesInParents): (WebCore::Navigation::updateForNavigation): This is called for same-document navigations. If it's a PUSH navigation, call disposeOfForwardEntriesInParents. The ItemID that we keep in these parent frames is the current ItemID right before this PUSH operation happens. * Source/WebCore/page/Navigation.h: Canonical link: https://commits.webkit.org/300721@main
eric-carlson
pushed a commit
that referenced
this pull request
Oct 29, 2025
…Observer and MediaSessionObserver <https://bugs.webkit.org/show_bug.cgi?id=301393> <rdar://problem/163402233> Unreviewed internal build fix. See 302140@main and 302145@main. * Source/WebKit/Platform/ios/PlaybackSessionInterfaceLMK.mm: (WebKit::nowPlayingMetadataObserver): - Add missing namespace qualifier. Canonical link: https://commits.webkit.org/302147@main
eric-carlson
pushed a commit
that referenced
this pull request
Oct 29, 2025
…files <https://bugs.webkit.org/show_bug.cgi?id=301376> <rdar://problem/163297495> Unreviewed build fix for watchOS. Follow-up fix for 302143@main and 302150@main. * Tools/TestWebKitAPI/Tests/WebCore/cocoa/CaptionPreferencesTests.mm: (TestWebKitAPI::CaptionPreferenceTests::showMenuAndThen): - Make use of USE(UICONTEXTMENU) to exclude code on watchOS. Canonical link: https://commits.webkit.org/302152@main
eric-carlson
pushed a commit
that referenced
this pull request
Feb 27, 2026
…ning inline boxes https://bugs.webkit.org/show_bug.cgi?id=308696 Reviewed by Antti Koivisto. 1. 303091@main: we started creating empty (inline box) display boxes to be able to provided offsetTop/offsetHeight values for cases like <div> content<br> <span id=empty-inline-box></span> </div> where before the fix, the trailing <span> did not initiate any display boxes (empty line). 2. However after 303091@main, we also started creating empty display boxes for _line_spanning_ inline boxes when we couldn't fit any content on the line due to intrusive floats. <div> content <float><span> this does not fit the line due to the float here</span> </div> and this turned out to be not web-compatible. 3. 307916@main and 308175@main addressed this issue by filtering the display box rects as we collect them in RenderInline's generateLineBoxRects (this is called by functions like offsetHeight etc. Let's fix #2 and undo #3. * LayoutTests/fast/inline/empty-inline-box-bounding-rect.html: * Source/WebCore/layout/formattingContexts/inline/display/InlineDisplayContentBuilder.cpp: (WebCore::Layout::InlineDisplayContentBuilder::processNonBidiContent): * Source/WebCore/rendering/RenderInline.cpp: (WebCore::RenderInline::generateLineBoxRects const): Canonical link: https://commits.webkit.org/308289@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 1, 2026
https://bugs.webkit.org/show_bug.cgi?id=310082 rdar://172722059 Reviewed by Dan Hecht. Previously, there were two distinct behaviors for this macro on Darwin. 1. PAS_ASSERT with two or more arguments would store the __LINE__ and subsequent arguments in registers, then properly execute `brk 0xc471` to crash. 2. PAS_ASSERT with one argument would do none of that, and fall through to __builtin_unreachable() on the assumption that it would be implemented as a trap. The actual benefit of #2 seems to be minimal, if anything, while having the downside of obfuscating crash logs (among other things). Barring some horrific unforseen perf regression, this seems like the obvious thing to do. Here's some example asm before/after (in this case, pas_segregated_page_switch_lock_slow) before: ``` JavaScriptCore`pas_segregated_page_switch_lock_slow: 0x104686b70 <+0>: pacibsp 0x104686b74 <+4>: sub sp, sp, #0x30 0x104686b78 <+8>: stp x20, x19, [sp, #0x10] 0x104686b7c <+12>: stp x29, x30, [sp, #0x20] 0x104686b80 <+16>: add x29, sp, #0x20 0x104686b84 <+20>: str x1, [sp, #0x8] 0x104686b88 <+24>: cmp x1, x2 0x104686b8c <+28>: b.eq 0x104686c20 ; <+176> [inlined] pas_assertion_failed at pas_utils.h:248:5 0x104686b90 <+32>: mov x20, x2 0x104686b94 <+36>: mov x19, x0 0x104686b98 <+40>: cbz x1, 0x104686bbc ; <+76> [inlined] os_unfair_lock_trylock_inline at lock_private.h:784:20 0x104686b9c <+44>: mrs x8, TPIDRRO_EL0 ... 0x104686c0c <+156>: ldr x0, [sp, #0x8] 0x104686c10 <+160>: ldp x29, x30, [sp, #0x20] 0x104686c14 <+164>: ldp x20, x19, [sp, #0x10] 0x104686c18 <+168>: add sp, sp, #0x30 0x104686c1c <+172>: retab -> 0x104686c20 <+176>: brk #0x1 ``` after: ``` JavaScriptCore`pas_segregated_page_switch_lock_slow: 0x1045d688c <+0>: pacibsp 0x1045d6890 <+4>: sub sp, sp, #0x30 0x1045d6894 <+8>: stp x20, x19, [sp, #0x10] 0x1045d6898 <+12>: stp x29, x30, [sp, #0x20] 0x1045d689c <+16>: add x29, sp, #0x20 0x1045d68a0 <+20>: str x1, [sp, #0x8] 0x1045d68a4 <+24>: cmp x1, x2 0x1045d68a8 <+28>: b.eq 0x1045d693c ; <+176> [inlined] pas_assertion_failed_noreturn_silencer0 at pas_utils.h:314:5 0x1045d68ac <+32>: mov x20, x2 0x1045d68b0 <+36>: mov x19, x0 0x1045d68b4 <+40>: cbz x1, 0x1045d68d8 ; <+76> [inlined] os_unfair_lock_trylock_inline at lock_private.h:784:20 0x1045d68b8 <+44>: mrs x8, TPIDRRO_EL0 ... 0x1045d6928 <+156>: ldr x0, [sp, #0x8] 0x1045d692c <+160>: ldp x29, x30, [sp, #0x20] 0x1045d6930 <+164>: ldp x20, x19, [sp, #0x10] 0x1045d6934 <+168>: add sp, sp, #0x30 0x1045d6938 <+172>: retab -> 0x1045d693c <+176>: bl 0x104732c60 ; set_up_range.cold.16 at pas_designated_intrinsic_heap.c ``` The only inline impact is that the `brk` is replaced with a `bl` to out-of-line code that handles the actual register fiddling. Unfortunately, this does require obviating some of 309224@main, as these non-inline functions fall afoul of TAPI's checks for symbol-availability, as it'd be libpas.a that contains the symbol and not bmalloc per se. There are ways to get around that but they're pretty disruptive, so for now we'll have to go without __LINE__ information for BAssert. This isn't a regression since BAssert was only wired up to the single-argument PAS_ASSERT, which as we've seen, did not actually implement that desired behavior. Canonical link: https://commits.webkit.org/309669@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 16, 2026
…o fit-content https://bugs.webkit.org/show_bug.cgi?id=242837 <rdar://problem/97492632> Reviewed by Antti Koivisto. Given: <div style="position: absolute; inset: 0; height: fit-content"> <div style="height: 100%"> <div style="height: 80px"></div> </div> </div> The out-of-flow container is sized to 0px, while the correct height is 80px. availableLogicalHeightForPercentageComputation has a condition that decides if an out-of-flow element's height is definite for its children's percentage resolution. It has two branches: 1. height is specified (length/percent/calc) -> definite 2. both insets are set -> definite is always definite and can be used to resolve the percent height. However with content-dependent values like fit-content, answering "how tall is the parent?" requires laying out the children first - but the child is asking precisely because it needs that answer to size itself. That's a cyclic dependency, so the answer should be "indefinite" and the percentage falls back to auto. The fix restricts #2 to height:auto. For intrinsic keywords, availableLogicalHeightForPercentageComputation now returns nullopt and the child's percentage falls back to auto. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/abspos-intrinsic-height-inset-percentage-child-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/abspos-intrinsic-height-inset-percentage-child.html: Added. * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::availableLogicalHeightForPercentageComputation const): Canonical link: https://commits.webkit.org/311375@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 20, 2026
https://bugs.webkit.org/show_bug.cgi?id=312485 rdar://174932570 Reviewed by Yusuke Suzuki. Use value - trunc(value) == 0.0 for isInteger, which rejects NaN and Infinity without an explicit isFinite check since NaN - NaN and Inf - Inf both produce NaN. Add MacroAssembler::isDoubleInteger helper and use it in the DFG NumberIsInteger path, removing the manual exponent-bit extraction branch. Simplify the FTL NumberIsInteger path similarly using B3 IR. Remove redundant isFinite checks from DFG and FTL NumberIsSafeInteger, since the range check against maxSafeInteger already rejects Infinity. On ARM64 the difference in codegen is roughly: isIntegerOld(double): frintz d1, d0 fmov x8, d0 mov w9, WebKit#2047 ubfx x8, x8, WebKit#52, WebKit#11 fcmp d1, d0 ccmp x8, x9, #2, eq cset w0, lo ret isIntegerNew(double): frintz d1, d0 fsub d0, d0, d1 fcmp d0, #0.0 cset w0, eq ret No new tests, no behavior change. Covered by existing tests. Canonical link: https://commits.webkit.org/311413@main
webkit-commit-queue
pushed a commit
that referenced
this pull request
Apr 28, 2026
…rsInlines.h into separate UnifiedBuild sources https://bugs.webkit.org/show_bug.cgi?id=313363 rdar://175635833 Reviewed by Geoffrey Garen. Source files that include expensive headers are now grouped together in unified source bundles, reducing the number of translation units that pay the parsing cost for those headers. Previously, files needing RenderStyle+GettersInlines.h (a 3.7s parse cost) were scattered across 190 unified source bundles. Most of those bundles also contained files that didn't need the header, but paid the cost anyway since unified sources share a single preprocessing context. By tagging these files and grouping them together, the header is now parsed in only 91 bundles instead of 190. Measured impact on WebCore clean build: Frontend parsing: 8018s -> 7351s (-667s, -8.3%) Backend/Codegen: 558s -> 542s (-16s, -2.8%) RenderStyle+GettersInlines.h: 730s -> 339s (-391s, -54%) RenderStyle+GettersInlines.h drops from the #2 most expensive WebCore header to... #3, even while it's parsing cost dropped by more than half. The implementation adds a general-purpose @Header:<name> attribute to the unified source generator. Files tagged with the same header group name are placed into their own unified source bundles, separate from untagged files. This prevents expensive headers from "leaking" into bundles that don't need them. The mechanism is extensible; other expensive headers can be targeted the same way by tagging their includers in Sources.txt with a new @Header:<name> group. No header refactoring or code changes are required; only the Sources.txt tagging and a bundle count parameter in generate-unified-sources.sh. Compensating #includes were added to several .cpp files that previously relied on transitive includes from other files in their unified source bundle. Regrouping exposed these missing direct includes. * Source/WTF/Scripts/generate-unified-source-bundles.py: (SourceFile.__init__): (parse_args): (main): * Source/WebCore/Scripts/generate-unified-sources.sh: * Source/WebCore/Sources.txt: * Source/WebCore/SourcesCocoa.txt: * Source/WebCore/UnifiedSources-output.xcfilelist: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/accessibility/AXCrossProcessSearch.cpp: * Source/WebCore/accessibility/AXLogger.cpp: * Source/WebCore/accessibility/AccessibilityScrollView.cpp: * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: * Source/WebCore/dom/DeviceMotionEvent.cpp: * Source/WebCore/dom/InternalObserverInspect.cpp: * Source/WebCore/editing/cocoa/AutofillElements.cpp: * Source/WebCore/history/CachedFrame.cpp: * Source/WebCore/html/ModelDocument.cpp: * Source/WebCore/inspector/agents/InspectorDOMAgent.cpp: * Source/WebCore/layout/integration/grid/LayoutIntegrationGridCoverage.cpp: (WebCore::LayoutIntegration::printLegacyGridReasons): (WebCore::LayoutIntegration::printTextForSubtree): Deleted. * Source/WebCore/page/FrameTree.cpp: * Source/WebCore/page/ImageAnalysisQueue.cpp: * Source/WebCore/page/IntelligenceTextEffectsSupport.cpp: * Source/WebCore/page/LocalFrame.cpp: * Source/WebCore/page/scrolling/ScrollLatchingController.cpp: * Source/WebCore/platform/animation/values/paths/AcceleratedEffectBoxPath.cpp: * Source/WebCore/platform/animation/values/paths/AcceleratedEffectBoxPath.h: * Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp: * Source/WebCore/platform/graphics/cg/GradientRendererCG.h: (WebCore::GradientRendererCG::createGradientBySampling): * Source/WebCore/rendering/RenderModel.cpp: * Source/WebCore/rendering/SubtreeScrollbarChangesState.cpp: * Source/WebCore/style/StyleLocalPropertyRegistry.cpp: Canonical link: https://commits.webkit.org/312184@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 29, 2026
https://bugs.webkit.org/show_bug.cgi?id=313498 rdar://175714384 Reviewed by Sihui Liu. In Document::isSecureContext, WebKit walks the frame tree to check if all of a frame's ancestors are "secure". It does this to gate access to powerful web APIs such as navigator.geolocation. For each ancestor, we call Document::isDocumentSecure which performs checks to see if the frame is potentially trustworthy. Below is the implementation. It does the following: 1. If the document is sandboxed, it checks if the document's URL is trustworthy 2. Otherwise, check if the document's security origin is trustworthy. ``` static inline bool isDocumentSecure(const Document& document) { if (document.isSandboxed(SandboxFlag::Origin)) return isURLPotentiallyTrustworthy(document.url()); return document.securityOrigin().isPotentiallyTrustworthy(); } ``` With site isolation enabled, it is possible for some of the document's ancestors to be RemoteFrames in different processes. Currently, the code in Document::isSecureContext, only handles LocalFrames and silently skips any RemoteFrame ancestors. This patch handles the RemoteFrame case by adding a fallback when an ancestor frame can't be cast to a LocalFrame. Since we can't get the document of the RemoteFrame, we can't call Document::isDocumentSecure like the LocalFrame case. Instead, this patch directly calls isPotentiallyTrustworthy on the RemoteFrame's security origin. This is #2 from the description of Document::isDocumentSecure earlier in this commit message. I chose to skip #1 since we don't have the full URL or sandbox flags of the remote frame. Also, only checking the RemoteFrame's security origin is more conservative since in the worst case we would treat a frame as insecure (and block requests) where the pre site isolation case would treat it as secure. This patches fixes imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html with site isolation enabled. * LayoutTests/platform/ios-site-isolation/TestExpectations: * LayoutTests/platform/mac-site-isolation/TestExpectations: * Source/WebCore/dom/Document.cpp: (WebCore::Document::isSecureContext const): Canonical link: https://commits.webkit.org/312199@main
eric-carlson
pushed a commit
that referenced
this pull request
Apr 29, 2026
…rsInlines.h into separate UnifiedBuild sources https://bugs.webkit.org/show_bug.cgi?id=313363 rdar://175635833 Reviewed by Geoffrey Garen. Source files that include expensive headers are now grouped together in unified source bundles, reducing the number of translation units that pay the parsing cost for those headers. Previously, files needing RenderStyle+GettersInlines.h (a 3.7s parse cost) were scattered across 190 unified source bundles. Most of those bundles also contained files that didn't need the header, but paid the cost anyway since unified sources share a single preprocessing context. By tagging these files and grouping them together, the header is now parsed in only 91 bundles instead of 190. Measured impact on WebCore clean build: Frontend parsing: 8018s -> 7351s (-667s, -8.3%) Backend/Codegen: 558s -> 542s (-16s, -2.8%) RenderStyle+GettersInlines.h: 730s -> 339s (-391s, -54%) RenderStyle+GettersInlines.h drops from the #2 most expensive WebCore header to... #3, even while it's parsing cost dropped by more than half. The implementation adds a general-purpose @Header:<name> attribute to the unified source generator. Files tagged with the same header group name are placed into their own unified source bundles, separate from untagged files. This prevents expensive headers from "leaking" into bundles that don't need them. The mechanism is extensible; other expensive headers can be targeted the same way by tagging their includers in Sources.txt with a new @Header:<name> group. No header refactoring or code changes are required; only the Sources.txt tagging and a bundle count parameter in generate-unified-sources.sh. Compensating #includes were added to several .cpp files that previously relied on transitive includes from other files in their unified source bundle. Regrouping exposed these missing direct includes. * Source/WTF/Scripts/generate-unified-source-bundles.py: (SourceFile.__init__): (parse_args): (main): * Source/WebCore/Scripts/generate-unified-sources.sh: * Source/WebCore/Sources.txt: * Source/WebCore/SourcesCocoa.txt: * Source/WebCore/UnifiedSources-output.xcfilelist: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/accessibility/AXCrossProcessSearch.cpp: * Source/WebCore/accessibility/AXLogger.cpp: * Source/WebCore/accessibility/AccessibilityScrollView.cpp: * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: * Source/WebCore/dom/DeviceMotionEvent.cpp: * Source/WebCore/dom/InternalObserverInspect.cpp: * Source/WebCore/editing/cocoa/AutofillElements.cpp: * Source/WebCore/history/CachedFrame.cpp: * Source/WebCore/html/ModelDocument.cpp: * Source/WebCore/inspector/agents/InspectorDOMAgent.cpp: * Source/WebCore/layout/integration/grid/LayoutIntegrationGridCoverage.cpp: (WebCore::LayoutIntegration::printLegacyGridReasons): (WebCore::LayoutIntegration::printTextForSubtree): Deleted. * Source/WebCore/page/FrameTree.cpp: * Source/WebCore/page/ImageAnalysisQueue.cpp: * Source/WebCore/page/IntelligenceTextEffectsSupport.cpp: * Source/WebCore/page/LocalFrame.cpp: * Source/WebCore/page/scrolling/ScrollLatchingController.cpp: * Source/WebCore/platform/animation/values/paths/AcceleratedEffectBoxPath.cpp: * Source/WebCore/platform/animation/values/paths/AcceleratedEffectBoxPath.h: * Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp: * Source/WebCore/platform/graphics/cg/GradientRendererCG.h: (WebCore::GradientRendererCG::createGradientBySampling): * Source/WebCore/rendering/RenderModel.cpp: * Source/WebCore/rendering/SubtreeScrollbarChangesState.cpp: * Source/WebCore/style/StyleLocalPropertyRegistry.cpp: Canonical link: https://commits.webkit.org/312240@main
eric-carlson
pushed a commit
that referenced
this pull request
May 1, 2026
…ScrollbarDoesNotAdaptToDarkMode is flaky failure https://bugs.webkit.org/show_bug.cgi?id=313768 <rdar://problem/175961655> Reviewed by Abrar Rahman Protyasha. Restore pre-312300@main behavior, by only applying the page's dark mode appearance to full-frame PDF scrollbars, not embedded ones. Before 312300@main For full-frame PDFs, the scrollbar appearance follows the system dark mode. For embedded PDFs, it does not - the scrollbar always uses the default appearance. Codeflow before 312300@main: return useDarkAppearance() || scrollbarOverlayStyle() == ScrollbarOverlayStyle::Light; (and with no useDarkAppearance() override on PDFPluginBase -where base class returns false) Full-frame PDF, light mode: 1. useDarkAppearance() -> no override, base class -> false 2. scrollbarOverlayStyle() -> Default (updateScrollbarOverlayStyle() set it to Default since page->useDarkAppearance() was false) 3. Default == Light -> false Result: false Full-frame PDF, dark mode: 1. useDarkAppearance() -> no override, base class -> false 2. scrollbarOverlayStyle() -> Light (updateScrollbarOverlayStyle() set it to Light since page->useDarkAppearance() was true) 3. Light == Light -> true Result: true Embedded PDF, light mode: 1. useDarkAppearance() -> no override, base class -> false 2. scrollbarOverlayStyle() -> Default (updateScrollbarOverlayStyle() at UnifiedPDFPlugin.mm bails out with if (!isFullMainFramePlugin()) return - never sets overlay style) 3. Default == Light -> false Result: false Embedded PDF, dark mode: 1. useDarkAppearance() -> no override, base class -> false 2. scrollbarOverlayStyle() -> Default (same reason as above) 3. Default == Light -> false Result: false Codeflow after this fix: Full-frame PDF, light mode: 1. useDarkAppearance() -> PDFPluginBase override -> page->useDarkAppearance() -> false 2. Falls through to check #2 3. scrollbarOverlayStyle() -> Default (updateScrollbarOverlayStyle() set it to Default since page->useDarkAppearance() was false) 4. Default == Light -> false Result: false Full-frame PDF, dark mode: 1. useDarkAppearance() -> PDFPluginBase override -> page->useDarkAppearance() -> true Result: true Embedded PDF, light mode: 1. useDarkAppearance() -> PDFPluginBase override -> base class ScrollableArea::useDarkAppearance() -> false 2. Falls through to check #2 3. scrollbarOverlayStyle() -> Default (updateScrollbarOverlayStyle() bails out with if (!isFullMainFramePlugin()) return -- never sets overlay style) 4. Default == Light -> false Result: false Embedded PDF, dark mode: 1. useDarkAppearance() -> PDFPluginBase override -> base class ScrollableArea::useDarkAppearance() -> false 2. Falls through to check #2 3. scrollbarOverlayStyle() -> Default (same reason as above) 4. Default == Light -> false Result: false All four cases match the behavior before 312300@main. The only difference is how full-frame dark mode works: before, it went through the overlay style path (scrollbarOverlayStyle was set to Light); now it goes through useDarkAppearance(). * Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.mm: (WebKit::PDFPluginBase::useDarkAppearance const): * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/UnifiedPDFTests.mm: (TestWebKitAPI::UNIFIED_PDF_TEST): Canonical link: https://commits.webkit.org/312409@main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
update to use NativePromise