Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1284203004:
Generate scroll/clip display item hierarchy for SPv2 (Closed)
|
Created:
5 years, 4 months ago by trchen Modified:
5 years, 1 month ago CC:
aelias_OOO_until_Jul13, blink-reviews, blink-reviews-paint_chromium.org, Rik, danakj, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), Justin Novosad, pdr., pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, skobes, slimming-paint-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionGenerate clip/scroll display item hierarchy for SPv2
This CL is a preparation for impl-side scrolling support for SPv2.
In the old world, when we paint a stacking context (i.e. self-painting layer)
we compute the accumulated clip and scroll offset up to the painting root
(i.e. usually the invalidation backing, but may be shifted by 2D transform).
In order to support impl-side scrolling we need to send the full clip/scroll
hierarchy to cc, so the accumulated clip/scroll can be recomputed.
This CL should not affect SPv1. Any difference is unintentional.
Significant change is being made to display item list generation. In the past,
each self-painting layer is responsible for applying its own accumulated
clip/scroll. In other words, the graphics context is passed to DPLPainter
in the unclipped/unscrolled space. With this change, each layer applies its
own clip/scroll before passing the graphics context to children.
Things that are not implemented in this CL (possible follow-up tasks):
1. Generate scroll and transform nodes for scroll items in the tree builder.
2. Clip / transform warping for fixed-position elements.
3. Transformed layers are clipped incorrectly.
4. CSS clip. (crbug.com/527568)
5. Pagination / CSS multicol.
BUG=521179, 527568
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix crash #Patch Set 4 : still long way to go #
Total comments: 12
Patch Set 5 : add overflow clips back #Patch Set 6 : add fixed-pos workaround #Patch Set 7 : rebase, s/Sans/Exclude/, enumify #Patch Set 8 : fix build #Patch Set 9 : more fixed-pos workaround & update test & add test #Patch Set 10 : Fix failing test. Update test expectation. #
Total comments: 29
Patch Set 11 : revise #Patch Set 12 : rebase #Patch Set 13 : fix assertion failure #
Total comments: 22
Patch Set 14 : rebase & address comments. have not splitted fixed-pos workaround yet. #Patch Set 15 : stripped localPaintingInfo cleanup and fixed-pos workarounds #
Total comments: 17
Messages
Total messages: 58 (13 generated)
trchen@chromium.org changed reviewers: + vollick@chromium.org, wangxianzhu@chromium.org
This is an early preview for the design described in https://docs.google.com/a/chromium.org/document/d/1UV6R_o3iccxwsZzakiipWbQrGN... Parts yet need to be done: * Update transform tree builder * Add tests * Remove FixedPositionDisplayItem in favor of scroll space referral
trchen@chromium.org changed reviewers: + aelias@chromium.org
Patchset #3 (id:40001) has been deleted
Is there a bug for this CL? I'd like to see a test case for the problem of the original code.
On 2015/08/18 21:52:04, Xianzhu wrote: > Is there a bug for this CL? I'd like to see a test case for the problem of the > original code. chrishtr filed a issue: https://code.google.com/p/chromium/issues/detail?id=521179 Currently this CL is stuck because I also need to correct the clipping space. To issue clip in the local space is a bit tricky.
pdr@chromium.org changed reviewers: + pdr@chromium.org
I like where this is going but we still need tests (unit tests please). This will be a large change so I would support landing something simple with tests, then iterating on the TODOs such as fragments. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.cpp:713: return location() + LayoutSize(scrollOffset); Shouldn't this be subtraction? https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.h:145: LayoutPoint locationSansOverflowScroll() const; Super nit: locationWithoutOverflowScroll. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:219: // TODO(trchen): The transparent clip serves the purpose of raster hint for Skia, Can we fix this before landing your patch? I agree with your analysis. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:447: paintChildrenV2(childrenToVisit, context, paintingInfo, paintFlags); return here? https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:510: DeprecatedPaintLayer* container; DeprecatedPaintLayer* container = nullptr; (pointers aren't null by default) https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayerPainter.h (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.h:43: void paintChildrenV2(unsigned childrenToVisit, GraphicsContext*, const DeprecatedPaintLayerPaintingInfo&, PaintLayerFlags); Can you add a comment describing what's going on? Pretty much just say that we're not using accumulated clips, that this is for spv2, etc. May want to rename this paintChildrenForSlimmingPaintV2().
Patchset #5 (id:100001) has been deleted
Today's update: Now it generates correct overflow clip hierarchy! However many of the scroll/clip display items are duplicates. So the next step would be implementing the referral display items. Then the next next step would be using referral to implement fixed-pos elements. Currently broken: CSS clip, fixed-pos elements https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.cpp:713: return location() + LayoutSize(scrollOffset); On 2015/08/25 20:47:55, pdr wrote: > Shouldn't this be subtraction? A layer moves up (subtraction) when it scrolls, so it is moving down (addition) to revert scroll. ;) https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.h:145: LayoutPoint locationSansOverflowScroll() const; On 2015/08/25 20:47:55, pdr wrote: > Super nit: locationWithoutOverflowScroll. Acknowledged. Probably will de-dupe the function and use an enum option instead. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:219: // TODO(trchen): The transparent clip serves the purpose of raster hint for Skia, On 2015/08/25 20:47:55, pdr wrote: > Can we fix this before landing your patch? I agree with your analysis. Done. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:447: paintChildrenV2(childrenToVisit, context, paintingInfo, paintFlags); On 2015/08/25 20:47:55, pdr wrote: > return here? Done. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.cpp:510: DeprecatedPaintLayer* container; On 2015/08/25 20:47:55, pdr wrote: > DeprecatedPaintLayer* container = nullptr; > (pointers aren't null by default) Done. https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayerPainter.h (right): https://codereview.chromium.org/1284203004/diff/80001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayerPainter.h:43: void paintChildrenV2(unsigned childrenToVisit, GraphicsContext*, const DeprecatedPaintLayerPaintingInfo&, PaintLayerFlags); On 2015/08/25 20:47:55, pdr wrote: > Can you add a comment describing what's going on? Pretty much just say that > we're not using accumulated clips, that this is for spv2, etc. May want to > rename this paintChildrenForSlimmingPaintV2(). Acknowledged. Will come up with a descriptive name later.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284203004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284203004/220001
Ready for review! :) https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:291: if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { This change is unnecessary at this moment. Probably better implemented at tree builder / display item replay level. Will revert.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Lots of great stuff in this patch. Half my comments are just requests to pull easy bits out and land them while we iterate on the harder parts. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/BoxC... File Source/core/paint/BoxClipper.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/BoxC... Source/core/paint/BoxClipper.cpp:27: bool isOverflowClip = m_box.hasOverflowClip() && (!m_box.layer()->isSelfPaintingLayer() || RuntimeEnabledFeatures::slimmingPaintV2Enabled()); Should we return false from isSelfPaintingLayer when spv2 is on? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:2119: LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const DeprecatedPaintLayer* ancestorLayer, const LayoutPoint* offsetFromRoot) const Can we just switch this to two functions? LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const DeprecatedPaintLayer* ancestorLayer) const LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const LayoutPoint* offsetFromRoot) const If so, can this be done as a separate patch before landing the big one? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:149: LayoutPoint location(LocationQueryBehavior behavior = IncludeScroll) const { ASSERT(!m_needsPositionUpdate); return behavior == IncludeScroll ? m_location : locationExcludeOverflowScroll(); } With SPV2, won't all location(...)/convertToLayerCoords(...)/etc use ExcludeScroll? If so, can we refactor these functions like so: LayoutPoint location() const { ... if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) return m_location; // Compute and return locationExcludeOverflowScroll here. } And if we can do this, can we write unittests for these functions and land these changes before the big patch? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:256: LayoutRect physicalBoundingBoxIncludingReflectionAndStackingChildren(const LayoutPoint& offsetFromRoot) const; Lets pull the physicalBoundingBoxIncludingReflectionAndStackingChildren change into a separate patch. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:220: // A counter scroll needs to be applied in non-rootLayerScrolls mode, Can you add a test for this? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:287: // FilterRecorder may mutate paintingInfo. FilterRecorder -> FilterPainter, but I agree about reverting this area for now. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:497: { ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:511: IntSize scrollOffset = layer->layoutBox()->scrolledContentOffset(); Don't we need to emit a scroll recorder even if the offset is 0 in case the user scrolls? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:354: " <div id='positioned' style='position:relative; z-index: -1; width: 300px; height: 300px; border: 1px dotted black;'></div>" Is z-index: -1 needed here? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:357: document().view()->updateAllLifecyclePhases(); This isn't necessary as setBodyInnerHTML (confusingly) does it. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:408: Can you add a second test of the following scenario (from https://docs.google.com/document/d/1cOPCJufeXU7ifkb7kn3zQL8Bf7VZkS-KDGm3IyzXe...): <div id='scroller1' style='overflow: scroll; width: 100px; height: 100px;'> <div id='scroller2' style='overflow: scroll; width: 200px; height: 200px;'> <div id='positioned' style='top: 10px; left: 10px; position:relative; width: 300px; height: 300px; outline: 3px dotted black; background-color: green;'></div> </div> </div> I would expect this to generate two nested clip/scroll trees with one containing the positioned background, and one the positioned outline (plus some scrollbars that can be ignored). I tried this test and didn't see the outline phase working properly, which may mean recursivelyScrollAndPaintChildLayer doesn't handle phases properly, or my test was just bad. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... File Source/core/paint/LayerClipRecorder.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... Source/core/paint/LayerClipRecorder.cpp:47: ASSERT(layoutBox.hasOverflowClip()); Can you assert spv2 here? ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); Similarly, in the other constructor can you assert that spv2 is not enabled? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... File Source/core/paint/LayerClipRecorder.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... Source/core/paint/LayerClipRecorder.h:46: LayerClipRecorder(GraphicsContext&, const LayoutBox&, const LayoutPoint& paintOffset); Because the two modes for LayerClipRecorder are so different, lets add a second class in this file called LayerClipRecorderForSlimmingPaintV2 instead of re-using the existing class. The comment is very helpful so lets keep it, just tweaked for a new class. https://codereview.chromium.org/1284203004/diff/220001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItem.h:165: FixedPositionContainer, Won't we still need begin/end pairs for multiple fixed-pos containers (nested, or not). For example: http://jsfiddle.net/progers/euhb41Lf/1 Can we pull the fixed pos logic into a separate patch, maybe after the big one? I think the direction looks good overall, just want to simplify this patch.
https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/BoxC... File Source/core/paint/BoxClipper.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/BoxC... Source/core/paint/BoxClipper.cpp:27: bool isOverflowClip = m_box.hasOverflowClip() && (!m_box.layer()->isSelfPaintingLayer() || RuntimeEnabledFeatures::slimmingPaintV2Enabled()); On 2015/09/03 06:16:59, pdr wrote: > Should we return false from isSelfPaintingLayer when spv2 is on? Nope it affects painting order. Most of the time isSelfPaintingLayer means being a (pseudo) stacking context. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:2119: LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const DeprecatedPaintLayer* ancestorLayer, const LayoutPoint* offsetFromRoot) const On 2015/09/03 06:16:59, pdr wrote: > Can we just switch this to two functions? > > LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const DeprecatedPaintLayer* > ancestorLayer) const > > LayoutRect DeprecatedPaintLayer::physicalBoundingBox(const LayoutPoint* > offsetFromRoot) const > > If so, can this be done as a separate patch before landing the big one? Done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:149: LayoutPoint location(LocationQueryBehavior behavior = IncludeScroll) const { ASSERT(!m_needsPositionUpdate); return behavior == IncludeScroll ? m_location : locationExcludeOverflowScroll(); } On 2015/09/03 06:16:59, pdr wrote: > With SPV2, won't all location(...)/convertToLayerCoords(...)/etc use > ExcludeScroll? If so, can we refactor these functions like so: > LayoutPoint location() const > { > ... > if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) > return m_location; > > // Compute and return locationExcludeOverflowScroll here. > } > > And if we can do this, can we write unittests for these functions and land these > changes before the big patch? Better, we can exclude scroll offset in the beginning by changing DPL::updateLayerPosition(). There are many things that still rely on DPL geometry, e.g. hit-testing. IMO the smoother way to proceed is to gradually migrate to SPv2, then delete the IncludeScroll version afterwards. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:256: LayoutRect physicalBoundingBoxIncludingReflectionAndStackingChildren(const LayoutPoint& offsetFromRoot) const; On 2015/09/03 06:16:59, pdr wrote: > Lets pull the physicalBoundingBoxIncludingReflectionAndStackingChildren change > into a separate patch. Good point! Done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:220: // A counter scroll needs to be applied in non-rootLayerScrolls mode, On 2015/09/03 06:16:59, pdr wrote: > Can you add a test for this? I think it is a bit too early. This is only a workaround to make fixed-pos not looking too bad when we develop SPv2. If we do decide to ship with rootLayerScrolls, we can delete this workaround. If we ship without rootLayerScrolls, this workaround won't allow impl-thread scrolling. (i.e. the fixed-pos has to be treated as slow-repaint object.) We will need some kind of special display item to copy the viewport scroll offset. I propose BeginFixedPositionDisplayItem with nullptr anchor. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:287: // FilterRecorder may mutate paintingInfo. On 2015/09/03 06:16:59, pdr wrote: > FilterRecorder -> FilterPainter, but I agree about reverting this area for now. Revert done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:497: { On 2015/09/03 06:16:59, pdr wrote: > ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); Done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:511: IntSize scrollOffset = layer->layoutBox()->scrolledContentOffset(); On 2015/09/03 06:16:59, pdr wrote: > Don't we need to emit a scroll recorder even if the offset is 0 in case the user > scrolls? Isn't it the first clause on line 513? https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:354: " <div id='positioned' style='position:relative; z-index: -1; width: 300px; height: 300px; border: 1px dotted black;'></div>" On 2015/09/03 06:16:59, pdr wrote: > Is z-index: -1 needed here? Not important. Removal done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:357: document().view()->updateAllLifecyclePhases(); On 2015/09/03 06:16:59, pdr wrote: > This isn't necessary as setBodyInnerHTML (confusingly) does it. Done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:408: On 2015/09/03 06:16:59, pdr wrote: > Can you add a second test of the following scenario (from > https://docs.google.com/document/d/1cOPCJufeXU7ifkb7kn3zQL8Bf7VZkS-KDGm3IyzXe...): > <div id='scroller1' style='overflow: scroll; width: 100px; height: 100px;'> > <div id='scroller2' style='overflow: scroll; width: 200px; height: 200px;'> > <div id='positioned' style='top: 10px; left: 10px; position:relative; width: > 300px; height: 300px; outline: 3px dotted black; background-color: > green;'></div> > </div> > </div> > > I would expect this to generate two nested clip/scroll trees with one containing > the positioned background, and one the positioned outline (plus some scrollbars > that can be ignored). I tried this test and didn't see the outline phase working > properly, which may mean recursivelyScrollAndPaintChildLayer doesn't handle > phases properly, or my test was just bad. I ran your test case locally, the drawing display item for outline is appended right after the box decoration background, sharing the same pairs of clip/scroll tree. This is expected because position:relative makes the block a pseudo stacking context. If we remove position:relative, we shall see separate trees for each paint phase. Will add a test for it. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... File Source/core/paint/LayerClipRecorder.cpp (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... Source/core/paint/LayerClipRecorder.cpp:47: ASSERT(layoutBox.hasOverflowClip()); On 2015/09/03 06:16:59, pdr wrote: > Can you assert spv2 here? > ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); > > Similarly, in the other constructor can you assert that spv2 is not enabled? Done. https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... File Source/core/paint/LayerClipRecorder.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/core/paint/Laye... Source/core/paint/LayerClipRecorder.h:46: LayerClipRecorder(GraphicsContext&, const LayoutBox&, const LayoutPoint& paintOffset); On 2015/09/03 06:16:59, pdr wrote: > Because the two modes for LayerClipRecorder are so different, lets add a second > class in this file called LayerClipRecorderForSlimmingPaintV2 instead of > re-using the existing class. The comment is very helpful so lets keep it, just > tweaked for a new class. Done. https://codereview.chromium.org/1284203004/diff/220001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1284203004/diff/220001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItem.h:165: FixedPositionContainer, On 2015/09/03 06:16:59, pdr wrote: > Won't we still need begin/end pairs for multiple fixed-pos containers (nested, > or not). For example: http://jsfiddle.net/progers/euhb41Lf/1 Previously when I added it I was thinking the fixed-pos container will establish a scope, and fixed-pos display item will refer to the innermost enclosing one. Now I think the anchor/warp is a simpler approach (also more flexible). We may rename them to [Anchor|Warp]DisplayItem in the future if we also want to implement de-duping for O(n^2) scroll display item problem. > Can we pull the fixed pos logic into a separate patch, maybe after the big one? > I think the direction looks good overall, just want to simplify this patch. Sure we can! Just that fixed-pos will be broken in the meantime.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284203004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284203004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284203004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284203004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
pdr@chromium.org changed reviewers: + chrishtr@chromium.org
Looking good, lets shoot for landing this Wednesday. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:716: // Our m_location already has scroll offset baked-in. We have to revert it here. Ok, lets keep this and LocationQueryBehavior bug lets ASSERT(spv2) here to keep other folks from making a mistake and using this too early. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:718: if (DeprecatedPaintLayer* positionedParent = layoutObject()->isOutOfFlowPositioned() ? enclosingPositionedAncestor() : nullptr) { Nit: temporary code, but can we rewrite it like so? LayoutPoint DeprecatedPaintLayer::locationExcludeOverflowScroll() const { ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); // Our m_location already has scroll offset baked-in. We have to revert it here. DeprecatedPaintLayer* scrollParent = layoutObject()->isOutOfFlowPositioned() ? enclosingPositionedAncestor() : parent(); if (scrollParent && scrollParent->layoutObject()->hasOverflowClip()) return m_location + LayoutSize(scrollParent->layoutBox()->scrolledContentOffset()) return m_location; } https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (left): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:94: LayerFixedPositionRecorder fixedPositionRecorder(*context, *m_paintLayer.layoutObject()); Can you pull this refactoring of LayerFixedPositionRecorder into its own change (the removal of m_isFixedPosition and m_isFixedPositionContainer, the change here, and the spv1-specific refactoring in LayerFixedPositionRecorder)? The basic idea seems to be sound and separable. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:212: if (m_paintLayer.compositingState() == PaintsIntoOwnBacking) The offsetFromRoot refactoring looks correct, but I'm less sure about removing the PaintsIntoOwnBacking check here. How does this not affect SPV1 (here, and the PaintsIntoOwnBacking changes below)? Can this be pulled into a separate patch (before or after this change)? https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:124: ClipPathHelper(GraphicsContext* context, const DeprecatedPaintLayer& paintLayer, const LayoutRect& interestRect, LayoutRect& rootRelativeBounds, bool& rootRelativeBoundsComputed, Chris just landed a patch that uses DPLPI's ancestorHasClipPathClipping but I like the refactoring towards an interest rect here. Maybe we should switch this back to DPLI but plan on renaming DPLPI.paintDirtyRect to DPLPI.interestRect in a future patch? https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:290: localPaintingInfo.clipToDirtyRect = true; This violate's the comment above clipToDirtyRect because it can cause us to clip twice with spv1: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Should we even respect clipToDirtyRect with spv2? https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:496: // TODO(trchen): Hey this is slow. We should be able to calculate this on traversal. Would it be difficult to go ahead and compute all of this data and store it on DeprecatedPaintLayerPaintingInfo as we walk down? https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:529: DeprecatedPaintLayer* currentLayer = child; Can you break this out into computeClippingLayersBetween(child, m_paintLayer) (or something like this). https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:350: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, OverflowClipHierarchy) If you'd like to keep the fixed position code in this patch we should add a test for it. I recommend leaving it broken for now and then adding support in a followup. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... File Source/core/paint/LayerDescendantClipRecorder.h (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... Source/core/paint/LayerDescendantClipRecorder.h:30: const LayoutBox& m_layoutObject; Nit: m_layoutBox;
On 2015/09/08 04:23:54, pdr wrote: > Looking good, lets shoot for landing this Wednesday. > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayer.cpp (right): > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayer.cpp:716: // Our m_location already has > scroll offset baked-in. We have to revert it here. > Ok, lets keep this and LocationQueryBehavior bug lets ASSERT(spv2) here to keep > other folks from making a mistake and using this too early. > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayer.cpp:718: if (DeprecatedPaintLayer* > positionedParent = layoutObject()->isOutOfFlowPositioned() ? > enclosingPositionedAncestor() : nullptr) { > Nit: temporary code, but can we rewrite it like so? > LayoutPoint DeprecatedPaintLayer::locationExcludeOverflowScroll() const > { > ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); > > // Our m_location already has scroll offset baked-in. We have to revert it > here. > DeprecatedPaintLayer* scrollParent = layoutObject()->isOutOfFlowPositioned() > ? enclosingPositionedAncestor() : parent(); > if (scrollParent && scrollParent->layoutObject()->hasOverflowClip()) > return m_location + > LayoutSize(scrollParent->layoutBox()->scrolledContentOffset()) > return m_location; > } > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayerPainter.cpp (left): > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:94: LayerFixedPositionRecorder > fixedPositionRecorder(*context, *m_paintLayer.layoutObject()); > Can you pull this refactoring of LayerFixedPositionRecorder into its own change > (the removal of m_isFixedPosition and m_isFixedPositionContainer, the change > here, and the spv1-specific refactoring in LayerFixedPositionRecorder)? The > basic idea seems to be sound and separable. > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:212: if > (m_paintLayer.compositingState() == PaintsIntoOwnBacking) > The offsetFromRoot refactoring looks correct, but I'm less sure about removing > the PaintsIntoOwnBacking check here. How does this not affect SPV1 (here, and > the PaintsIntoOwnBacking changes below)? Can this be pulled into a separate > patch (before or after this change)? > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:124: > ClipPathHelper(GraphicsContext* context, const DeprecatedPaintLayer& paintLayer, > const LayoutRect& interestRect, LayoutRect& rootRelativeBounds, bool& > rootRelativeBoundsComputed, > Chris just landed a patch that uses DPLPI's ancestorHasClipPathClipping but I > like the refactoring towards an interest rect here. Maybe we should switch this > back to DPLI but plan on renaming DPLPI.paintDirtyRect to DPLPI.interestRect in > a future patch? > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:290: > localPaintingInfo.clipToDirtyRect = true; > This violate's the comment above clipToDirtyRect because it can cause us to clip > twice with spv1: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Should we even respect clipToDirtyRect with spv2? > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:496: // TODO(trchen): Hey this > is slow. We should be able to calculate this on traversal. > Would it be difficult to go ahead and compute all of this data and store it on > DeprecatedPaintLayerPaintingInfo as we walk down? > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:529: DeprecatedPaintLayer* > currentLayer = child; > Can you break this out into computeClippingLayersBetween(child, m_paintLayer) > (or something like this). > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Disp... > File Source/core/paint/DisplayItemListPaintTest.cpp (right): > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Disp... > Source/core/paint/DisplayItemListPaintTest.cpp:350: > TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, OverflowClipHierarchy) > If you'd like to keep the fixed position code in this patch we should add a test > for it. I recommend leaving it broken for now and then adding support in a > followup. > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... > File Source/core/paint/LayerDescendantClipRecorder.h (right): > > https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... > Source/core/paint/LayerDescendantClipRecorder.h:30: const LayoutBox& > m_layoutObject; > Nit: m_layoutBox; This is perhaps a dumb question, but it looks like this is using the anchor approach from (https://docs.google.com/document/d/1UV6R_o3iccxwsZzakiipWbQrGNtVxsB-QVzMk9sqb...) (along with the fixed and scroll display list items), is that true? If so, does that document have the up-to-date description of how the display list is built and interpreted?
I hope you don't mind a couple nits. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:153: LayoutPoint locationExcludeOverflowScroll() const; nit: "locationExcludingOverflowScroll", please? I initially read this as "locationExcludesOverflowScroll" and wondered why it didn't return a boolean. https://codereview.chromium.org/1284203004/diff/280001/Source/platform/graphi... File Source/platform/graphics/paint/FixedPositionDisplayItem.h (right): https://codereview.chromium.org/1284203004/diff/280001/Source/platform/graphi... Source/platform/graphics/paint/FixedPositionDisplayItem.h:23: DisplayItemClient anchor() const { return m_anchor; } nit: Would you mind a comment (here or as a class comment) which explains what the semantics of this are? I think I've figured it out, but I don't think it's obvious to someone passing through what a "fixed position anchor" is. (I might argue that the "fixed position container" is closer to spec wording, but I'm not picky about that.)
One other question (at the risk of bikeshedding): vollick@ recently reminded me that absolute-positioned content also exhibits the same escaping/anchoring behaviour. Is the plan to use FixedPositionDisplayItem for this, too? (It seems like it might be reasonable, even though absolutely positioned content is in some sense "fixed", just to a different containing block.) If so, should we switch to a more generic name. Some things that have been discussed (none of which I'm a huge fan of, but since I'm bikeshedding): - AnchorDisplayItem/AnchoredDisplayItem - AnchorDisplayItem/AnchoredContentDisplayItem - PositionDisplayItem/Positioned(Content)?DisplayItem
https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:716: // Our m_location already has scroll offset baked-in. We have to revert it here. On 2015/09/08 04:23:53, pdr wrote: > Ok, lets keep this and LocationQueryBehavior bug lets ASSERT(spv2) here to keep > other folks from making a mistake and using this too early. Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:718: if (DeprecatedPaintLayer* positionedParent = layoutObject()->isOutOfFlowPositioned() ? enclosingPositionedAncestor() : nullptr) { On 2015/09/08 04:23:53, pdr wrote: > Nit: temporary code, but can we rewrite it like so? > LayoutPoint DeprecatedPaintLayer::locationExcludeOverflowScroll() const > { > ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); > > // Our m_location already has scroll offset baked-in. We have to revert it > here. > DeprecatedPaintLayer* scrollParent = layoutObject()->isOutOfFlowPositioned() > ? enclosingPositionedAncestor() : parent(); > if (scrollParent && scrollParent->layoutObject()->hasOverflowClip()) > return m_location + > LayoutSize(scrollParent->layoutBox()->scrolledContentOffset()) > return m_location; > } Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:153: LayoutPoint locationExcludeOverflowScroll() const; On 2015/09/09 19:58:30, jbroman wrote: > nit: "locationExcludingOverflowScroll", please? I initially read this as > "locationExcludesOverflowScroll" and wondered why it didn't return a boolean. Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (left): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:94: LayerFixedPositionRecorder fixedPositionRecorder(*context, *m_paintLayer.layoutObject()); On 2015/09/08 04:23:53, pdr wrote: > Can you pull this refactoring of LayerFixedPositionRecorder into its own change > (the removal of m_isFixedPosition and m_isFixedPositionContainer, the change > here, and the spv1-specific refactoring in LayerFixedPositionRecorder)? The > basic idea seems to be sound and separable. Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:212: if (m_paintLayer.compositingState() == PaintsIntoOwnBacking) On 2015/09/08 04:23:53, pdr wrote: > The offsetFromRoot refactoring looks correct, but I'm less sure about removing > the PaintsIntoOwnBacking check here. How does this not affect SPV1 (here, and > the PaintsIntoOwnBacking changes below)? Can this be pulled into a separate > patch (before or after this change)? Done. I think paintingInfo.subPixelAccumulation will be set by the caller in every code path. Anyway it is not a necessity here. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:124: ClipPathHelper(GraphicsContext* context, const DeprecatedPaintLayer& paintLayer, const LayoutRect& interestRect, LayoutRect& rootRelativeBounds, bool& rootRelativeBoundsComputed, On 2015/09/08 04:23:53, pdr wrote: > Chris just landed a patch that uses DPLPI's ancestorHasClipPathClipping but I > like the refactoring towards an interest rect here. Maybe we should switch this > back to DPLI but plan on renaming DPLPI.paintDirtyRect to DPLPI.interestRect in > a future patch? Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:290: localPaintingInfo.clipToDirtyRect = true; On 2015/09/08 04:23:53, pdr wrote: > This violate's the comment above clipToDirtyRect because it can cause us to clip > twice with spv1: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Should we even respect clipToDirtyRect with spv2? IMO clipToDirtyRect is not inherited and shouldn't belong to DPLPI anyway. Normally we apply clip per-fragment, but skip clipping if a filter clip is applied so the edge won't be doubly clipped with anti-alias. I feel this hack should be removed and we should always apply per-fragment clip. Yea the changes around localPaintingInfo in this CL are totally no-op. Just to make sure it doesn't serve any purpose other than carrying clipToDirtyRect. I can revert it and upload a separate CL to remove it at all. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:496: // TODO(trchen): Hey this is slow. We should be able to calculate this on traversal. On 2015/09/08 04:23:53, pdr wrote: > Would it be difficult to go ahead and compute all of this data and store it on > DeprecatedPaintLayerPaintingInfo as we walk down? Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:529: DeprecatedPaintLayer* currentLayer = child; On 2015/09/08 04:23:53, pdr wrote: > Can you break this out into computeClippingLayersBetween(child, m_paintLayer) > (or something like this). Done. https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... File Source/core/paint/LayerDescendantClipRecorder.h (right): https://codereview.chromium.org/1284203004/diff/280001/Source/core/paint/Laye... Source/core/paint/LayerDescendantClipRecorder.h:30: const LayoutBox& m_layoutObject; On 2015/09/08 04:23:54, pdr wrote: > Nit: m_layoutBox; Done.
On 2015/09/09 19:50:26, vollick wrote: > This is perhaps a dumb question, but it looks like this is using the anchor > approach from > (https://docs.google.com/document/d/1UV6R_o3iccxwsZzakiipWbQrGNtVxsB-QVzMk9sqb...) > (along with the fixed and scroll display list items), is that true? If so, does > that document have the up-to-date description of how the display list is built > and interpreted? Yes the concept is pretty much the same as described, though some of the details are subject to change. e.g. renaming FixedPosContainerDI to AnchorDI, and whether we should separate transform anchor and clip anchor. On 2015/09/10 15:20:57, jbroman wrote: > One other question (at the risk of bikeshedding): vollick@ recently reminded me > that absolute-positioned content also exhibits the same escaping/anchoring > behaviour. Does it has something to do with clip-path? CSS clip requires position:absolute so it guarantees being a position ancestor of an affected element unless the affected element is fixed-pos. clip-path looks like a different beast. It guarantees stacking context but not position container. I think it totally makes sense to implement it as an effect node instead of a clip node. > Is the plan to use FixedPositionDisplayItem for this, too? (It seems like it > might be reasonable, even though absolutely positioned content is in some sense > "fixed", just to a different containing block.) I think absolute position will be fine with this implementation without using anchors. > If so, should we switch to a more generic name. Some things that have been > discussed (none of which I'm a huge fan of, but since I'm bikeshedding): > - AnchorDisplayItem/AnchoredDisplayItem > - AnchorDisplayItem/AnchoredContentDisplayItem > - PositionDisplayItem/Positioned(Content)?DisplayItem +1. I haven't done it just because it is unimportant for now.
LGTM I'd like either chrishtr or wangxianzhu to give this an LGTM as well.
lgtm https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:159: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, OverflowClipHierarchy) Can this be moved into DeprecatedPaintLayerPainterTest.cpp?
Reviewing now. Please don't commit just yet.
https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:2108: if (!(paintLayerFlags & PaintLayerPaintingOverflowContents) && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { Why this change? https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:521: static DeprecatedPaintLayerPainter::PaintResult recursivelyScrollAndPaintChildLayer(Vector<DeprecatedPaintLayer*>& clippingAncestors, DeprecatedPaintLayer& child, GraphicsContext& context, const DeprecatedPaintLayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags, DeprecatedPaintLayer* paintOffsetBase, LayoutPoint paintOffset) This looks like a bottom-up approach: when encountering a new DPL, figure out its entire clip and scroll chain up to the root of the document, and emit it. Why not instead do it top-down: when painting children of a DPL, first paint pieces of the subtree that are clipped/scrolled by it, then the part that is not. e.g.: void paintLayer() { { ClipRecorder recorder; paintClippedChildren(); } paintUnclippedChildren(); } paintClippedChildren() { for (child in children) { if (isClipped(child, this)) paintLayer(child); } } void paintUnclippedChildren() { { ScrollRecorder recorder; paintScrolledButNotClippedChildren(); } paintUnclippedAndUnscrolledChildren(); } void paintScrolledButNotClippedChildren() { for (child in children) { // I don't think there are any such children, but anyway. if (!clippedBy(child, this) && scrollsWith(child, this)) child->paintLayer(); } etc. Will this algorithm work? (modulo position: fixed, which can maybe be handled specially) I believe it's also linear time. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:551: static void collectClippingLayersBetween(Vector<DeprecatedPaintLayer*> &clippingAncestors, DeprecatedPaintLayer* layer, DeprecatedPaintLayer* ancestor) This function is doing almost (exactly?) the same thing as DeprecatedPaintLayerClipper.
https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:2108: if (!(paintLayerFlags & PaintLayerPaintingOverflowContents) && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { On 2015/09/17 00:53:46, chrishtr wrote: > Why this change? In rootLayerScrolls-mode the composited bounds will be the size of the viewport because LayoutView clip and scrolls the document. Our interest rect will incorrectly cropped in that case. In normal mode this is no-op. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:521: static DeprecatedPaintLayerPainter::PaintResult recursivelyScrollAndPaintChildLayer(Vector<DeprecatedPaintLayer*>& clippingAncestors, DeprecatedPaintLayer& child, GraphicsContext& context, const DeprecatedPaintLayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags, DeprecatedPaintLayer* paintOffsetBase, LayoutPoint paintOffset) On 2015/09/17 00:53:46, chrishtr wrote: > This looks like a bottom-up approach: when encountering a new DPL, figure out > its entire clip and scroll chain up to the root of the document, and emit it. Only up to the nearest self-painting ancestor. i.e. (pseudo) stacking context. > Why not instead do it top-down: when painting children of a DPL, first paint > pieces of the subtree that are clipped/scrolled by it, then the part that is > not. e.g.: > > void paintLayer() { > { > ClipRecorder recorder; > paintClippedChildren(); > } > paintUnclippedChildren(); > } > > paintClippedChildren() { > for (child in children) { > if (isClipped(child, this)) > paintLayer(child); > } > } > > void paintUnclippedChildren() { > { > ScrollRecorder recorder; > paintScrolledButNotClippedChildren(); > } > paintUnclippedAndUnscrolledChildren(); > } > > void paintScrolledButNotClippedChildren() { > for (child in children) { > // I don't think there are any such children, but anyway. > if (!clippedBy(child, this) && scrollsWith(child, this)) > child->paintLayer(); > } > > etc. > > > Will this algorithm work? (modulo position: fixed, which can maybe be handled > specially) > > I believe it's also linear time. Order will be incorrect. We want to traverse non-self-painting descendants in painting order. The above algorithm traverses in positioning order. For example: <div style="overflow:scroll"> <div style="position:relative; z-index:1;">A</div> <div style="position:relative; z-index:3;">B</div> </div> <div style="overflow:scroll"> <div style="position:relative; z-index:2;">C</div> <div style="position:relative; z-index:4;">D</div> </div> The traverse order of your algorithm will be ABCD, but the correct order should be ACBD. An alternative approach is to run your algorithm first to generate a bunch of meta display items without painting actual layer contents. Then use a warp gate (a.k.a. referral display item) to jump to the correct space when we paint them in painting order. I discussed this alternative approach with pdr@ before, but our conclusion is to favor simplicity unless we see performance problem in real world https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:551: static void collectClippingLayersBetween(Vector<DeprecatedPaintLayer*> &clippingAncestors, DeprecatedPaintLayer* layer, DeprecatedPaintLayer* ancestor) On 2015/09/17 00:53:46, chrishtr wrote: > This function is doing almost (exactly?) the same thing as > DeprecatedPaintLayerClipper. Yep. Albeit we don't try to collapse clip rects right away, but defer it until layerization or even rasterization. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:159: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, OverflowClipHierarchy) On 2015/09/15 23:40:13, Xianzhu wrote: > Can this be moved into DeprecatedPaintLayerPainterTest.cpp? Acknowledged.
On 2015/09/17 at 01:56:04, trchen wrote: > https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... > File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp (right): > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... > Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:2108: if (!(paintLayerFlags & PaintLayerPaintingOverflowContents) && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { > On 2015/09/17 00:53:46, chrishtr wrote: > > Why this change? > > In rootLayerScrolls-mode the composited bounds will be the size of the viewport because LayoutView clip and scrolls the document. Our interest rect will incorrectly cropped in that case. > > In normal mode this is no-op. > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:521: static DeprecatedPaintLayerPainter::PaintResult recursivelyScrollAndPaintChildLayer(Vector<DeprecatedPaintLayer*>& clippingAncestors, DeprecatedPaintLayer& child, GraphicsContext& context, const DeprecatedPaintLayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags, DeprecatedPaintLayer* paintOffsetBase, LayoutPoint paintOffset) > On 2015/09/17 00:53:46, chrishtr wrote: > > This looks like a bottom-up approach: when encountering a new DPL, figure out > > its entire clip and scroll chain up to the root of the document, and emit it. > > Only up to the nearest self-painting ancestor. i.e. (pseudo) stacking context. > > > Why not instead do it top-down: when painting children of a DPL, first paint > > pieces of the subtree that are clipped/scrolled by it, then the part that is > > not. e.g.: > > > > void paintLayer() { > > { > > ClipRecorder recorder; > > paintClippedChildren(); > > } > > paintUnclippedChildren(); > > } > > > > paintClippedChildren() { > > for (child in children) { > > if (isClipped(child, this)) > > paintLayer(child); > > } > > } > > > > void paintUnclippedChildren() { > > { > > ScrollRecorder recorder; > > paintScrolledButNotClippedChildren(); > > } > > paintUnclippedAndUnscrolledChildren(); > > } > > > > void paintScrolledButNotClippedChildren() { > > for (child in children) { > > // I don't think there are any such children, but anyway. > > if (!clippedBy(child, this) && scrollsWith(child, this)) > > child->paintLayer(); > > } > > > > etc. > > > > > > Will this algorithm work? (modulo position: fixed, which can maybe be handled > > specially) > > > > I believe it's also linear time. > > Order will be incorrect. We want to traverse non-self-painting descendants in painting order. The above algorithm traverses in positioning order. For example: > <div style="overflow:scroll"> > <div style="position:relative; z-index:1;">A</div> > <div style="position:relative; z-index:3;">B</div> > </div> > <div style="overflow:scroll"> > <div style="position:relative; z-index:2;">C</div> > <div style="position:relative; z-index:4;">D</div> > </div> > > The traverse order of your algorithm will be ABCD, but the correct order should be ACBD. Right, of course, sorry for forgetting that. Thinking more... > > An alternative approach is to run your algorithm first to generate a bunch of meta display items without painting actual layer contents. Then use a warp gate (a.k.a. referral display item) to jump to the correct space when we paint them in painting order. > > I discussed this alternative approach with pdr@ before, but our conclusion is to favor simplicity unless we see performance problem in real world > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:551: static void collectClippingLayersBetween(Vector<DeprecatedPaintLayer*> &clippingAncestors, DeprecatedPaintLayer* layer, DeprecatedPaintLayer* ancestor) > On 2015/09/17 00:53:46, chrishtr wrote: > > This function is doing almost (exactly?) the same thing as > > DeprecatedPaintLayerClipper. > > Yep. Albeit we don't try to collapse clip rects right away, but defer it until layerization or even rasterization. > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... > File Source/core/paint/DisplayItemListPaintTest.cpp (right): > > https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Disp... > Source/core/paint/DisplayItemListPaintTest.cpp:159: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, OverflowClipHierarchy) > On 2015/09/15 23:40:13, Xianzhu wrote: > > Can this be moved into DeprecatedPaintLayerPainterTest.cpp? > > Acknowledged.
Ok I'm now on board with the approach in this CL, as long as we address the design questions, and can prove to ourselves that we will be able to nuke DeprecatedPaintLayerClipper entirely. Deleting that code will be a great win just by itself for code complexity and performance. https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/layout/com... Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:2108: if (!(paintLayerFlags & PaintLayerPaintingOverflowContents) && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { On 2015/09/17 at 01:56:04, trchen wrote: > On 2015/09/17 00:53:46, chrishtr wrote: > > Why this change? > > In rootLayerScrolls-mode the composited bounds will be the size of the viewport because LayoutView clip and scrolls the document. Our interest rect will incorrectly cropped in that case. > > In normal mode this is no-op. What does this have to do with this CL? Furthermore, interest rects will be managed by Blink, not cc, in v2, so all the code in this file will be obsolete. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/BoxC... File Source/core/paint/BoxClipper.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/BoxC... Source/core/paint/BoxClipper.cpp:27: bool isOverflowClip = m_box.hasOverflowClip() && (!m_box.layer()->isSelfPaintingLayer() || RuntimeEnabledFeatures::slimmingPaintV2Enabled()); Why this change? https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/BoxC... Source/core/paint/BoxClipper.cpp:38: if (contentsClipBehavior == SkipContentsClipIfPossible && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) { Is this change because we want to preserve clips in phase 2 even if they are no-op, in order to make overflowClip composite more easily? https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.cpp:1304: location += layer->location(behavior); I think this introduces n^2 performance, because of the way that location without scrolling asks for the positioned ancestor. Instead how about storing m_locationWithoutScrollOffset directly on each DPL and recomputing along with m_location? https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:280: // TODO(trchen): Need to handle layer fragmentation. How hard will this be? We need a clear plan for it before committing this patch. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:569: if (container->layoutObject()->hasOverflowClip()) What about css clip and clip-path? https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:591: // TODO(trchen): Fixed position needs to be implemented here. We need a plan for how to do it before committing this patch.
chrishtr@chromium.org changed reviewers: + mstensho@opera.com
trchen, pdr, wangxianzhu and I just had a long and productive meeting about this patch and the larger question of the path towards implementing the correct property tree states in the DisplayItemList. Here are outcomes (others please add more if I forgot some): 1. The achievement of this patch is to unroll nested clips and transforms between a paint ancestor (i.e. stacking context container or stacking context container for paint) and its child. This will help us implement correct and general SimpleLayer layerization for such scrollers. Warp points/property tree state references can be done separately. 2. CSS clip and clip-path are difficult and it does not suffice to unroll up to the paint ancesto r (consider: an element with CSS clip, an overflow:scroll child with opacity 0.999, and a position:absolute grandchild). Positioned descendants will in general need their own clip tree node path up to their positioned ancestor's clip node. On reflection, I think this is the reason DeprecatedPaintLayerClipper is so complicated. Furthermore, I'm quite sure that clip-path is implemented incorrectly as a result, because it does not use the DeprecatedPaintLayerClipper semantics. 3. More work is needed to implement fragmentation correctly. Tien-Ren has an idea that it could be easier to implement fragmentation top down from the pagination container, rather than self-fragmenting. +mstensho for comment. 4. Fixed position shouldn't be too much harder than absolute position.
There was also an action item for trchen to upload some more test cases for weird corner cases involving clip.
Re clips vs overflow clips and having to go up to the root to generate clip hierarchies: how about we mark clips and overflow clips differently in the display list? Then even though we might have nested sequences of overflow clips and clips, drawing display items can specify whether they obey the combined clip and overflow clip parent hierarchy (non-positioned), or just the clip one (position abs or fixed). This would allow us to represent the clip or overflow clip display items in the DisplayItemList just once. When we then create the clip property tree from them DisplayItemList, we can generate two hierarchies: one with clip and overflow clip, and one with just clip. abs/fixed position elements would refer to the latter, otherwise to the former.
On 2015/09/19 00:15:21, chrishtr wrote:
> Re clips vs overflow clips and having to go up to the root to generate clip
> hierarchies: how about we mark clips and overflow clips differently in the
> display list? Then even though we might have nested sequences of overflow
clips
> and clips, drawing display items can specify whether they obey the combined
> clip and overflow clip parent hierarchy (non-positioned), or just the clip one
> (position abs or fixed).
>
> This would allow us to represent the clip or overflow clip display items in
the
> DisplayItemList just once. When we then create the clip property tree from
them
> DisplayItemList, we can generate two hierarchies: one with clip and overflow
> clip, and one with just clip. abs/fixed position elements would refer to the
> latter, otherwise to the former.
abs-pos and rel-pos should work the same way as static-pos, they both apply
overflow clips in positioning ancestor chain.
fixed-pos is special because it could apply clip outside of positioning ancestor
chain.
How about this case:
<div style="position:absolute; clip:rect(...);">
<div style="transform:...;">
<div style="position:absolute; clip:rect(...);">
<div style="position:fixed;">
The fixed-pos element escape the inner clip but not the outer clip.
On 2015/09/19 at 00:36:53, trchen wrote: > On 2015/09/19 00:15:21, chrishtr wrote: > > Re clips vs overflow clips and having to go up to the root to generate clip > > hierarchies: how about we mark clips and overflow clips differently in the > > display list? Then even though we might have nested sequences of overflow clips > > and clips, drawing display items can specify whether they obey the combined > > clip and overflow clip parent hierarchy (non-positioned), or just the clip one > > (position abs or fixed). > > > > This would allow us to represent the clip or overflow clip display items in the > > DisplayItemList just once. When we then create the clip property tree from them > > DisplayItemList, we can generate two hierarchies: one with clip and overflow > > clip, and one with just clip. abs/fixed position elements would refer to the > > latter, otherwise to the former. > > abs-pos and rel-pos should work the same way as static-pos, they both apply overflow clips in positioning ancestor chain. > > fixed-pos is special because it could apply clip outside of positioning ancestor chain. > > How about this case: > <div style="position:absolute; clip:rect(...);"> > <div style="transform:...;"> > <div style="position:absolute; clip:rect(...);"> > <div style="position:fixed;"> > > The fixed-pos element escape the inner clip but not the outer clip. Why does it escape the inner clip?
On 2015/09/19 00:55:08, chrishtr wrote:
> On 2015/09/19 at 00:36:53, trchen wrote:
> > On 2015/09/19 00:15:21, chrishtr wrote:
> > > Re clips vs overflow clips and having to go up to the root to generate
clip
> > > hierarchies: how about we mark clips and overflow clips differently in the
> > > display list? Then even though we might have nested sequences of overflow
> clips
> > > and clips, drawing display items can specify whether they obey the
combined
> > > clip and overflow clip parent hierarchy (non-positioned), or just the clip
> one
> > > (position abs or fixed).
> > >
> > > This would allow us to represent the clip or overflow clip display items
in
> the
> > > DisplayItemList just once. When we then create the clip property tree from
> them
> > > DisplayItemList, we can generate two hierarchies: one with clip and
overflow
> > > clip, and one with just clip. abs/fixed position elements would refer to
the
> > > latter, otherwise to the former.
> >
> > abs-pos and rel-pos should work the same way as static-pos, they both apply
> overflow clips in positioning ancestor chain.
> >
> > fixed-pos is special because it could apply clip outside of positioning
> ancestor chain.
> >
> > How about this case:
> > <div style="position:absolute; clip:rect(...);">
> > <div style="transform:...;">
> > <div style="position:absolute; clip:rect(...);">
> > <div style="position:fixed;">
> >
> > The fixed-pos element escape the inner clip but not the outer clip.
>
> Why does it escape the inner clip?
Oops, sorry I made the example wrong. Should be this:
<div style="overflow:hidden;">
<div style="transform:...;">
<div style="overflow:hidden;">
<div style="position:fixed;">
On 2015/09/19 at 00:58:33, trchen wrote: > On 2015/09/19 00:55:08, chrishtr wrote: > > On 2015/09/19 at 00:36:53, trchen wrote: > > > On 2015/09/19 00:15:21, chrishtr wrote: > > > > Re clips vs overflow clips and having to go up to the root to generate clip > > > > hierarchies: how about we mark clips and overflow clips differently in the > > > > display list? Then even though we might have nested sequences of overflow > > clips > > > > and clips, drawing display items can specify whether they obey the combined > > > > clip and overflow clip parent hierarchy (non-positioned), or just the clip > > one > > > > (position abs or fixed). > > > > > > > > This would allow us to represent the clip or overflow clip display items in > > the > > > > DisplayItemList just once. When we then create the clip property tree from > > them > > > > DisplayItemList, we can generate two hierarchies: one with clip and overflow > > > > clip, and one with just clip. abs/fixed position elements would refer to the > > > > latter, otherwise to the former. > > > > > > abs-pos and rel-pos should work the same way as static-pos, they both apply > > overflow clips in positioning ancestor chain. > > > > > > fixed-pos is special because it could apply clip outside of positioning > > ancestor chain. > > > > > > How about this case: > > > <div style="position:absolute; clip:rect(...);"> > > > <div style="transform:...;"> > > > <div style="position:absolute; clip:rect(...);"> > > > <div style="position:fixed;"> > > > > > > The fixed-pos element escape the inner clip but not the outer clip. > > > > Why does it escape the inner clip? > > Oops, sorry I made the example wrong. Should be this: > <div style="overflow:hidden;"> > <div style="transform:...;"> > <div style="overflow:hidden;"> > <div style="position:fixed;"> Ah. But this example will apply to positioned elements also, no?
On 2015/09/19 at 01:03:04, chrishtr wrote: > On 2015/09/19 at 00:58:33, trchen wrote: > > On 2015/09/19 00:55:08, chrishtr wrote: > > > On 2015/09/19 at 00:36:53, trchen wrote: > > > > On 2015/09/19 00:15:21, chrishtr wrote: > > > > > Re clips vs overflow clips and having to go up to the root to generate clip > > > > > hierarchies: how about we mark clips and overflow clips differently in the > > > > > display list? Then even though we might have nested sequences of overflow > > > clips > > > > > and clips, drawing display items can specify whether they obey the combined > > > > > clip and overflow clip parent hierarchy (non-positioned), or just the clip > > > one > > > > > (position abs or fixed). > > > > > > > > > > This would allow us to represent the clip or overflow clip display items in > > > the > > > > > DisplayItemList just once. When we then create the clip property tree from > > > them > > > > > DisplayItemList, we can generate two hierarchies: one with clip and overflow > > > > > clip, and one with just clip. abs/fixed position elements would refer to the > > > > > latter, otherwise to the former. > > > > > > > > abs-pos and rel-pos should work the same way as static-pos, they both apply > > > overflow clips in positioning ancestor chain. > > > > > > > > fixed-pos is special because it could apply clip outside of positioning > > > ancestor chain. > > > > > > > > How about this case: > > > > <div style="position:absolute; clip:rect(...);"> > > > > <div style="transform:...;"> > > > > <div style="position:absolute; clip:rect(...);"> > > > > <div style="position:fixed;"> > > > > > > > > The fixed-pos element escape the inner clip but not the outer clip. > > > > > > Why does it escape the inner clip? > > > > Oops, sorry I made the example wrong. Should be this: > > <div style="overflow:hidden;"> > > <div style="transform:...;"> > > <div style="overflow:hidden;"> > > <div style="position:fixed;"> > > Ah. But this example will apply to positioned elements also, no? Absolute positioned, that is.
On 2015/09/19 01:03:18, chrishtr wrote:
> On 2015/09/19 at 01:03:04, chrishtr wrote:
> > On 2015/09/19 at 00:58:33, trchen wrote:
> > > On 2015/09/19 00:55:08, chrishtr wrote:
> > > > On 2015/09/19 at 00:36:53, trchen wrote:
> > > > > On 2015/09/19 00:15:21, chrishtr wrote:
> > > > > > Re clips vs overflow clips and having to go up to the root to
generate
> clip
> > > > > > hierarchies: how about we mark clips and overflow clips differently
in
> the
> > > > > > display list? Then even though we might have nested sequences of
> overflow
> > > > clips
> > > > > > and clips, drawing display items can specify whether they obey the
> combined
> > > > > > clip and overflow clip parent hierarchy (non-positioned), or just
the
> clip
> > > > one
> > > > > > (position abs or fixed).
> > > > > >
> > > > > > This would allow us to represent the clip or overflow clip display
> items in
> > > > the
> > > > > > DisplayItemList just once. When we then create the clip property
tree
> from
> > > > them
> > > > > > DisplayItemList, we can generate two hierarchies: one with clip and
> overflow
> > > > > > clip, and one with just clip. abs/fixed position elements would
refer
> to the
> > > > > > latter, otherwise to the former.
> > > > >
> > > > > abs-pos and rel-pos should work the same way as static-pos, they both
> apply
> > > > overflow clips in positioning ancestor chain.
> > > > >
> > > > > fixed-pos is special because it could apply clip outside of
positioning
> > > > ancestor chain.
> > > > >
> > > > > How about this case:
> > > > > <div style="position:absolute; clip:rect(...);">
> > > > > <div style="transform:...;">
> > > > > <div style="position:absolute; clip:rect(...);">
> > > > > <div style="position:fixed;">
> > > > >
> > > > > The fixed-pos element escape the inner clip but not the outer clip.
> > > >
> > > > Why does it escape the inner clip?
> > >
> > > Oops, sorry I made the example wrong. Should be this:
> > > <div style="overflow:hidden;">
> > > <div style="transform:...;">
> > > <div style="overflow:hidden;">
> > > <div style="position:fixed;">
> >
> > Ah. But this example will apply to positioned elements also, no?
>
> Absolute positioned, that is.
Nope in the sense that abs-pos element don't need to create a separate clip node
that contains only CSS clip up to the positioning parent.
For example if we have
<div id="A" style="overflow:hidden;">
<div id="B" style="position:absolute; clip:rect(...);">
<div id="C" style="overflow:hidden;">
<div id="D" style="position:absolute; clip:rect(...);">
<div id="E" style="overflow:hidden;">
All the other stuff will be clipped by everything in between the root, that is,
either A or A&B or A&B&C or A&B&C&D or A&B&C&D&E, but only fixed-pos element
says it wants B&D but not A&C&E.
On 2015/09/18 20:28:36, chrishtr wrote: > trchen, pdr, wangxianzhu and I just had a long and productive meeting about this > patch and the larger question of the path towards implementing the correct > property tree states in the DisplayItemList. Here are outcomes (others please > add more if I forgot some): > [...] > 3. More work is needed to implement fragmentation correctly. Tien-Ren has an > idea that it could be easier to implement fragmentation top down from the > pagination container, rather than self-fragmenting. +mstensho for comment. Can you elaborate on this? Are you talking about a full multicol rewrite, where we abandon the idea of representing multicol as one tall single column internally (LayoutMultiColumnFlowThread) and only slice it into proper columns for painting / hit-testing / etc, and instead calculate visual coordinates and dimensions for everything, or what?
On 2015/09/21 at 20:45:21, mstensho wrote: > On 2015/09/18 20:28:36, chrishtr wrote: > > trchen, pdr, wangxianzhu and I just had a long and productive meeting about this > > patch and the larger question of the path towards implementing the correct > > property tree states in the DisplayItemList. Here are outcomes (others please > > add more if I forgot some): > > [...] > > 3. More work is needed to implement fragmentation correctly. Tien-Ren has an > > idea that it could be easier to implement fragmentation top down from the > > pagination container, rather than self-fragmenting. +mstensho for comment. > > Can you elaborate on this? Are you talking about a full multicol rewrite, where we abandon the idea of representing multicol as one tall single column internally (LayoutMultiColumnFlowThread) and only slice it into proper columns for painting / hit-testing / etc, and instead calculate visual coordinates and dimensions for everything, or what? Not sure. Tien-Ren didn't give more details, I just wanted to note that he had an idea he was mulling. Fleshing it out and bringing it to you for feedback would be the next step if the idea made sense.
On 2015/09/19 at 04:07:47, trchen wrote: > On 2015/09/19 01:03:18, chrishtr wrote: > > On 2015/09/19 at 01:03:04, chrishtr wrote: > > > On 2015/09/19 at 00:58:33, trchen wrote: > > > > On 2015/09/19 00:55:08, chrishtr wrote: > > > > > On 2015/09/19 at 00:36:53, trchen wrote: > > > > > > On 2015/09/19 00:15:21, chrishtr wrote: > > > > > > > Re clips vs overflow clips and having to go up to the root to generate > > clip > > > > > > > hierarchies: how about we mark clips and overflow clips differently in > > the > > > > > > > display list? Then even though we might have nested sequences of > > overflow > > > > > clips > > > > > > > and clips, drawing display items can specify whether they obey the > > combined > > > > > > > clip and overflow clip parent hierarchy (non-positioned), or just the > > clip > > > > > one > > > > > > > (position abs or fixed). > > > > > > > > > > > > > > This would allow us to represent the clip or overflow clip display > > items in > > > > > the > > > > > > > DisplayItemList just once. When we then create the clip property tree > > from > > > > > them > > > > > > > DisplayItemList, we can generate two hierarchies: one with clip and > > overflow > > > > > > > clip, and one with just clip. abs/fixed position elements would refer > > to the > > > > > > > latter, otherwise to the former. > > > > > > > > > > > > abs-pos and rel-pos should work the same way as static-pos, they both > > apply > > > > > overflow clips in positioning ancestor chain. > > > > > > > > > > > > fixed-pos is special because it could apply clip outside of positioning > > > > > ancestor chain. > > > > > > > > > > > > How about this case: > > > > > > <div style="position:absolute; clip:rect(...);"> > > > > > > <div style="transform:...;"> > > > > > > <div style="position:absolute; clip:rect(...);"> > > > > > > <div style="position:fixed;"> > > > > > > > > > > > > The fixed-pos element escape the inner clip but not the outer clip. > > > > > > > > > > Why does it escape the inner clip? > > > > > > > > Oops, sorry I made the example wrong. Should be this: > > > > <div style="overflow:hidden;"> > > > > <div style="transform:...;"> > > > > <div style="overflow:hidden;"> > > > > <div style="position:fixed;"> > > > > > > Ah. But this example will apply to positioned elements also, no? > > > > Absolute positioned, that is. > > Nope in the sense that abs-pos element don't need to create a separate clip node that contains only CSS clip up to the positioning parent. > For example if we have > <div id="A" style="overflow:hidden;"> > <div id="B" style="position:absolute; clip:rect(...);"> > <div id="C" style="overflow:hidden;"> > <div id="D" style="position:absolute; clip:rect(...);"> > <div id="E" style="overflow:hidden;"> > > All the other stuff will be clipped by everything in between the root, that is, either A or A&B or A&B&C or A&B&C&D or A&B&C&D&E, but only fixed-pos element says it wants B&D but not A&C&E. I don't think that's right. Div D will only be clipped by div B, and not by A and C.
On 2015/09/21 22:14:58, chrishtr wrote:
> On 2015/09/19 at 04:07:47, trchen wrote:
> > On 2015/09/19 01:03:18, chrishtr wrote:
> > > On 2015/09/19 at 01:03:04, chrishtr wrote:
> > > > On 2015/09/19 at 00:58:33, trchen wrote:
> > > > > On 2015/09/19 00:55:08, chrishtr wrote:
> > > > > > On 2015/09/19 at 00:36:53, trchen wrote:
> > > > > > > On 2015/09/19 00:15:21, chrishtr wrote:
> > > > > > > > Re clips vs overflow clips and having to go up to the root to
> generate
> > > clip
> > > > > > > > hierarchies: how about we mark clips and overflow clips
> differently in
> > > the
> > > > > > > > display list? Then even though we might have nested sequences of
> > > overflow
> > > > > > clips
> > > > > > > > and clips, drawing display items can specify whether they obey
> the
> > > combined
> > > > > > > > clip and overflow clip parent hierarchy (non-positioned), or
just
> the
> > > clip
> > > > > > one
> > > > > > > > (position abs or fixed).
> > > > > > > >
> > > > > > > > This would allow us to represent the clip or overflow clip
display
> > > items in
> > > > > > the
> > > > > > > > DisplayItemList just once. When we then create the clip property
> tree
> > > from
> > > > > > them
> > > > > > > > DisplayItemList, we can generate two hierarchies: one with clip
> and
> > > overflow
> > > > > > > > clip, and one with just clip. abs/fixed position elements would
> refer
> > > to the
> > > > > > > > latter, otherwise to the former.
> > > > > > >
> > > > > > > abs-pos and rel-pos should work the same way as static-pos, they
> both
> > > apply
> > > > > > overflow clips in positioning ancestor chain.
> > > > > > >
> > > > > > > fixed-pos is special because it could apply clip outside of
> positioning
> > > > > > ancestor chain.
> > > > > > >
> > > > > > > How about this case:
> > > > > > > <div style="position:absolute; clip:rect(...);">
> > > > > > > <div style="transform:...;">
> > > > > > > <div style="position:absolute; clip:rect(...);">
> > > > > > > <div style="position:fixed;">
> > > > > > >
> > > > > > > The fixed-pos element escape the inner clip but not the outer
clip.
> > > > > >
> > > > > > Why does it escape the inner clip?
> > > > >
> > > > > Oops, sorry I made the example wrong. Should be this:
> > > > > <div style="overflow:hidden;">
> > > > > <div style="transform:...;">
> > > > > <div style="overflow:hidden;">
> > > > > <div style="position:fixed;">
> > > >
> > > > Ah. But this example will apply to positioned elements also, no?
> > >
> > > Absolute positioned, that is.
> >
> > Nope in the sense that abs-pos element don't need to create a separate clip
> node that contains only CSS clip up to the positioning parent.
> > For example if we have
> > <div id="A" style="overflow:hidden;">
> > <div id="B" style="position:absolute; clip:rect(...);">
> > <div id="C" style="overflow:hidden;">
> > <div id="D" style="position:absolute; clip:rect(...);">
> > <div id="E" style="overflow:hidden;">
> >
> > All the other stuff will be clipped by everything in between the root, that
> is, either A or A&B or A&B&C or A&B&C&D or A&B&C&D&E, but only fixed-pos
element
> says it wants B&D but not A&C&E.
>
> I don't think that's right. Div D will only be clipped by div B, and not by A
> and C.
Sorry I made another wrong example. :(
The point I'm trying to make is that except fixed-pos elements, an element will
be affected by an overflow clip or CSS clip if and only if the clip is in the
positioning ancestor chain. The example is bad because ABCDE does not actually
make a positioning chain.
The positioning tree and paint order tree looks like this in the above example:
(positioning tree)
root-+-->A---->C---->E
+-->B---->D
(paint order tree)
root-+-->A
+-->B---->C
+-->D---->E
Let's say we added a fixed-pos element F in the innermost, F will be affected by
clip B D but its only positioning ancestor is the root. Whereas, if we added an
abs-pos element G in the innermost, it will be affected by clip B D because only
B and D are its positioning ancestor.
Let's look at another example (which I originally intended to make):
<div id="A" style="overflow:hidden;"><div id="A1" style="position:relative;">
<div id="B" style="position:absolute; clip:rect(...);">
<div id="C" style="overflow:hidden;"><div id="C1" style="position:relative;">
<div id="D" style="position:absolute; clip:rect(...);">
<div id="E" style="overflow:hidden;"><div id="E1"
style="position:relative;">
(positioning tree)
root-+-->A---->A1---->B---->C---->C1---->D---->E---->E1
(paint order tree)
root-+-->A
+-->A1
+-->B---->C
+-->C1
+-->D---->E
+-->E1
Now let's say we added a fixed-pos element F in the innermost, F will be
affected by clip B D but its only positioning ancestor is the root. Whereas, if
we added an abs-pos element G in the innermost, it will be affected by all clip
because everything is in the clipping ancestor chain.
On 2015/09/21 20:45:21, mstensho wrote: > On 2015/09/18 20:28:36, chrishtr wrote: > > trchen, pdr, wangxianzhu and I just had a long and productive meeting about > this > > patch and the larger question of the path towards implementing the correct > > property tree states in the DisplayItemList. Here are outcomes (others please > > add more if I forgot some): > > [...] > > 3. More work is needed to implement fragmentation correctly. Tien-Ren has an > > idea that it could be easier to implement fragmentation top down from the > > pagination container, rather than self-fragmenting. +mstensho for comment. > > Can you elaborate on this? Are you talking about a full multicol rewrite, where > we abandon the idea of representing multicol as one tall single column > internally (LayoutMultiColumnFlowThread) and only slice it into proper columns > for painting / hit-testing / etc, and instead calculate visual coordinates and > dimensions for everything, or what? I don't intend to change how we layout multicol. Laying-out multicol as a variable-width block-formatting context is perfectly fine. I would imagine much of the logic will be similar to how float is handled. The thing I'm trying to change is how we traverse and paint a fragmented layer. I feel it is very unnatural that a paginating layer does nothing special, while normal descendant layers have to figure out whether it is fragmented by some ancestor. Actually we do have some bugs in that area, for example, the compositing (Skia, not cc) bound of filters consider only the first fragment: http://jsbin.com/cuwoni/ [#1] I strongly feel it should be the paginating layer that setup the proper coordinate conversion, and the descendants should paint just normally. This will allow easier transition toward a more efficient multicol rendering model. For example, we can start with a naive implementation that is equivalent to what we have today: using scoped display items and recur each child multiple times depends on the number of fragments. This has a few drawbacks: 1. Subtree caching will be disabled. 2. If overflow scroll is involved, the recording rect of each fragment needs to be expanded by the interest rect, resulting huge recording in each fragment and a lots of duplicated drawing display items. If it becomes performance issue in practice, we may decide to implement display items for non-linear transform (fragmenting), potentially as an effect node. Then we can record the subtree only once, but raster multiple times. Or further more cc may decide to layerize it, so the subtree will be raster on a render surface but to be drawn multiple times. Note #1: Also it is unclear on the spec how a fragmented stacking context should be rastered. Should it be rastered as a whole, with stacking context effects applied, then fragmented; or should it be rastered in fragments then apply effects at last? Note #2: The spec doesn't say whether multicol establishes stacking context. Blink/WebKit creates pseudo stacking context for it.
On 2015/09/21 23:33:20, trchen wrote: > On 2015/09/21 20:45:21, mstensho wrote: > > On 2015/09/18 20:28:36, chrishtr wrote: > > > trchen, pdr, wangxianzhu and I just had a long and productive meeting about > > this > > > patch and the larger question of the path towards implementing the correct > > > property tree states in the DisplayItemList. Here are outcomes (others > please > > > add more if I forgot some): > > > [...] > > > 3. More work is needed to implement fragmentation correctly. Tien-Ren has an > > > idea that it could be easier to implement fragmentation top down from the > > > pagination container, rather than self-fragmenting. +mstensho for comment. > > > > Can you elaborate on this? Are you talking about a full multicol rewrite, > where > > we abandon the idea of representing multicol as one tall single column > > internally (LayoutMultiColumnFlowThread) and only slice it into proper columns > > for painting / hit-testing / etc, and instead calculate visual coordinates and > > dimensions for everything, or what? > > I don't intend to change how we layout multicol. Laying-out multicol as a > variable-width block-formatting context is perfectly fine. I would imagine much > of the logic will be similar to how float is handled. Not sure what you mean by "variable width". All columns have the same width. > The thing I'm trying to change is how we traverse and paint a fragmented layer. > I feel it is very unnatural that a paginating layer does nothing special, while > normal descendant layers have to figure out whether it is fragmented by some > ancestor. Actually we do have some bugs in that area, for example, the > compositing (Skia, not cc) bound of filters consider only the first fragment: > http://jsbin.com/cuwoni/ [#1] > > I strongly feel it should be the paginating layer that setup the proper > coordinate conversion, and the descendants should paint just normally. This will > allow easier transition toward a more efficient multicol rendering model. > > For example, we can start with a naive implementation that is equivalent to what > we have today: using scoped display items and recur each child multiple times > depends on the number of fragments. This has a few drawbacks: 1. Subtree caching > will be disabled. 2. If overflow scroll is involved, the recording rect of each > fragment needs to be expanded by the interest rect, resulting huge recording in > each fragment and a lots of duplicated drawing display items. > > If it becomes performance issue in practice, we may decide to implement display > items for non-linear transform (fragmenting), potentially as an effect node. > Then we can record the subtree only once, but raster multiple times. Or further > more cc may decide to layerize it, so the subtree will be raster on a render > surface but to be drawn multiple times. I'm not sure if I understand all this yet, but I'd like to point out that a child layer of a multicol container isn't necessarily fragmented, or affected by the multicol container. For example: absolutely positioned layers whose containing block is outside the multicol container, or fixed positioned layers. > Note #1: Also it is unclear on the spec how a fragmented stacking context should > be rastered. Should it be rastered as a whole, with stacking context effects > applied, then fragmented; or should it be rastered in fragments then apply > effects at last? https://drafts.csswg.org/css-break/#transforms Does this answer your question? > Note #2: The spec doesn't say whether multicol establishes stacking context. > Blink/WebKit creates pseudo stacking context for it. A multicol container shouldn't establish a stacking context.
I think this CL should be ok with the referenced optimizations, and also a plan for implementing fragmentation. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:581: DeprecatedPaintLayerPainter::PaintResult DeprecatedPaintLayerPainter::paintChildrenWithFullScrollClipChain(unsigned childrenToVisit, GraphicsContext* context, const DeprecatedPaintLayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags) Let's implement an optimization here: between painting successive child DPLs, don't pop the clip or scroll chain off the stack. Instead, keep them around to potentially reuse if the subsequent child has the same clip/scroll chain. This will optimize a probably common scenario of multiple children with the same clip/scroll pattern. Another optimization I think you should add is to just push the clip and scroll items on the stack before any recursion at all if the current DPL is a stacking context. https://codereview.chromium.org/1284203004/diff/320001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:591: // TODO(trchen): Fixed position needs to be implemented here. On 2015/09/17 at 18:07:53, chrishtr wrote: > We need a plan for how to do it before committing this patch. On reflection, I think fixed position shouldn't be super hard, right? Just compute whether clips and scrolls apply to it. Not requiring it be implemented here, but checking.
On 2015/09/22 12:48:14, mstensho wrote: > On 2015/09/21 23:33:20, trchen wrote: > > On 2015/09/21 20:45:21, mstensho wrote: > > > On 2015/09/18 20:28:36, chrishtr wrote: > > > > trchen, pdr, wangxianzhu and I just had a long and productive meeting > about > > > this > > > > patch and the larger question of the path towards implementing the correct > > > > property tree states in the DisplayItemList. Here are outcomes (others > > please > > > > add more if I forgot some): > > > > [...] > > > > 3. More work is needed to implement fragmentation correctly. Tien-Ren has > an > > > > idea that it could be easier to implement fragmentation top down from the > > > > pagination container, rather than self-fragmenting. +mstensho for comment. > > > > > > Can you elaborate on this? Are you talking about a full multicol rewrite, > > where > > > we abandon the idea of representing multicol as one tall single column > > > internally (LayoutMultiColumnFlowThread) and only slice it into proper > columns > > > for painting / hit-testing / etc, and instead calculate visual coordinates > and > > > dimensions for everything, or what? > > > > I don't intend to change how we layout multicol. Laying-out multicol as a > > variable-width block-formatting context is perfectly fine. I would imagine > much > > of the logic will be similar to how float is handled. > > Not sure what you mean by "variable width". All columns have the same width. Oops you're right, I was thinking about css3-regions. Anyway I think the existing layout logic works great. > > The thing I'm trying to change is how we traverse and paint a fragmented > layer. > > I feel it is very unnatural that a paginating layer does nothing special, > while > > normal descendant layers have to figure out whether it is fragmented by some > > ancestor. Actually we do have some bugs in that area, for example, the > > compositing (Skia, not cc) bound of filters consider only the first fragment: > > http://jsbin.com/cuwoni/ [#1] > > > > I strongly feel it should be the paginating layer that setup the proper > > coordinate conversion, and the descendants should paint just normally. This > will > > allow easier transition toward a more efficient multicol rendering model. > > > > For example, we can start with a naive implementation that is equivalent to > what > > we have today: using scoped display items and recur each child multiple times > > depends on the number of fragments. This has a few drawbacks: 1. Subtree > caching > > will be disabled. 2. If overflow scroll is involved, the recording rect of > each > > fragment needs to be expanded by the interest rect, resulting huge recording > in > > each fragment and a lots of duplicated drawing display items. > > > > If it becomes performance issue in practice, we may decide to implement > display > > items for non-linear transform (fragmenting), potentially as an effect node. > > Then we can record the subtree only once, but raster multiple times. Or > further > > more cc may decide to layerize it, so the subtree will be raster on a render > > surface but to be drawn multiple times. > > I'm not sure if I understand all this yet, but I'd like to point out that a > child layer of a multicol container isn't necessarily fragmented, or affected by > the multicol container. For example: absolutely positioned layers whose > containing block is outside the multicol container, or fixed positioned layers. Yep in some sense a multicol container is similar to an overflow scroll. They both create two CSS boxes, one for in-flow and one for out-of-flow descendants. They both don't create stacking context. Fixed-pos elements escape them both. > > Note #1: Also it is unclear on the spec how a fragmented stacking context > should > > be rastered. Should it be rastered as a whole, with stacking context effects > > applied, then fragmented; or should it be rastered in fragments then apply > > effects at last? > > https://drafts.csswg.org/css-break/#transforms > Does this answer your question? Thanks for the info! Blink currently implements it incorrectly then. > > Note #2: The spec doesn't say whether multicol establishes stacking context. > > Blink/WebKit creates pseudo stacking context for it. > > A multicol container shouldn't establish a stacking context. I think so. If the spec doesn't say so, we should assume it doesn't. Blink treat it as a pseudo stacking context though. Example: http://jsbin.com/musecup/
Closing because we are doing an alternative implementation to add a pre-paint phase to generate property trees. |
