Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292273003:
Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. (Closed)
DescriptionDefine contentsProperties on ObjectPaintProperties, for use in paint invalidation.
BUG=638415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/abb4cf14dd75848473eaf8706fbca3be284f027a
Cr-Commit-Position: refs/heads/master@{#416079}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #
Total comments: 5
Patch Set 5 : none #Patch Set 6 : none #Patch Set 7 : none #
Total comments: 4
Patch Set 8 : none #
Messages
Total messages: 38 (16 generated)
Description was changed from ========== none none none BUG= ========== to ========== none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. (merge into M53) BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. (merge into M53) BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org, szager@chromium.org
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
pdr@chromium.org changed reviewers: + trchen@chromium.org
https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: std::unique_ptr<PropertyTreeState> m_contentsProperties; Instead of caching such a large object, could we compute this dynamically in O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1592: TEST_P(PaintPropertyTreeBuilderTest, OverflowClipContentsProperties) Can you add a test for SVG with svgLocalToBorderBoxTransform set?
https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: std::unique_ptr<PropertyTreeState> m_contentsProperties; On 2016/08/31 04:48:52, pdr. wrote: > Instead of caching such a large object, could we compute this dynamically in > O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? +1, I don't think it should be stored; it should be generated dynamically on each call to contentsProperties().
https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: std::unique_ptr<PropertyTreeState> m_contentsProperties; On 2016/08/31 at 04:48:52, pdr. wrote: > Instead of caching such a large object, could we compute this dynamically in O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? Yes. I think you could say the same for localBorderBoxProperties. I can change one or both. Why was it cached for the other one? Is it anticipated to be used often? https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1592: TEST_P(PaintPropertyTreeBuilderTest, OverflowClipContentsProperties) On 2016/08/31 at 04:48:52, pdr. wrote: > Can you add a test for SVG with svgLocalToBorderBoxTransform set? Done.
On 2016/08/31 at 17:43:30, chrishtr wrote: > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): > > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: std::unique_ptr<PropertyTreeState> m_contentsProperties; > On 2016/08/31 at 04:48:52, pdr. wrote: > > Instead of caching such a large object, could we compute this dynamically in O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? > > Yes. I think you could say the same for localBorderBoxProperties. I can change one or both. Why was it cached for the other one? Is it anticipated to be used often? I think the comment above LocalBorderBoxProperties talks about why we need to cache it. The local border box can't be computed in O(1) because we need to walk up the containing block chain, but I don't think we have any issues like that once you have the local border box. My thinking is that you can just take the local border box's properties and update the additional child properties if they exist (e.g., overwrite the transform if scrollTranslation() is set).
On 2016/08/31 at 17:48:11, pdr wrote: > On 2016/08/31 at 17:43:30, chrishtr wrote: > > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): > > > > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: std::unique_ptr<PropertyTreeState> m_contentsProperties; > > On 2016/08/31 at 04:48:52, pdr. wrote: > > > Instead of caching such a large object, could we compute this dynamically in O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? > > > > Yes. I think you could say the same for localBorderBoxProperties. I can change one or both. Why was it cached for the other one? Is it anticipated to be used often? > > I think the comment above LocalBorderBoxProperties talks about why we need to cache it. The local border box can't be computed in O(1) because we need to walk up the containing block chain, but I don't think we have any issues like that once you have the local border box. My thinking is that you can just take the local border box's properties and update the additional child properties if they exist (e.g., overwrite the transform if scrollTranslation() is set). Done. Sorry for not reading that comment more carefully in advance.
https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:83: PropertyTreeState contentsProperties() const; Perf nit: I'm not sure it's worth caring about, but this would be more efficient if it took a PropertyTreeState& argument: void getContentsProperties(PropertyTreeState&) const; I would only worry about it if this method is going to be called a lot. https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1635: LayoutBoxModelObject* clipper = toLayoutBoxModelObject(document().getElementById("clipper")->layoutObject()); I'd like this test better if it had a call to document().getElementById("clipper")->scrollTo(nonZeroX, nonZeroY) in it.
On 2016/08/31 18:41:35, chrishtr wrote: > On 2016/08/31 at 17:48:11, pdr wrote: > > On 2016/08/31 at 17:43:30, chrishtr wrote: > > > > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): > > > > > > > https://codereview.chromium.org/2292273003/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:143: > std::unique_ptr<PropertyTreeState> m_contentsProperties; > > > On 2016/08/31 at 04:48:52, pdr. wrote: > > > > Instead of caching such a large object, could we compute this dynamically > in O(1) using LocalBorderBoxProperties and the nodes on ObjectPaintProperties? > > > > > > Yes. I think you could say the same for localBorderBoxProperties. I can > change one or both. Why was it cached for the other one? Is it anticipated to be > used often? > > > > I think the comment above LocalBorderBoxProperties talks about why we need to > cache it. The local border box can't be computed in O(1) because we need to walk > up the containing block chain, but I don't think we have any issues like that > once you have the local border box. My thinking is that you can just take the > local border box's properties and update the additional child properties if they > exist (e.g., overwrite the transform if scrollTranslation() is set). > > Done. Sorry for not reading that comment more carefully in advance. Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory.
https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:83: PropertyTreeState contentsProperties() const; On 2016/08/31 at 21:16:19, szager1 wrote: > Perf nit: I'm not sure it's worth caring about, but this would be more efficient if it took a PropertyTreeState& argument: > > void getContentsProperties(PropertyTreeState&) const; > > I would only worry about it if this method is going to be called a lot. Done. https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2292273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1635: LayoutBoxModelObject* clipper = toLayoutBoxModelObject(document().getElementById("clipper")->layoutObject()); On 2016/08/31 at 21:16:19, szager1 wrote: > I'd like this test better if it had a call to document().getElementById("clipper")->scrollTo(nonZeroX, nonZeroY) in it. Done.
On 2016/08/31 at 21:30:59, trchen wrote: > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. This is a good point :(
On 2016/09/01 at 17:11:07, pdr wrote: > On 2016/08/31 at 21:30:59, trchen wrote: > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > This is a good point :( We don't have a way to compute them at all in the other cases, in the present code.
On 2016/09/01 at 17:13:26, chrishtr wrote: > On 2016/09/01 at 17:11:07, pdr wrote: > > On 2016/08/31 at 21:30:59, trchen wrote: > > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > > > This is a good point :( > > We don't have a way to compute them at all in the other cases, in the present code. Is this asserted anywhere? E.g., that mapLocalRectToPaintInvalidationBacking isn't called when !isBox&&!hasLayer? Tien-ren/me added that optimization in an ad-hoc way.
On 2016/09/01 at 17:22:18, pdr wrote: > On 2016/09/01 at 17:13:26, chrishtr wrote: > > On 2016/09/01 at 17:11:07, pdr wrote: > > > On 2016/08/31 at 21:30:59, trchen wrote: > > > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > > > > > This is a good point :( > > > > We don't have a way to compute them at all in the other cases, in the present code. > > Is this asserted anywhere? E.g., that mapLocalRectToPaintInvalidationBacking isn't called when !isBox&&!hasLayer? Tien-ren/me added that optimization in an ad-hoc way. No, and in fact it will get called in such cases. I'd like to address that in a followup.
On 2016/09/01 at 17:24:52, chrishtr wrote: > On 2016/09/01 at 17:22:18, pdr wrote: > > On 2016/09/01 at 17:13:26, chrishtr wrote: > > > On 2016/09/01 at 17:11:07, pdr wrote: > > > > On 2016/08/31 at 21:30:59, trchen wrote: > > > > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > > > > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > > > > > > > This is a good point :( > > > > > > We don't have a way to compute them at all in the other cases, in the present code. > > > > Is this asserted anywhere? E.g., that mapLocalRectToPaintInvalidationBacking isn't called when !isBox&&!hasLayer? Tien-ren/me added that optimization in an ad-hoc way. > > No, and in fact it will get called in such cases. I'd like to address that in a followup. LGTM
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Define contentsProperties on ObjectPaintProperties, for use in paint invalidation. BUG=638415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/abb4cf14dd75848473eaf8706fbca3be284f027a Cr-Commit-Position: refs/heads/master@{#416079} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/abb4cf14dd75848473eaf8706fbca3be284f027a Cr-Commit-Position: refs/heads/master@{#416079}
Message was sent while issue was closed.
On 2016/09/01 at 17:24:52, chrishtr wrote: > On 2016/09/01 at 17:22:18, pdr wrote: > > On 2016/09/01 at 17:13:26, chrishtr wrote: > > > On 2016/09/01 at 17:11:07, pdr wrote: > > > > On 2016/08/31 at 21:30:59, trchen wrote: > > > > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > > > > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > > > > > > > This is a good point :( > > > > > > We don't have a way to compute them at all in the other cases, in the present code. > > > > Is this asserted anywhere? E.g., that mapLocalRectToPaintInvalidationBacking isn't called when !isBox&&!hasLayer? Tien-ren/me added that optimization in an ad-hoc way. > > No, and in fact it will get called in such cases. I'd like to address that in a followup. Belated comment: can you add a todo or file a bug for this?
Message was sent while issue was closed.
On 2016/09/01 at 22:14:41, pdr wrote: > On 2016/09/01 at 17:24:52, chrishtr wrote: > > On 2016/09/01 at 17:22:18, pdr wrote: > > > On 2016/09/01 at 17:13:26, chrishtr wrote: > > > > On 2016/09/01 at 17:11:07, pdr wrote: > > > > > On 2016/08/31 at 21:30:59, trchen wrote: > > > > > > Note that LocalBorderBoxProperties are not always cached. Currently we only cache them when the LayoutObject has a PaintLayer because that's where "interesting" things such as clip escaping can happen. > > > > > > > > > > > > If you need O(1) random access to any LayoutObject's content box properties, you'll need to cache LocalBorderBoxProperties on every LayoutObject then. That would cost a lot of memory. > > > > > > > > > > This is a good point :( > > > > > > > > We don't have a way to compute them at all in the other cases, in the present code. > > > > > > Is this asserted anywhere? E.g., that mapLocalRectToPaintInvalidationBacking isn't called when !isBox&&!hasLayer? Tien-ren/me added that optimization in an ad-hoc way. > > > > No, and in fact it will get called in such cases. I'd like to address that in a followup. > > Belated comment: can you add a todo or file a bug for this? https://bugs.chromium.org/p/chromium/issues/detail?id=643426 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
