Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1651153003:
[SPv2] Adds pre-computed paint property context to PaintLayer. (Closed)
|
Created:
4 years, 10 months ago by trchen Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[SPv2] Adds pre-computed paint property context to PaintLayer.
This is part #1 of the CLs to associate overflow clip/scroll nodes to
display items.
This CL adds pre-computed paint context to PaintLayer, so a layer won't need
to traverse the positional ancestor chain (which is different from paint
ancestor chain) to find the nearest clip node.
Committed: https://crrev.com/e849a2e8bf5bb255524af4fb27a7ae2f652fa123
Cr-Commit-Position: refs/heads/master@{#373718}
Patch Set 1 #
Total comments: 10
Patch Set 2 : rename & add comments #Patch Set 3 : migrated the cache to ObjectPaintProperties #
Total comments: 7
Patch Set 4 : added tests. revised comments. renamed to recordTreeContextIfNeeded. #
Total comments: 15
Patch Set 5 : renamed the cache. revised comments. #
Dependent Patchsets: Messages
Total messages: 41 (12 generated)
trchen@chromium.org changed reviewers: + jbroman@chromium.org, pdr@chromium.org
[1/5] Split out of my previous CL.
https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:138: } cachedSPv2PaintParameters; Is this data actually rare or is it just on raredata temporarily? Can you add a short comment above CachedSPv2PaintParameters explaining what it's for? We can name this something better than CachedSPv2PaintParameters, but I don't understand this patch well enough to make a suggestion. If this is just for overflow clip, maybe something like CachedSPv2OverflowClipPaintState? https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:688: const PaintLayerRareData::CachedSPv2PaintParameters& cachedSPv2PaintParameters() const { ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); ASSERT(m_rareData); return m_rareData->cachedSPv2PaintParameters; } WDYT about some lifecycle asserts here? For the accessor, something like ASSERT(DocumentLifecycle::InPaint)? For the setter, something like ASSERT(DocumentLifecycle::InUpdatePaintProperties)? https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:313: void cachePropertyIfPaintLayerPresents(LayoutObject& object, const PaintPropertyTreeBuilderContext& context) Nit: cachePropertyIfPaintLayerPresent (because "if present" is singular) https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:333: cachePropertyIfPaintLayerPresents(object, localContext); I think what you're trying to do here is to store off the "paint property state of the world" just before applying the overflow clip--is that correct? Overflow clip doesn't affect transform though... why do we cache the transform paint property instead of using PaintLayer.layoutObject->objectPaintProperties().transform or ClipPaintPropertyNode.localTransformSpace? https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:334: RefPtr<ClipPaintPropertyNode> newClipNodeForOverflowClip = createOverflowClipIfNeeded(object, localContext); This creates clips for both border radius and overflow clip. Does this patch need to consider the border radius clip created before overflow clip?
What do you mean by "collecting overflow clips"? Isn't the goal that we can just point at a clip node at paint time, rather than collecting a list of clips?
Description was changed from ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to collect overflow clips. ========== to ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to find the nearest clip node. ==========
On 2016/02/02 16:11:13, jbroman wrote: > What do you mean by "collecting overflow clips"? Isn't the goal that we can just > point at a clip node at paint time, rather than collecting a list of clips? To find the nearest containing block ancestor that has non-null clip node. Revised.
https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:138: } cachedSPv2PaintParameters; On 2016/02/02 06:18:20, pdr wrote: > Is this data actually rare or is it just on raredata temporarily? It will be no longer rare once SPv2 ships. :) > Can you add a short comment above CachedSPv2PaintParameters explaining what it's > for? Acknowledged. > We can name this something better than CachedSPv2PaintParameters, but I don't > understand this patch well enough to make a suggestion. If this is just for > overflow clip, maybe something like CachedSPv2OverflowClipPaintState? Yea... The variable name is too generic. Having the code name SPv2 in there isn't good either. However it is not exclusive for overflow either, as I'm planning to include paint context for fixed-pos descendants in the future (for background-attachment:fixed). Does CachedLayerPaintPropertyContext sound better? https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:688: const PaintLayerRareData::CachedSPv2PaintParameters& cachedSPv2PaintParameters() const { ASSERT(RuntimeEnabledFeatures::slimmingPaintV2Enabled()); ASSERT(m_rareData); return m_rareData->cachedSPv2PaintParameters; } On 2016/02/02 06:18:20, pdr wrote: > WDYT about some lifecycle asserts here? > > For the accessor, something like ASSERT(DocumentLifecycle::InPaint)? > > For the setter, something like > ASSERT(DocumentLifecycle::InUpdatePaintProperties)? Acknowledged. Hey, fantastic idea! https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:313: void cachePropertyIfPaintLayerPresents(LayoutObject& object, const PaintPropertyTreeBuilderContext& context) On 2016/02/02 06:18:20, pdr wrote: > Nit: cachePropertyIfPaintLayerPresent (because "if present" is singular) Acknowledged. https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:333: cachePropertyIfPaintLayerPresents(object, localContext); On 2016/02/02 06:18:20, pdr wrote: > I think what you're trying to do here is to store off the "paint property state > of the world" just before applying the overflow clip--is that correct? > > Overflow clip doesn't affect transform though... why do we cache the transform > paint property instead of using > PaintLayer.layoutObject->objectPaintProperties().transform or > ClipPaintPropertyNode.localTransformSpace? Essentially it is the state for painting the stacking context. Overflow clip applies at the normal flow level, thus not cached here. For example, if we have a stacking-context scroller: <div style="overflow:scroll; position:relative; z-index:0;"> Lorem ipsum </div> The box decoration is not scrolled, but is painted by this layer. That's why we don't include overflow clip in the cache. "Lorem ipsum" will be painted by normal flow, and the overflow clip will be applied by BoxClipper/BlockPainter in my 5th CL. The cache still serves a purpose though. Let's look at another example (extra clip not in the painting ancestor): <div style="overflow:scroll"> <div style="overflow:scroll"> <div style="position:relative; z-index:0;"></div> The innermost div will be painted as a positive child (child stacking context) of the root, thus the intermediate overflow clips won't be traversed during paint. To avoid the need to look for non-stacking-context ancestor during paint, it is useful to cache the paint property state in the layers. And there is the opposite example (clip in painting ancestor doesn't apply): <div style="opacity:0.5; overflow:scroll"> <div style="position:absolute;"> Again, I don't think there is an easy way we can determine the clip node without an ancestor walk. https://codereview.chromium.org/1651153003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:334: RefPtr<ClipPaintPropertyNode> newClipNodeForOverflowClip = createOverflowClipIfNeeded(object, localContext); On 2016/02/02 06:18:20, pdr wrote: > This creates clips for both border radius and overflow clip. Does this patch > need to consider the border radius clip created before overflow clip? See comments above. Border radius clip on normal flow will be applied in my 5th CL.
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/1651153003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651153003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Tien-Ren spent several hours with me and I think I finally understand this patch (and the direction of the followups). This patch is conceptually sound but needs work to socialize the idea to the rest of the team. To recap, there are two tree orders we care about: tree order and paint order. Here's the Escher problem as an example: http://jsbin.com/xumuho Escher layout tree (roughly shows tree order): html div id=scroller div id=scrolled-stacking-context, relatively positioned div id=yellow, positioned Escher layer tree (roughly shows paint order): html normal flow list: layer created by div id=scroller, clipped positive z-order list: layer created by div id=yellow, positioned layer created by div id=scrolled-stacking-context, relatively positioned, clipped LayoutObject's ObjectPaintProperties currently store the properties created by a LayoutObject. For example, if a LayoutObject does not create a clip itself, the ObjectPaintProperties will have a null clip instead of a pointer to the current clip. This patch is storing the full paint property tree context directly on layers when building the property trees so they don't need to be looked up during paint. This way, when painting the layer for scrolled-stacking-context in the above example, the scroller's clip is found in O(1) without needing to crawl back up the layer tree. My feedback: There's a 1:1 mapping between LayoutObjects with ObjectPaintProperties and PaintLayers, so whether this cache is stored on PaintLayer or inside ObjectPaintProperties shouldn't make a difference. We can move the location of this cache without breaking the dependent patchsets, they will just need a simple rebase. I think it would be better in terms of code organization to move "cachedLayerPaintPropertyContext" to be a lazily-allocated member of ObjectPaintProperties. We should consider a followup patch where ObjectPaintProperties stores the properties that are needed for paint instead of just those created by the object itself. With cachedLayerPaintPropertyContext inside ObjectPaintProperties, this is a simple refactoring. Do we need this cache for all PaintLayers, or only self painting layers? WDYT about renaming "cachedLayerPaintPropertyContext" to "selfPaintingLayerPaintPropertyContext"?
Chris and I chatted about this this morning--specifically, the invalidation problem if we store the complete property tree state. We walked through some examples and it doesn't seem hard to recompute. I think we should go ahead and fully store the property tree state on every ObjectPaintProperties instead of this approach.
migrated the cache to ObjectPaintProperties
pdr@chromium.org changed reviewers: + chrishtr@chromium.org
This is looking pretty good. Can you add one or two unit tests that show it works and verify the values? +cc chrishtr, please review this as well. https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:49: // +---[ transform ] The space created by CSS transform. WDYT about updating this diagram to include the context? https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: TransformPaintPropertyNode* transformForLayerContents() const; Does transformForLayerContents need to be updated to return the propertyTreeContext node?
On 2016/02/03 23:56:11, pdr wrote: > This is looking pretty good. Can you add one or two unit tests that show it > works and verify the values? Sure! I think the examples I showed you yesterday would make great unit tests.
https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:49: // +---[ transform ] The space created by CSS transform. On 2016/02/03 23:56:11, pdr wrote: > WDYT about updating this diagram to include the context? Acknowledged. https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: TransformPaintPropertyNode* transformForLayerContents() const; On 2016/02/03 23:56:11, pdr wrote: > Does transformForLayerContents need to be updated to return the > propertyTreeContext node? Ah ha! Yes we can make use of the cache to get rid of the cascading ifs. Though I'll have to look through the call sites to make sure. Perhaps in another cleanup CL?
https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:313: PassOwnPtr<ObjectPaintProperties::PropertyTreeContext> cacheTreeContextIfNeeded(LayoutObject& object, const PaintPropertyTreeBuilderContext& context) I still don't really view this as a cache any more than the entire tree is a cache for efficiently looking up property data. Rename createAncestorContextIfNeeded? (or similar)?
On 2016/02/04 at 00:03:02, trchen wrote: > Ah ha! Yes we can make use of the cache to get rid of the cascading ifs. Though I'll have to look through the call sites to make sure. Perhaps in another cleanup CL? Hmm, I was just thinking of adding one more if statement. If we can remove the if statements in a future patch, that would be great.
https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: TransformPaintPropertyNode* transformForLayerContents() const; On 2016/02/04 00:03:02, trchen wrote: > On 2016/02/03 23:56:11, pdr wrote: > > Does transformForLayerContents need to be updated to return the > > propertyTreeContext node? > > Ah ha! Yes we can make use of the cache to get rid of the cascading ifs. Though > I'll have to look through the call sites to make sure. Perhaps in another > cleanup CL? NVM. We can simply remove it with my CL #2.
https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:313: PassOwnPtr<ObjectPaintProperties::PropertyTreeContext> cacheTreeContextIfNeeded(LayoutObject& object, const PaintPropertyTreeBuilderContext& context) On 2016/02/04 00:06:45, pdr wrote: > I still don't really view this as a cache any more than the entire tree is a > cache for efficiently looking up property data. > > Rename createAncestorContextIfNeeded? (or similar)? recordTreeContextIfNeeded that is.
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/1651153003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651153003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking great, pretty much down to bikeshedding which is a good sign :) https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:8: #include "platform/geometry/LayoutPoint.h" Can you update PaintPropertyTreePrinter too? To test, I usually add this to FrameView.cpp: #include "core/paint/PaintPropertyTreePrinter.h" and showTransformPropertyTree(*this); showClipPropertyTree(*this); to the end of FrameView::updatePaintProperties. Or, if you don't find this class useful, lets delete PaintPropertyTreePrinter in a followup. AFAIK, I'm the only user. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:19: // The minimal set of paint properties created by a |LayoutObject|. These We need to update this comment too, since the properties here are not just the ones created by a layout object. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:50: // | It is the local border box space the painter expects, This helped me understand the situation. Maybe even rephrase this a little: "<-- This is the local border box space, see: PropertyTreeContext below." https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:61: TransformPaintPropertyNode* transformForLayerContents() const; I think this comment and code still needs updating to return the context transform node. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:74: struct PropertyTreeContext { WDYT of "LocalBorderBoxProperties"? In reading through this patch (and your comment above), I realized this may be a better name since it implies that transform may come before, and overflow clip after, etc. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:76: PaintChunkProperties properties; Can you think of a case when the effect/filter property ever be used? https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:315: // Note: Currently only layer painter makes use of the pre-computed context. This condition can be changed. I tried to think of a case where this would be used for non-PaintLayers (e.g., svg, etc) but couldn't think of one. Can you think of a future case where we would need to use the local context other than layers? I see the local context as mainly working around the not-very-sane rules of overflow clipping but I don't think we will be adding more of those cases.
Looking fine to me modulo pdr's comments.
https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:8: #include "platform/geometry/LayoutPoint.h" On 2016/02/04 04:54:01, pdr wrote: > Can you update PaintPropertyTreePrinter too? > > To test, I usually add this to FrameView.cpp: > #include "core/paint/PaintPropertyTreePrinter.h" > and > showTransformPropertyTree(*this); > showClipPropertyTree(*this); > to the end of FrameView::updatePaintProperties. > > Or, if you don't find this class useful, lets delete PaintPropertyTreePrinter in > a followup. AFAIK, I'm the only user. Acknowledged. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:19: // The minimal set of paint properties created by a |LayoutObject|. These On 2016/02/04 04:54:01, pdr wrote: > We need to update this comment too, since the properties here are not just the > ones created by a layout object. Acknowledged. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:50: // | It is the local border box space the painter expects, On 2016/02/04 04:54:01, pdr wrote: > This helped me understand the situation. Maybe even rephrase this a little: > "<-- This is the local border box space, see: PropertyTreeContext below." Acknowledged. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:61: TransformPaintPropertyNode* transformForLayerContents() const; On 2016/02/04 04:54:01, pdr wrote: > I think this comment and code still needs updating to return the context > transform node. Not really. The difference is that this function only create nodes created locally, while the cache always return the nearest ancestor. (And this function is going away anyway.) https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:74: struct PropertyTreeContext { On 2016/02/04 04:54:01, pdr wrote: > WDYT of "LocalBorderBoxProperties"? In reading through this patch (and your > comment above), I realized this may be a better name since it implies that > transform may come before, and overflow clip after, etc. Sounds clear! Let's do that. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:76: PaintChunkProperties properties; On 2016/02/04 04:54:01, pdr wrote: > Can you think of a case when the effect/filter property ever be used? Probably not. Effect node can be derived from the paint walk in O(1). It is there just for convenience. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:315: // Note: Currently only layer painter makes use of the pre-computed context. This condition can be changed. On 2016/02/04 04:54:01, pdr wrote: > I tried to think of a case where this would be used for non-PaintLayers (e.g., > svg, etc) but couldn't think of one. Can you think of a future case where we > would need to use the local context other than layers? I see the local context > as mainly working around the not-very-sane rules of overflow clipping but I > don't think we will be adding more of those cases. I don't know. It may be used by the merge algorithm for updating property nodes for cached display items? Anyway the comments just want to say there will be no adverse effect to loosen the condition.
Revised CL uploaded. https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1651153003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:8: #include "platform/geometry/LayoutPoint.h" I examined PaintPropertyTreePainter. The painter only dumps the property tree hierarchy, but this CL doesn't change the tree hierarchy at all. For the newly added information you probably want to show them in the layout tree dumper.
LGTM, thanks for sticking through this review! I think we got to a pretty nice place.
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/1651153003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651153003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651153003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651153003/80001
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to find the nearest clip node. ========== to ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to find the nearest clip node. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to find the nearest clip node. ========== to ========== [SPv2] Adds pre-computed paint property context to PaintLayer. This is part #1 of the CLs to associate overflow clip/scroll nodes to display items. This CL adds pre-computed paint context to PaintLayer, so a layer won't need to traverse the positional ancestor chain (which is different from paint ancestor chain) to find the nearest clip node. Committed: https://crrev.com/e849a2e8bf5bb255524af4fb27a7ae2f652fa123 Cr-Commit-Position: refs/heads/master@{#373718} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e849a2e8bf5bb255524af4fb27a7ae2f652fa123 Cr-Commit-Position: refs/heads/master@{#373718} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
