Summary: | [IntersectionObserver] Factor out rect mapping and clipping logic from computeRectForRepaint | ||
---|---|---|---|
Product: | WebKit | Reporter: | Ali Juma <ajuma> |
Component: | Layout and Rendering | Assignee: | Ali Juma <ajuma> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bfulgham, commit-queue, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, zalan |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Local Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | |||
Bug Blocks: | 189624, 190899, 190905 | ||
Attachments: |
Description
Ali Juma
2018-09-21 08:40:56 PDT
Created attachment 350370 [details]
WIP for EWS
Comment on attachment 350370 [details] WIP for EWS Attachment 350370 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9297545 New failing tests: svg/custom/image-parent-translation.xhtml fast/repaint/selection-ruby-rl.html Created attachment 350375 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350370 [details] WIP for EWS Attachment 350370 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9297611 New failing tests: svg/custom/image-parent-translation.xhtml Created attachment 350377 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350370 [details] WIP for EWS Attachment 350370 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9297726 New failing tests: svg/custom/image-parent-translation.xhtml Created attachment 350390 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 350370 [details] WIP for EWS Attachment 350370 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9297938 New failing tests: svg/custom/image-parent-translation.xhtml fast/repaint/selection-ruby-rl.html Created attachment 350396 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350370 [details] WIP for EWS Attachment 350370 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9299550 New failing tests: fast/repaint/selection-ruby-rl.html svg/custom/image-parent-translation.xhtml Created attachment 350406 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 350814 [details]
WIP for EWS
Comment on attachment 350814 [details] WIP for EWS Attachment 350814 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9350713 New failing tests: fast/frames/lots-of-iframes.html imported/w3c/web-platform-tests/encoding/single-byte-decoder.html fast/frames/frame-limit.html Created attachment 350834 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 350864 [details]
Patch
Comment on attachment 350864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350864&action=review > Source/WebCore/rendering/RenderInline.cpp:915 > + // FIXME: Respect the value of context.m_applyCompositedContainerScrolls. We weren't calling shouldApplyCachedClipAndScrollPositionForRepaint() here before, though it seems like that was a bug since everywhere else we do call that method before calling applyCachedClipAndScrollPositionForRepaint(). But to avoid a behavior change in this patch, I've left this as a FIXME. Created attachment 351409 [details]
Patch
Rebased
Comment on attachment 351409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351409&action=review > Source/WebCore/rendering/RenderBox.cpp:973 > +bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const I think this needs a better name, or maybe shouldn't return a bool, since only some callers care about intersection. Perhaps the intersection-related stuff should be in a 'context' object pass as an argument? > Source/WebCore/rendering/RenderBox.cpp:2138 > +LayoutRect RenderBox::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const I think this is relative to the repaint container, not "absolute". The comments say that layoutState->paintOffset() is offset from the layout root, but I think it's probably the repaint root. Maybe just computeVisibleRectUsingPaintOffset() > Source/WebCore/rendering/RenderBox.cpp:2259 > + bool isEmpty = !containerBox.applyCachedClipAndScrollPosition(adjustedRect, container, context.m_applyCompositedClips, context.m_applyCompositedContainerScrolls, context.m_useEdgeInclusiveIntersection); Maybe just pass VisibleRectContext down > Source/WebCore/rendering/RenderBox.h:574 > + bool applyCachedClipAndScrollPosition(LayoutRect&, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const; A comment should say what the return value means. > Source/WebCore/rendering/RenderInline.cpp:849 > + containingBlock->applyCachedClipAndScrollPosition(repaintRect, repaintContainer, false /* applyCompositedClips */, shouldApplyCompositedContainerScrollsForRepaint(), false /* useEdgeInclusiveIntersection */); Ick! > Source/WebCore/rendering/RenderInline.cpp:874 > +LayoutRect RenderInline::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const Needs same rename. > Source/WebCore/rendering/RenderObject.h:699 > + // Given a rect in the object's coordinate space, compute the location in container space where this rect is visible, > + // when clipping and scrolling as specified by the context. When using edge-inclusive intersection, return std::nullopt > + // rather than an empty rect if the rect is completely clipped out in container space. > + struct VisibleRectContext { > + VisibleRectContext(bool hasPositionFixedDescendant = false, bool dirtyRectIsFlipped = false, bool useEdgeInclusiveIntersection = false, bool applyCompositedClips = true, bool applyCompositedContainerScrolls = true) > : m_hasPositionFixedDescendant(hasPositionFixedDescendant) > , m_dirtyRectIsFlipped(dirtyRectIsFlipped) > + , m_useEdgeInclusiveIntersection(useEdgeInclusiveIntersection) > + , m_applyCompositedClips(applyCompositedClips) > + , m_applyCompositedContainerScrolls(applyCompositedContainerScrolls) > { > } > bool m_hasPositionFixedDescendant; > bool m_dirtyRectIsFlipped; > + bool m_useEdgeInclusiveIntersection; > + bool m_applyCompositedClips; > + bool m_applyCompositedContainerScrolls; I think this could just be an enum and an OptionSet<> Created attachment 352140 [details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=351409&action=review >> Source/WebCore/rendering/RenderBox.cpp:973 >> +bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const > > I think this needs a better name, or maybe shouldn't return a bool, since only some callers care about intersection. Perhaps the intersection-related stuff should be in a 'context' object pass as an argument? Based on our offline discussion, I added a comment saying what the return value means, but left the name as-is. >> Source/WebCore/rendering/RenderBox.cpp:2138 >> +LayoutRect RenderBox::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const > > I think this is relative to the repaint container, not "absolute". The comments say that layoutState->paintOffset() is offset from the layout root, but I think it's probably the repaint root. > > Maybe just computeVisibleRectUsingPaintOffset() Done. >> Source/WebCore/rendering/RenderBox.cpp:2259 >> + bool isEmpty = !containerBox.applyCachedClipAndScrollPosition(adjustedRect, container, context.m_applyCompositedClips, context.m_applyCompositedContainerScrolls, context.m_useEdgeInclusiveIntersection); > > Maybe just pass VisibleRectContext down Done. >> Source/WebCore/rendering/RenderBox.h:574 >> + bool applyCachedClipAndScrollPosition(LayoutRect&, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const; > > A comment should say what the return value means. Done. >> Source/WebCore/rendering/RenderInline.cpp:849 >> + containingBlock->applyCachedClipAndScrollPosition(repaintRect, repaintContainer, false /* applyCompositedClips */, shouldApplyCompositedContainerScrollsForRepaint(), false /* useEdgeInclusiveIntersection */); > > Ick! Fixed now that the VisibleRectContext gets passed here. >> Source/WebCore/rendering/RenderInline.cpp:874 >> +LayoutRect RenderInline::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const > > Needs same rename. Done. >> Source/WebCore/rendering/RenderObject.h:699 >> + bool m_applyCompositedContainerScrolls; > > I think this could just be an enum and an OptionSet<> Done. Comment on attachment 352140 [details] Patch Clearing flags on attachment: 352140 Committed r237255: <https://trac.webkit.org/changeset/237255> All reviewed patches have been landed. Closing bug. This broke text selection in form fields: bug 190899 |