Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1294233004:
Subtree caching implementation in blink-core (Closed)
|
Created:
5 years, 4 months ago by Xianzhu Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-rendering, Rik, danakj, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, Justin Novosad, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSubtree caching implementation in blink-core
Add DeprecatedPaintLayer::needsRepaint flag to indicate the repaint
requirements of layers. The flags are checked and cleared in DPL::paintLayer().
BUG=410097
TEST=DisplayItemListPaintTestForSlimmingPaintV2.SubsequenceCache
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201414
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : #Patch Set 4 : Enable SPv2 #Patch Set 5 : #Patch Set 6 : s/setSelfNeedsRepaint/setNeedsRepaint/ #
Total comments: 30
Patch Set 7 : Address pdr's comments (will add tests in the next patch) #Patch Set 8 : Remove return value of clearRepaintFlagsRecursively #Patch Set 9 : Ready for review #
Total comments: 4
Patch Set 10 : Add a complex-subtree-update test #
Total comments: 4
Patch Set 11 : Based on DPL and simplified subsequence #Patch Set 12 : Unittest #
Total comments: 8
Patch Set 13 : #
Total comments: 2
Patch Set 14 : Avoid FrameView changes #Patch Set 15 : Avoid additional lifecycle methods #Patch Set 16 : Don't mark non-self-painting layers for needsRepaint #
Messages
Total messages: 43 (9 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
pdr@chromium.org changed reviewers: + trchen@chromium.org
+reviewer trchen, could you please take a look too? Lots of comments below but I like this patch overall. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1205: ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); Can we assert that we're in the appropriate document lifecycle phase here and below? https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1221: void LayoutObject::markContainerChainForRepaint() markContainerChainForChildNeedsRepaint? https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:120: class NonLayoutObjectDisplayItemClientWrapper : public DisplayItemClientWrapper { I'd prefer not to add this, but it may be needed. Don't non-layout objects also need a function like setNeedsRepaint()? https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:1074: bool selfNeedsRepaint() const { return m_bitfields.selfNeedsRepaint(); } Can you add a comment here explaining this a bit? // Slimming paint v2 tracks whether a LayoutObject is invalid instead of using invalidation rects. // For more information, see: https://docs.google.com/document/d/1FTqk3MwUAS1qLJsnA9zgfDbeqAQBdLYC4GJ9jpN-x... https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:262: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, SubtreeCache) Can you add a few more tests of updating subtrees? For example, subtrees where one phase is empty or becomes empty, and nested subtrees. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... File Source/core/paint/LayoutObjectSubtreeRecorder.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:16: // Convenience wrapper of SubtreeRecorder for LayoutObject painters. Do we need subtree recorders for anything but layout objects? It would be nice if we could combine these. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:46: bool m_canUseCache; I think we can reuse the pointer in m_subtreeRecorder instead of needing this extra boolean. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:47: #if ENABLE(ASSERT) Oops https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemList.cpp:356: m_validlyCachedClientsDirty = true; Are we still planning on removing m_validlyCachedClientsDirty and m_validlyCachedClients? https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemList.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemList.h:172: // Stores indices to BeginSubtree display items for subtreeCacheIsValid(). Instead of tracking this off to the side, can we store this on the layout object itself? For example, if the layoutobject doesn't need a child repaint, can we assume the subtree is valid? https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... File Source/platform/graphics/paint/SubtreeRecorder.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/SubtreeRecorder.cpp:16: bool SubtreeRecorder::useCachedSubtreeIfPossible(GraphicsContext& context, const DisplayItemClientWrapper& client, int paintPhase) int paintPhase -> PaintPhase phase, or maybe unsigned? https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/SubtreeRecorder.cpp:30: // Append CachedSubtree display item only if we have cached the subtree. WDYT about disabling the empty pair optimization for subtrees instead of doing this extra check?
You can review the patch before I add tests in the next patch. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1205: ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); On 2015/08/20 22:45:05, pdr wrote: > Can we assert that we're in the appropriate document lifecycle phase here and > below? Done. The condition will change in next steps for combining paint invalidation into painting and combining paint invalidation flags with repaint flags. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1221: void LayoutObject::markContainerChainForRepaint() On 2015/08/20 22:45:05, pdr wrote: > markContainerChainForChildNeedsRepaint? Done. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:120: class NonLayoutObjectDisplayItemClientWrapper : public DisplayItemClientWrapper { On 2015/08/20 22:45:05, pdr wrote: > I'd prefer not to add this, but it may be needed. Don't non-layout objects also > need a function like setNeedsRepaint()? In next steps, I'll try to decouple setting of repaint flags from invalidateDisplayItemClient(), then whether the display item client is a LayoutObject will be no longer matter. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:1074: bool selfNeedsRepaint() const { return m_bitfields.selfNeedsRepaint(); } On 2015/08/20 22:45:05, pdr wrote: > Can you add a comment here explaining this a bit? > > // Slimming paint v2 tracks whether a LayoutObject is invalid instead of using > invalidation rects. > // For more information, see: > https://docs.google.com/document/d/1FTqk3MwUAS1qLJsnA9zgfDbeqAQBdLYC4GJ9jpN-x... Done. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:262: TEST_F(DisplayItemListPaintTestForSlimmingPaintV2, SubtreeCache) On 2015/08/20 22:45:06, pdr wrote: > Can you add a few more tests of updating subtrees? For example, subtrees where > one phase is empty or becomes empty, and nested subtrees. This seems to test mainly the merge algorithm about subtrees, right? I'd like to add some tests in DisplayItemListTest which can be better controlled. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... File Source/core/paint/LayoutObjectSubtreeRecorder.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:16: // Convenience wrapper of SubtreeRecorder for LayoutObject painters. On 2015/08/20 22:45:06, pdr wrote: > Do we need subtree recorders for anything but layout objects? It would be nice > if we could combine these. Done. Previously I moved SubtreeRecorder from core/paint to platform/graphics/paint for testing subtree merging algorithm in DisplayItemListTest. Added SimpleSubtreeRecorder in the test just to issue BeginSubtree/EndSubtree display items so that the test no longer requires SubtreeRedorder. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:46: bool m_canUseCache; On 2015/08/20 22:45:06, pdr wrote: > I think we can reuse the pointer in m_subtreeRecorder instead of needing this > extra boolean. Done. https://codereview.chromium.org/1294233004/diff/100001/Source/core/paint/Layo... Source/core/paint/LayoutObjectSubtreeRecorder.h:47: #if ENABLE(ASSERT) On 2015/08/20 22:45:06, pdr wrote: > Oops Done. https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemList.cpp:356: m_validlyCachedClientsDirty = true; On 2015/08/20 22:45:06, pdr wrote: > Are we still planning on removing m_validlyCachedClientsDirty and > m_validlyCachedClients? I'm thinking of this, but not sure for now about: 1. non-LayoutObject clients: they still need the set to check for validity of cache; 2. deleted objects (including LayoutObjects). For 1, without the set, we can treat them as non-cacheable or to duplicate the same logic for LayoutObjects. Perhaps we can do the latter for LineBoxes, and the former for all other clients. For 2, I'm not sure how deleted objects will be represented in the diff passed to cc. The diff must be able to convey the information of deleted display items, as new display items of new objects may use the same ids. Perhaps we can use a m_detetedClients set for the need. https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... File Source/platform/graphics/paint/SubtreeRecorder.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... Source/platform/graphics/paint/SubtreeRecorder.cpp:30: // Append CachedSubtree display item only if we have cached the subtree. On 2015/08/20 22:45:06, pdr wrote: > WDYT about disabling the empty pair optimization for subtrees instead of doing > this extra check? That will generate empty subtrees for paint phases that produces nothing, e.g. PaintPhaseOutline phase for a subtree containing no object having outline. PaintPhaseSelection, PaintPhaseMask, etc. also have such problems.
https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1225: container->m_bitfields.setChildNeedsRepaint(true); I think we need to special case fixed-pos elements. If I understand correctly, we want to mark all ancestors in the painting order, but instead this marks all ancestors in the positioning order. For most element isPaintingAncestor(o) implies isPositionAncestor(o). Fixed-pos is the only exception. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:120: class NonLayoutObjectDisplayItemClientWrapper : public DisplayItemClientWrapper { On 2015/08/20 22:45:05, pdr wrote: > I'd prefer not to add this, but it may be needed. Don't non-layout objects also > need a function like setNeedsRepaint()? It doesn't seem that it is used in this CL. Shall we add it in another CL when it is actually used? https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:1080: bool clearRepaintFlagsRecursively(); In this CL it seems the return value will always be true. Is it reserved for future use?
https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1225: container->m_bitfields.setChildNeedsRepaint(true); On 2015/08/21 00:37:29, trchen wrote: > I think we need to special case fixed-pos elements. > > If I understand correctly, we want to mark all ancestors in the painting order, > but instead this marks all ancestors in the positioning order. For most element > isPaintingAncestor(o) implies isPositionAncestor(o). Fixed-pos is the only > exception. container() is actually equivalent to paintingAncestor(). It has a special case for fixed position. If not, the original paint invalidation would have been incorrect. However, I need to confirm the paint invalidation traversal sequence (which is also the clearRepaintFlagsRecursively traversal sequence) about fixed-pos. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:120: class NonLayoutObjectDisplayItemClientWrapper : public DisplayItemClientWrapper { On 2015/08/21 00:37:29, trchen wrote: > On 2015/08/20 22:45:05, pdr wrote: > > I'd prefer not to add this, but it may be needed. Don't non-layout objects > also > > need a function like setNeedsRepaint()? > Currently non-LayoutObject clients depend on DisplayItemList::validlyCachedClients for paint invalidation. Thoughts here: https://codereview.chromium.org/1294233004/diff/100001/Source/platform/graphi... > It doesn't seem that it is used in this CL. Shall we add it in another CL when > it is actually used? It's used by invalidateDisplayItemClient() to distinguish non-LayoutObject and LayoutObject clients. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:1080: bool clearRepaintFlagsRecursively(); On 2015/08/21 00:37:29, trchen wrote: > In this CL it seems the return value will always be true. Is it reserved for > future use? Will remove the return value first.
https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1225: container->m_bitfields.setChildNeedsRepaint(true); On 2015/08/21 04:54:51, Xianzhu wrote: > On 2015/08/21 00:37:29, trchen wrote: > > I think we need to special case fixed-pos elements. > > > > If I understand correctly, we want to mark all ancestors in the painting > order, > > but instead this marks all ancestors in the positioning order. For most > element > > isPaintingAncestor(o) implies isPositionAncestor(o). Fixed-pos is the only > > exception. > > container() is actually equivalent to paintingAncestor(). It has a special case > for fixed position. If not, the original paint invalidation would have been > incorrect. > > However, I need to confirm the paint invalidation traversal sequence (which is > also the clearRepaintFlagsRecursively traversal sequence) about fixed-pos. Scratch the above. Perhaps I misunderstood painting order about fixed-pos. Will investigate more. https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1294233004/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutObject.h:1080: bool clearRepaintFlagsRecursively(); On 2015/08/21 04:54:51, Xianzhu wrote: > On 2015/08/21 00:37:29, trchen wrote: > > In this CL it seems the return value will always be true. Is it reserved for > > future use? > > Will remove the return value first. Done.
Sorry, I just found some bug in SubtreeRecorder, and also need to investigate the paint order issue. Please hold off on review.
The latest patch is ready for review. Ptal. The tree traversal in painting-order issue is not significant for now because we still have DeprecatedPaintLayer to handle it. Painting of the root layout object of a DeprecatedPaintLayer is not controlled by subtree recorders for now. Added TODOs in the code.
Overall, this is great. This still feels a little under tested. https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemList.cpp:198: // We have indexed all BeginSubtree display items, so don't need to find forward. Should this be a few lines up or do we need to call findMatchingItemForIndex? https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemListTest.cpp (right): https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemListTest.cpp:33: private: Should these be const?
Added DisplayItemListTest.CachedNestedSubtreeUpdate unit test. https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemList.cpp:198: // We have indexed all BeginSubtree display items, so don't need to find forward. On 2015/08/24 20:30:10, pdr wrote: > Should this be a few lines up or do we need to call findMatchingItemForIndex? Rearranged the statements and rephrased the comments. https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... File Source/platform/graphics/paint/DisplayItemListTest.cpp (right): https://codereview.chromium.org/1294233004/diff/160001/Source/platform/graphi... Source/platform/graphics/paint/DisplayItemListTest.cpp:33: private: On 2015/08/24 20:30:10, pdr wrote: > Should these be const? Done.
LGTM Pretty cool to see subtree caching implemented without adding much complexity to the core algorithms. CommitNewDisplayItems is now even easier to understand.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294233004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294233004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1231: // object which is not fully controlled by normal LayoutBlock subtree repaint flags. Sorry I just realized that fixed-pos element is not the only thing that breaks the invariant "being painting ancestor implies being position ancestor". There are tons of things that creates stacking context without being position container, for example, opacity. I think it wouldn't be too hard to add a helper function to find painting parent?
https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1231: // object which is not fully controlled by normal LayoutBlock subtree repaint flags. On 2015/08/25 06:30:38, trchen wrote: > Sorry I just realized that fixed-pos element is not the only thing that breaks > the invariant "being painting ancestor implies being position ancestor". There > are tons of things that creates stacking context without being position > container, for example, opacity. > > I think it wouldn't be too hard to add a helper function to find painting > parent? This is not too hard, but it'll need non-constant time to find the contained descendants (needed in clearRepaintFlagsRecursively). For now the other non-position-container cases seem similar to fixed-positions which are handled by DeprecatedPaintLayer. I'd like to handle this in later CLs. Will update the comment.
On 2015/08/25 15:55:03, Xianzhu wrote: > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > File Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > Source/core/layout/LayoutObject.cpp:1231: // object which is not fully > controlled by normal LayoutBlock subtree repaint flags. > On 2015/08/25 06:30:38, trchen wrote: > > Sorry I just realized that fixed-pos element is not the only thing that breaks > > the invariant "being painting ancestor implies being position ancestor". There > > are tons of things that creates stacking context without being position > > container, for example, opacity. > > > > I think it wouldn't be too hard to add a helper function to find painting > > parent? > > This is not too hard, but it'll need non-constant time to find the contained > descendants (needed in clearRepaintFlagsRecursively). For now the other > non-position-container cases seem similar to fixed-positions which are handled > by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > Will update the comment. Changed my mind :) Will add LayoutObject* paintContainer() and let clearRepaintFlagsRecursively iterate all descendants.
On 2015/08/25 16:06:55, Xianzhu wrote: > On 2015/08/25 15:55:03, Xianzhu wrote: > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > File Source/core/layout/LayoutObject.cpp (right): > > > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > Source/core/layout/LayoutObject.cpp:1231: // object which is not fully > > controlled by normal LayoutBlock subtree repaint flags. > > On 2015/08/25 06:30:38, trchen wrote: > > > Sorry I just realized that fixed-pos element is not the only thing that > breaks > > > the invariant "being painting ancestor implies being position ancestor". > There > > > are tons of things that creates stacking context without being position > > > container, for example, opacity. > > > > > > I think it wouldn't be too hard to add a helper function to find painting > > > parent? > > > > This is not too hard, but it'll need non-constant time to find the contained > > descendants (needed in clearRepaintFlagsRecursively). For now the other > > non-position-container cases seem similar to fixed-positions which are handled > > by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > > > Will update the comment. > > Changed my mind :) Will add LayoutObject* paintContainer() and let > clearRepaintFlagsRecursively iterate all descendants. By the way I don' think it needs to traverse all descendants. You only need to look at all direct children that have needPainting flag (and recur). Then look at the DeprecatedPaintLayerStackingNode, if there is one, traverse all the z-order lists.
This CL seems to have two parts: 1. Add repaint flags, which will replace the current paint invalidation flags in phase 2. 2. Implement subtree caching. If that is correct, then this CL should be split in two, with part 1 happening first. Also, regarding subtree caching, the approach taken looks like it has too many performance footguns and too much complexity. Why not just do the following: cache a subtree of a DeprecatedPaintLayer if the layer is a stacking context and nothing inside of it has repaint flags set? Does that break in some situation? If not, it seems a lot simpler and operations related to checking cacheability etc are constant time. https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1231: // object which is not fully controlled by normal LayoutBlock subtree repaint flags. On 2015/08/25 at 15:55:03, Xianzhu wrote: > On 2015/08/25 06:30:38, trchen wrote: > > Sorry I just realized that fixed-pos element is not the only thing that breaks > > the invariant "being painting ancestor implies being position ancestor". There > > are tons of things that creates stacking context without being position > > container, for example, opacity. > > > > I think it wouldn't be too hard to add a helper function to find painting > > parent? > > This is not too hard, but it'll need non-constant time to find the contained descendants (needed in clearRepaintFlagsRecursively). For now the other non-position-container cases seem similar to fixed-positions which are handled by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > Will update the comment. I'm not sure all the complexity of encoding paint relationships that differ from layout tree topology. I think we just need to know if something in the subtree needed repaint, regardless of what its paint ancestor was, because the paint ancestor is always an ancestor at some distance up the tree. https://codereview.chromium.org/1294233004/diff/180001/Source/core/paint/Bloc... File Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1294233004/diff/180001/Source/core/paint/Bloc... Source/core/paint/BlockPainter.cpp:25: #include "core/paint/SubtreeRecorder.h" This file is missing from the CL?
On 2015/08/25 20:49:52, chrishtr wrote: > This CL seems to have two parts: > > 1. Add repaint flags, which will replace the current paint invalidation flags in > phase 2. > > 2. Implement subtree caching. > > If that is correct, then this CL should be split in two, with part 1 happening > first. > > Also, regarding subtree caching, the approach taken looks like it has too many > performance footguns and too much complexity. Why not just do the following: > cache a subtree of a DeprecatedPaintLayer if the layer is a stacking context and > nothing inside of it has repaint flags set? Does that break in some situation? > If not, it seems a lot simpler and operations related to checking cacheability > etc are constant time. This idea looks great because it can avoid many complex logics about subtrees. > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > File Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > Source/core/layout/LayoutObject.cpp:1231: // object which is not fully > controlled by normal LayoutBlock subtree repaint flags. > On 2015/08/25 at 15:55:03, Xianzhu wrote: > > On 2015/08/25 06:30:38, trchen wrote: > > > Sorry I just realized that fixed-pos element is not the only thing that > breaks > > > the invariant "being painting ancestor implies being position ancestor". > There > > > are tons of things that creates stacking context without being position > > > container, for example, opacity. > > > > > > I think it wouldn't be too hard to add a helper function to find painting > > > parent? > > > > This is not too hard, but it'll need non-constant time to find the contained > descendants (needed in clearRepaintFlagsRecursively). For now the other > non-position-container cases seem similar to fixed-positions which are handled > by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > > > Will update the comment. > > I'm not sure all the complexity of encoding paint relationships that differ from > layout tree topology. I think we just need to know if something in the subtree > needed repaint, > regardless of what its paint ancestor was, because the paint ancestor is always > an ancestor at some distance up the tree. > If we mark subtree repaint flags on DeprecatedPaintLayer, then markContainerChainForChildNeedsRepaint and clearRepaintFlagsRecursively will be no longer applicable. We'll automatically use paint relationship. One thing I'm not clear about is about the future of DeprecatedPaintLayer. We'll put more dependencies on it if we take the method discussed here.
On 2015/08/25 21:33:26, Xianzhu wrote: > On 2015/08/25 20:49:52, chrishtr wrote: > > This CL seems to have two parts: > > > > 1. Add repaint flags, which will replace the current paint invalidation flags > in > > phase 2. > > > > 2. Implement subtree caching. > > > > If that is correct, then this CL should be split in two, with part 1 happening > > first. > > > > Also, regarding subtree caching, the approach taken looks like it has too many > > performance footguns and too much complexity. Why not just do the following: > > cache a subtree of a DeprecatedPaintLayer if the layer is a stacking context > and > > nothing inside of it has repaint flags set? Does that break in some situation? > > If not, it seems a lot simpler and operations related to checking cacheability > > etc are constant time. > > This idea looks great because it can avoid many complex logics about subtrees. > > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > File Source/core/layout/LayoutObject.cpp (right): > > > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > Source/core/layout/LayoutObject.cpp:1231: // object which is not fully > > controlled by normal LayoutBlock subtree repaint flags. > > On 2015/08/25 at 15:55:03, Xianzhu wrote: > > > On 2015/08/25 06:30:38, trchen wrote: > > > > Sorry I just realized that fixed-pos element is not the only thing that > > breaks > > > > the invariant "being painting ancestor implies being position ancestor". > > There > > > > are tons of things that creates stacking context without being position > > > > container, for example, opacity. > > > > > > > > I think it wouldn't be too hard to add a helper function to find painting > > > > parent? > > > > > > This is not too hard, but it'll need non-constant time to find the contained > > descendants (needed in clearRepaintFlagsRecursively). For now the other > > non-position-container cases seem similar to fixed-positions which are handled > > by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > > > > > Will update the comment. > > > > I'm not sure all the complexity of encoding paint relationships that differ > from > > layout tree topology. I think we just need to know if something in the subtree > > needed repaint, > > regardless of what its paint ancestor was, because the paint ancestor is > always > > an ancestor at some distance up the tree. > > > > If we mark subtree repaint flags on DeprecatedPaintLayer, then > markContainerChainForChildNeedsRepaint and clearRepaintFlagsRecursively will be > no longer applicable. We'll automatically use paint relationship. > > One thing I'm not clear about is about the future of DeprecatedPaintLayer. We'll > put more dependencies on it if we take the method discussed here. I think it is fine. We are only using the DPLStackingNode, that is, the stacking context tree, which IMO is one thing we still want to pre-compute and cache in the future. It doesn't really have to be tied to DPL. The future I'm imagining is that we will keep DPLStackingNode and DPLScrollableArea as a part of LayoutObject rare data, but kill DPL, DPLClipper, and DPLReflectionInfo. Maybe keep some part of DPL as rare data for position container tree.
On 2015/08/25 at 21:47:31, trchen wrote: > On 2015/08/25 21:33:26, Xianzhu wrote: > > On 2015/08/25 20:49:52, chrishtr wrote: > > > This CL seems to have two parts: > > > > > > 1. Add repaint flags, which will replace the current paint invalidation flags > > in > > > phase 2. > > > > > > 2. Implement subtree caching. > > > > > > If that is correct, then this CL should be split in two, with part 1 happening > > > first. > > > > > > Also, regarding subtree caching, the approach taken looks like it has too many > > > performance footguns and too much complexity. Why not just do the following: > > > cache a subtree of a DeprecatedPaintLayer if the layer is a stacking context > > and > > > nothing inside of it has repaint flags set? Does that break in some situation? > > > If not, it seems a lot simpler and operations related to checking cacheability > > > etc are constant time. > > > > This idea looks great because it can avoid many complex logics about subtrees. > > > > > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > > File Source/core/layout/LayoutObject.cpp (right): > > > > > > > > https://codereview.chromium.org/1294233004/diff/180001/Source/core/layout/Lay... > > > Source/core/layout/LayoutObject.cpp:1231: // object which is not fully > > > controlled by normal LayoutBlock subtree repaint flags. > > > On 2015/08/25 at 15:55:03, Xianzhu wrote: > > > > On 2015/08/25 06:30:38, trchen wrote: > > > > > Sorry I just realized that fixed-pos element is not the only thing that > > > breaks > > > > > the invariant "being painting ancestor implies being position ancestor". > > > There > > > > > are tons of things that creates stacking context without being position > > > > > container, for example, opacity. > > > > > > > > > > I think it wouldn't be too hard to add a helper function to find painting > > > > > parent? > > > > > > > > This is not too hard, but it'll need non-constant time to find the contained > > > descendants (needed in clearRepaintFlagsRecursively). For now the other > > > non-position-container cases seem similar to fixed-positions which are handled > > > by DeprecatedPaintLayer. I'd like to handle this in later CLs. > > > > > > > > Will update the comment. > > > > > > I'm not sure all the complexity of encoding paint relationships that differ > > from > > > layout tree topology. I think we just need to know if something in the subtree > > > needed repaint, > > > regardless of what its paint ancestor was, because the paint ancestor is > > always > > > an ancestor at some distance up the tree. > > > > > > > If we mark subtree repaint flags on DeprecatedPaintLayer, then > > markContainerChainForChildNeedsRepaint and clearRepaintFlagsRecursively will be > > no longer applicable. We'll automatically use paint relationship. > > > > One thing I'm not clear about is about the future of DeprecatedPaintLayer. We'll > > put more dependencies on it if we take the method discussed here. > > I think it is fine. We are only using the DPLStackingNode, that is, the stacking context tree, which IMO is one thing we still want to pre-compute and cache in the future. It doesn't really have to be tied to DPL. > > The future I'm imagining is that we will keep DPLStackingNode and DPLScrollableArea as a part of LayoutObject rare data, but kill DPL, DPLClipper, and DPLReflectionInfo. Maybe keep some part of DPL as rare data for position container tree. +1. Don't worry about dependencies on DPL. The function of it will remain, even if the class itself is refactored away.
The new implementation is based on simplified subsequence and DPL. Ptal.
https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... Source/core/frame/FrameView.h:603: friend class LayoutPart; What is LayoutPart a friend for? Does this deserve a TODO to fix? https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:113: // we can't use it for now because it conflicts with PaintInfo::paintContainer. How about stackingContainerForPaint()? With a comment saying that it's either a stacking context or acts like one in the context of the css 2.1 painting algorithm? https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:87: if (!needsRepaint && SubsequenceRecorder::useCachedSubsequenceIfPossible(*context, *m_paintLayer.layoutObject())) Why aren't you checking for being a stacking context? https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:71: void updateLifecyclePhasesToPaintForSlimmingPaintV2Clean() { document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToPaintForSlimmingPaintV2Clean); } These are just for testing? That's sad. Is it really necessary?
https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... Source/core/frame/FrameView.h:603: friend class LayoutPart; On 2015/08/27 04:29:48, chrishtr wrote: > What is LayoutPart a friend for? Does this deserve a TODO to fix? Done. https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:113: // we can't use it for now because it conflicts with PaintInfo::paintContainer. On 2015/08/27 04:29:48, chrishtr wrote: > How about stackingContainerForPaint()? With a comment saying that it's either a > stacking context or acts like one in the context > of the css 2.1 painting algorithm? stackingContainerForPaint seems incomplete, because for a non-treated-as-stacking-context layer, the function returns the parent layer of any kind, including non-stacking-context overflow-scroll layer. A description might be: the ancestor layer whose paintLayer method will directly call this layer's paintLayer method. https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayerPainter.cpp:87: if (!needsRepaint && SubsequenceRecorder::useCachedSubsequenceIfPossible(*context, *m_paintLayer.layoutObject())) On 2015/08/27 04:29:48, chrishtr wrote: > Why aren't you checking for being a stacking context? Adding condition for stacking context seems just to complicate and cause missing of some chances of relatively independently painted subsequences e.g for non-stacking-context overflow:scroll divs. https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... File Source/core/paint/DisplayItemListPaintTest.cpp (right): https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... Source/core/paint/DisplayItemListPaintTest.cpp:71: void updateLifecyclePhasesToPaintForSlimmingPaintV2Clean() { document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToPaintForSlimmingPaintV2Clean); } On 2015/08/27 04:29:48, chrishtr wrote: > These are just for testing? That's sad. Is it really necessary? The cached display items are only visible in display list after painting and before committing (via newDisplayitemsBeforeUpdate). Without them we can only test the final merged result of display lists and unable to check the cached display items during line 308 to 319 below.
On 2015/08/27 at 16:38:02, wangxianzhu wrote: > https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... > File Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/frame/Fram... > Source/core/frame/FrameView.h:603: friend class LayoutPart; > On 2015/08/27 04:29:48, chrishtr wrote: > > What is LayoutPart a friend for? Does this deserve a TODO to fix? > > Done. > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayer.h (right): > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayer.h:113: // we can't use it for now because it conflicts with PaintInfo::paintContainer. > On 2015/08/27 04:29:48, chrishtr wrote: > > How about stackingContainerForPaint()? With a comment saying that it's either a > > stacking context or acts like one in the context > > of the css 2.1 painting algorithm? > > stackingContainerForPaint seems incomplete, because for a non-treated-as-stacking-context layer, the function returns the parent layer of any kind, including non-stacking-context overflow-scroll layer. > > A description might be: the ancestor layer whose paintLayer method will directly call this layer's paintLayer method. > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayerPainter.cpp:87: if (!needsRepaint && SubsequenceRecorder::useCachedSubsequenceIfPossible(*context, *m_paintLayer.layoutObject())) > On 2015/08/27 04:29:48, chrishtr wrote: > > Why aren't you checking for being a stacking context? > > Adding condition for stacking context seems just to complicate and cause missing of some chances of relatively independently painted subsequences e.g for non-stacking-context overflow:scroll divs. Will non-stacking-context use cases actually work without a ScopeRecorder? Otherwise display item ids will not be unique. > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > File Source/core/paint/DisplayItemListPaintTest.cpp (right): > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > Source/core/paint/DisplayItemListPaintTest.cpp:71: void updateLifecyclePhasesToPaintForSlimmingPaintV2Clean() { document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToPaintForSlimmingPaintV2Clean); } > On 2015/08/27 04:29:48, chrishtr wrote: > > These are just for testing? That's sad. Is it really necessary? > > The cached display items are only visible in display list after painting and before committing (via newDisplayitemsBeforeUpdate). Without them we can only test the final merged result of display lists and unable to check the cached display items during line 308 to 319 below. How about calling the component methods of the lifecycle directly (rather than introducing a new lifecycle advancer that should never actually be used)?
On 2015/08/27 17:06:23, chrishtr wrote: > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > > Source/core/paint/DeprecatedPaintLayerPainter.cpp:87: if (!needsRepaint && > SubsequenceRecorder::useCachedSubsequenceIfPossible(*context, > *m_paintLayer.layoutObject())) > > On 2015/08/27 04:29:48, chrishtr wrote: > > > Why aren't you checking for being a stacking context? > > > > Adding condition for stacking context seems just to complicate and cause > missing of some chances of relatively independently painted subsequences e.g for > non-stacking-context overflow:scroll divs. > > Will non-stacking-context use cases actually work without a ScopeRecorder? > Otherwise display item ids will not be unique. > Are you concerned about the following case? <div id="non-stacking-context-layer" style="overflow: scroll; backface-visibility: hidden; ..."> <div style="position: relative; ..."> <div style="position: absolute; ..."> </div> In the case, non-stacking-context-layer paints its normal flow children only. The relative and absolute children are painted by their compositingContainer which is the layer for the html element. Do you (and trchen@) think of other cases? > > > > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > > File Source/core/paint/DisplayItemListPaintTest.cpp (right): > > > > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > > Source/core/paint/DisplayItemListPaintTest.cpp:71: void > updateLifecyclePhasesToPaintForSlimmingPaintV2Clean() { > document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToPaintForSlimmingPaintV2Clean); > } > > On 2015/08/27 04:29:48, chrishtr wrote: > > > These are just for testing? That's sad. Is it really necessary? > > > > The cached display items are only visible in display list after painting and > before committing (via newDisplayitemsBeforeUpdate). Without them we can only > test the final merged result of display lists and unable to check the cached > display items during line 308 to 319 below. > > How about calling the component methods of the lifecycle directly (rather than > introducing a new lifecycle advancer that should never actually be used)? Done.
On 2015/08/27 at 19:01:03, wangxianzhu wrote: > On 2015/08/27 17:06:23, chrishtr wrote: > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Depr... > > > Source/core/paint/DeprecatedPaintLayerPainter.cpp:87: if (!needsRepaint && > > SubsequenceRecorder::useCachedSubsequenceIfPossible(*context, > > *m_paintLayer.layoutObject())) > > > On 2015/08/27 04:29:48, chrishtr wrote: > > > > Why aren't you checking for being a stacking context? > > > > > > Adding condition for stacking context seems just to complicate and cause > > missing of some chances of relatively independently painted subsequences e.g for > > non-stacking-context overflow:scroll divs. > > > > Will non-stacking-context use cases actually work without a ScopeRecorder? > > Otherwise display item ids will not be unique. > > > > Are you concerned about the following case? > > <div id="non-stacking-context-layer" style="overflow: scroll; backface-visibility: hidden; ..."> > <div style="position: relative; ..."> > <div style="position: absolute; ..."> > </div> > > In the case, non-stacking-context-layer paints its normal flow children only. The relative and absolute children are painted by their compositingContainer which is the layer for the html element. > > Do you (and trchen@) think of other cases? I guess you're right. DPL is set up to paint all content that should be painted within it in one go, via paintLayer. > > > > > > > > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > > > File Source/core/paint/DisplayItemListPaintTest.cpp (right): > > > > > > > > https://codereview.chromium.org/1294233004/diff/220001/Source/core/paint/Disp... > > > Source/core/paint/DisplayItemListPaintTest.cpp:71: void > > updateLifecyclePhasesToPaintForSlimmingPaintV2Clean() { > > document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToPaintForSlimmingPaintV2Clean); > > } > > > On 2015/08/27 04:29:48, chrishtr wrote: > > > > These are just for testing? That's sad. Is it really necessary? > > > > > > The cached display items are only visible in display list after painting and > > before committing (via newDisplayitemsBeforeUpdate). Without them we can only > > test the final merged result of display lists and unable to check the cached > > display items during line 308 to 319 below. > > > > How about calling the component methods of the lifecycle directly (rather than > > introducing a new lifecycle advancer that should never actually be used)? > > Done.
The CQ bit was checked by chrishtr@chromium.org
lgtm https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:114: DeprecatedPaintLayer* compositingContainer() const; How about paintContainer()? Defined as: parent pointer for the paint tree, which is the tree whose topology is the recursion order for painting.
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1294233004/#ps280001 (title: "Avoid additional lifecycle methods")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294233004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294233004/280001
The CQ bit was unchecked by chrishtr@chromium.org
https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... File Source/core/paint/DeprecatedPaintLayer.h (right): https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... Source/core/paint/DeprecatedPaintLayer.h:114: DeprecatedPaintLayer* compositingContainer() const; On 2015/08/27 23:26:15, chrishtr wrote: > How about paintContainer()? Defined as: parent pointer for the paint tree, which > is the tree whose topology is the recursion order for painting. Yes, I think paintContainer is the best choice eventually, but for now it may confuse with paintContainer or paintInvalidationContainer of other places (e.g. PaintInfo::paintContainer) which mean the containing composited layer. I'd like to change this to paintContainer after we remove all paintContainer/paintInvalidationContainer of other places. Wdyt?
On 2015/08/27 at 23:49:08, wangxianzhu wrote: > https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... > File Source/core/paint/DeprecatedPaintLayer.h (right): > > https://codereview.chromium.org/1294233004/diff/240001/Source/core/paint/Depr... > Source/core/paint/DeprecatedPaintLayer.h:114: DeprecatedPaintLayer* compositingContainer() const; > On 2015/08/27 23:26:15, chrishtr wrote: > > How about paintContainer()? Defined as: parent pointer for the paint tree, which > > is the tree whose topology is the recursion order for painting. > > Yes, I think paintContainer is the best choice eventually, but for now it may confuse with paintContainer or paintInvalidationContainer of other places (e.g. PaintInfo::paintContainer) which mean the containing composited layer. I'd like to change this to paintContainer after we remove all paintContainer/paintInvalidationContainer of other places. Wdyt? ok
The newly added virtual/spv2 virtual suite discovered some bugs in this CL. Investigating.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1294233004/#ps300001 (title: "Don't mark non-self-painting layers for needsRepaint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294233004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294233004/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201414 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
