Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874413003:
LayoutMenuList::ControlClipRect may depend on size of children (Closed)
DescriptionLayoutMenuList::ControlClipRect may depend on size of children
Add NeedsPaintPropertyUpdate().
BUG=721249
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2874413003
Cr-Commit-Position: refs/heads/master@{#471828}
Committed: https://chromium.googlesource.com/chromium/src/+/727b9633cf8e84b3d79be12f6284248a9e129227
Patch Set 1 #Patch Set 2 : - #
Total comments: 4
Patch Set 3 : - #
Messages
Total messages: 31 (19 generated)
Description was changed from ========== Overflow clip may change without box size change LayoutMenuList::ControlClipRect() may depend on size of children. Change the logic in PaintPropertyTreeBuilder::UpdateForObjectLocationAndSize() to ensure paint property update on overflow clip change not depending on box size change. BUG=721249 ========== to ========== Overflow clip may change without box size change LayoutMenuList::ControlClipRect() may depend on size of children. Change the logic in PaintPropertyTreeBuilder::UpdateForObjectLocationAndSize() to ensure paint property update on overflow clip change not depending on box size change. BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@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...
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wangxianzhu@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // Check for change of overflow clip rect to ensure paint property update. In other areas of code we mark the object as needing a paint property update when the control clip changes, rather than detecting the change here. Is that possible in this case?
https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // Check for change of overflow clip rect to ensure paint property update. On 2017/05/12 23:20:30, pdr. wrote: > In other areas of code we mark the object as needing a paint property update > when the control clip changes, rather than detecting the change here. Is that > possible in this case? ControlClipRect() is calculated from other properties and it's hard to detect its change immediately after the calculated value changes. For example, LayoutMenuList::ControlClipRect() depends on child geometries. For that to work we would have to detect child geometry changes, which would need complex and tricky logic. We used to call SetNeedsPaintPropertyUpdate() from LayoutBox::SizeChanged() and it just happened to work because LayoutMenuList::SizeChanged() is called even when it size doesn't change. That method also caused unnecessary paint property updates on unchanged layout. For now for paint property updates caused by geometry changes, we rely on MayNeedsPaintInvalidation() to reach the object during PrePaint, and call this function regardless of NeedsPaintPropertyUpdate() and SetNeedsPaintPropertyUpdate() when needed. Previously overflow clip changes were detected by size change, but for the bug case, overflow clip changes without size change. This CL moves overflow clip case out of the size change condition.
https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // Check for change of overflow clip rect to ensure paint property update. On 2017/05/13 at 00:16:02, Xianzhu wrote: > On 2017/05/12 23:20:30, pdr. wrote: > > In other areas of code we mark the object as needing a paint property update > > when the control clip changes, rather than detecting the change here. Is that > > possible in this case? > > ControlClipRect() is calculated from other properties and it's hard to detect its change immediately after the calculated value changes. For example, LayoutMenuList::ControlClipRect() depends on child geometries. For that to work we would have to detect child geometry changes, which would need complex and tricky logic. Is this just needed for ControlClipRect or is it needed for all OverflowClips? It looks like LayoutMenuList manages it's children directly so it may not be that hard to detect changes to ControlClipRect. If it's not too hard, I would like to avoid adding branches to a central function like UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What about putting this in LayoutMenuList::AdjustInnerStyle: // The control clip paint property depends on the inner content // geometry (see: ControlClipRect). SetNeedsPaintPropertyUpdate();
https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // Check for change of overflow clip rect to ensure paint property update. On 2017/05/13 02:05:09, pdr. wrote: > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > On 2017/05/12 23:20:30, pdr. wrote: > > > In other areas of code we mark the object as needing a paint property update > > > when the control clip changes, rather than detecting the change here. Is > that > > > possible in this case? > > > > ControlClipRect() is calculated from other properties and it's hard to detect > its change immediately after the calculated value changes. For example, > LayoutMenuList::ControlClipRect() depends on child geometries. For that to work > we would have to detect child geometry changes, which would need complex and > tricky logic. > > Is this just needed for ControlClipRect or is it needed for all OverflowClips? > > It looks like LayoutMenuList manages it's children directly so it may not be > that hard to detect changes to ControlClipRect. If it's not too hard, I would > like to avoid adding branches to a central function like > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What about > putting this in LayoutMenuList::AdjustInnerStyle: > // The control clip paint property depends on the inner content > // geometry (see: ControlClipRect). > SetNeedsPaintPropertyUpdate(); SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but it seems not to catch all cases of ControlClipRect change. For example, LayoutMenuList::AddChild() will add child under inner_block_ which may affect inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also LayoutMenuList is just an example. I'm not sure if there have been or will be other cases like that. UpdateForObjectLocationAndSize() is a general solution for updating paint property caused by geometry changes. This CL moves the overflow clip case out of the box.Size() != box.PreviousSize() condition for any cases of overflow clip change. Actually if we could caught all the conditions needing paint property update before PrePaint, we would not need UpdateForObjectLocationAndSize() at all. I think we need it because we can't efficiently detect geometry changes before PrePaint (considering cases that geometry changed forth and back resulting no actual change in one layout stage). This was one of the reasons that we developed repaint-after-layout.
On 2017/05/13 at 04:55:06, wangxianzhu wrote: > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // Check for change of overflow clip rect to ensure paint property update. > On 2017/05/13 02:05:09, pdr. wrote: > > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > > On 2017/05/12 23:20:30, pdr. wrote: > > > > In other areas of code we mark the object as needing a paint property update > > > > when the control clip changes, rather than detecting the change here. Is > > that > > > > possible in this case? > > > > > > ControlClipRect() is calculated from other properties and it's hard to detect > > its change immediately after the calculated value changes. For example, > > LayoutMenuList::ControlClipRect() depends on child geometries. For that to work > > we would have to detect child geometry changes, which would need complex and > > tricky logic. > > > > Is this just needed for ControlClipRect or is it needed for all OverflowClips? > > > > It looks like LayoutMenuList manages it's children directly so it may not be > > that hard to detect changes to ControlClipRect. If it's not too hard, I would > > like to avoid adding branches to a central function like > > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What about > > putting this in LayoutMenuList::AdjustInnerStyle: > > // The control clip paint property depends on the inner content > > // geometry (see: ControlClipRect). > > SetNeedsPaintPropertyUpdate(); > > SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but it seems not to catch all cases of ControlClipRect change. For example, LayoutMenuList::AddChild() will add child under inner_block_ which may affect inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also LayoutMenuList is just an example. I'm not sure if there have been or will be other cases like that. I looked at the other cases and I think menu list is the only one. It doesn't seem too hard to sprinkle the menu list code with SetNeedsPropertyUpdate(), even if some aren't needed. Can you see if we can just add a few SetNeedsPropertyUpdate calls? If it is more than a few, or if the logic is more complex than just SetNeedsPropertyUpdate, then LGTM for this patch as-is.
On 2017/05/14 19:08:01, pdr. wrote: > On 2017/05/13 at 04:55:06, wangxianzhu wrote: > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp > (right): > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // > Check for change of overflow clip rect to ensure paint property update. > > On 2017/05/13 02:05:09, pdr. wrote: > > > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > > > On 2017/05/12 23:20:30, pdr. wrote: > > > > > In other areas of code we mark the object as needing a paint property > update > > > > > when the control clip changes, rather than detecting the change here. Is > > > that > > > > > possible in this case? > > > > > > > > ControlClipRect() is calculated from other properties and it's hard to > detect > > > its change immediately after the calculated value changes. For example, > > > LayoutMenuList::ControlClipRect() depends on child geometries. For that to > work > > > we would have to detect child geometry changes, which would need complex and > > > tricky logic. > > > > > > Is this just needed for ControlClipRect or is it needed for all > OverflowClips? > > > > > > It looks like LayoutMenuList manages it's children directly so it may not be > > > that hard to detect changes to ControlClipRect. If it's not too hard, I > would > > > like to avoid adding branches to a central function like > > > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What > about > > > putting this in LayoutMenuList::AdjustInnerStyle: > > > // The control clip paint property depends on the inner content > > > // geometry (see: ControlClipRect). > > > SetNeedsPaintPropertyUpdate(); > > > > SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but it > seems not to catch all cases of ControlClipRect change. For example, > LayoutMenuList::AddChild() will add child under inner_block_ which may affect > inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also > LayoutMenuList is just an example. I'm not sure if there have been or will be > other cases like that. > > I looked at the other cases and I think menu list is the only one. It doesn't > seem too hard to sprinkle the menu list code with SetNeedsPropertyUpdate(), even > if some aren't needed. Can you see if we can just add a few > SetNeedsPropertyUpdate calls? If it is more than a few, or if the logic is more > complex than just SetNeedsPropertyUpdate, then LGTM for this patch as-is. Yes, SetNeedsPaintPropertyUpdate() in LayoutMenuList::AdjustInnerStyle() and LayoutMenuList::AddChild() can fix the bug and I can create a patch for it. However, I would like to discuss the general topic more: 1. Do you think calling SetNeedsPaintPropertyUpdate() from UpdateForObjectLocationAndSize() is worse than calling SetNeedsPaintPropertyUpdate() before PrePaint, or just think moving the overflow clip case out of the Size() != PreviousSize() condition is not good? 2. If SetNeedsPaintPropertyUpdate() in UpdateForObjectLocationAndSize() not good, as it has been already there, do you think we should remove PaintProperTreeBuilder::UpdateForObjectLocationAndSize() in the future?
On 2017/05/14 at 19:24:37, wangxianzhu wrote: > On 2017/05/14 19:08:01, pdr. wrote: > > On 2017/05/13 at 04:55:06, wangxianzhu wrote: > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp > > (right): > > > > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // > > Check for change of overflow clip rect to ensure paint property update. > > > On 2017/05/13 02:05:09, pdr. wrote: > > > > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > > > > On 2017/05/12 23:20:30, pdr. wrote: > > > > > > In other areas of code we mark the object as needing a paint property > > update > > > > > > when the control clip changes, rather than detecting the change here. Is > > > > that > > > > > > possible in this case? > > > > > > > > > > ControlClipRect() is calculated from other properties and it's hard to > > detect > > > > its change immediately after the calculated value changes. For example, > > > > LayoutMenuList::ControlClipRect() depends on child geometries. For that to > > work > > > > we would have to detect child geometry changes, which would need complex and > > > > tricky logic. > > > > > > > > Is this just needed for ControlClipRect or is it needed for all > > OverflowClips? > > > > > > > > It looks like LayoutMenuList manages it's children directly so it may not be > > > > that hard to detect changes to ControlClipRect. If it's not too hard, I > > would > > > > like to avoid adding branches to a central function like > > > > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What > > about > > > > putting this in LayoutMenuList::AdjustInnerStyle: > > > > // The control clip paint property depends on the inner content > > > > // geometry (see: ControlClipRect). > > > > SetNeedsPaintPropertyUpdate(); > > > > > > SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but it > > seems not to catch all cases of ControlClipRect change. For example, > > LayoutMenuList::AddChild() will add child under inner_block_ which may affect > > inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also > > LayoutMenuList is just an example. I'm not sure if there have been or will be > > other cases like that. > > > > I looked at the other cases and I think menu list is the only one. It doesn't > > seem too hard to sprinkle the menu list code with SetNeedsPropertyUpdate(), even > > if some aren't needed. Can you see if we can just add a few > > SetNeedsPropertyUpdate calls? If it is more than a few, or if the logic is more > > complex than just SetNeedsPropertyUpdate, then LGTM for this patch as-is. > > Yes, SetNeedsPaintPropertyUpdate() in LayoutMenuList::AdjustInnerStyle() and LayoutMenuList::AddChild() can fix the bug and I can create a patch for it. > > However, I would like to discuss the general topic more: > > 1. Do you think calling SetNeedsPaintPropertyUpdate() from UpdateForObjectLocationAndSize() is worse than calling SetNeedsPaintPropertyUpdate() before PrePaint, or just think moving the overflow clip case out of the Size() != PreviousSize() condition is not good? > > 2. If SetNeedsPaintPropertyUpdate() in UpdateForObjectLocationAndSize() not good, as it has been already there, do you think we should remove PaintProperTreeBuilder::UpdateForObjectLocationAndSize() in the future? I have two reasons for wanting to avoid UpdateForObjectLocationAndSize if it is practically possible: 1) Performance: When we add code to UpdateForObjectLocationAndSize, we increase the cost for all objects, even though LayoutMenuList is the only object that needs this logic. LayoutMenuList is a good example of this because it is particularly rare. This can be mitigated by only doing this branch for objects that need overflow clip, but we're still having to read these extra instructions. 2) Code organization: This is sort of the "encapsulation" concept from object oriented programming. LayoutMenuList closely manages it's internal subtree which is why inner_block_ is a private member. I think we should keep the details of this strange implementation inside LayoutMenuList. The benefit here is just for us programmers: PaintPropertyTreeBuilder is complex and the simpler we can keep it, by not having menulist special cases, the easier it will be to maintain. I'm also pragmatic though, so I'm not against using UpdateForObjectLocationAndSize when we have to. But I'd really like to have just one way we detect changes to properties. We could, for example, put all of the SetNeedsPropertyUpdate calls in PaintProperTreeBuilder. Or, we could put them all in the layout objects.
On 2017/05/14 19:46:01, pdr. wrote: > On 2017/05/14 at 19:24:37, wangxianzhu wrote: > > On 2017/05/14 19:08:01, pdr. wrote: > > > On 2017/05/13 at 04:55:06, wangxianzhu wrote: > > > > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // > > > Check for change of overflow clip rect to ensure paint property update. > > > > On 2017/05/13 02:05:09, pdr. wrote: > > > > > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > > > > > On 2017/05/12 23:20:30, pdr. wrote: > > > > > > > In other areas of code we mark the object as needing a paint > property > > > update > > > > > > > when the control clip changes, rather than detecting the change > here. Is > > > > > that > > > > > > > possible in this case? > > > > > > > > > > > > ControlClipRect() is calculated from other properties and it's hard to > > > detect > > > > > its change immediately after the calculated value changes. For example, > > > > > LayoutMenuList::ControlClipRect() depends on child geometries. For that > to > > > work > > > > > we would have to detect child geometry changes, which would need complex > and > > > > > tricky logic. > > > > > > > > > > Is this just needed for ControlClipRect or is it needed for all > > > OverflowClips? > > > > > > > > > > It looks like LayoutMenuList manages it's children directly so it may > not be > > > > > that hard to detect changes to ControlClipRect. If it's not too hard, I > > > would > > > > > like to avoid adding branches to a central function like > > > > > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. > What > > > about > > > > > putting this in LayoutMenuList::AdjustInnerStyle: > > > > > // The control clip paint property depends on the inner content > > > > > // geometry (see: ControlClipRect). > > > > > SetNeedsPaintPropertyUpdate(); > > > > > > > > SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but > it > > > seems not to catch all cases of ControlClipRect change. For example, > > > LayoutMenuList::AddChild() will add child under inner_block_ which may > affect > > > inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also > > > LayoutMenuList is just an example. I'm not sure if there have been or will > be > > > other cases like that. > > > > > > I looked at the other cases and I think menu list is the only one. It > doesn't > > > seem too hard to sprinkle the menu list code with SetNeedsPropertyUpdate(), > even > > > if some aren't needed. Can you see if we can just add a few > > > SetNeedsPropertyUpdate calls? If it is more than a few, or if the logic is > more > > > complex than just SetNeedsPropertyUpdate, then LGTM for this patch as-is. > > > > Yes, SetNeedsPaintPropertyUpdate() in LayoutMenuList::AdjustInnerStyle() and > LayoutMenuList::AddChild() can fix the bug and I can create a patch for it. > > > > However, I would like to discuss the general topic more: > > > > 1. Do you think calling SetNeedsPaintPropertyUpdate() from > UpdateForObjectLocationAndSize() is worse than calling > SetNeedsPaintPropertyUpdate() before PrePaint, or just think moving the overflow > clip case out of the Size() != PreviousSize() condition is not good? > > > > 2. If SetNeedsPaintPropertyUpdate() in UpdateForObjectLocationAndSize() not > good, as it has been already there, do you think we should remove > PaintProperTreeBuilder::UpdateForObjectLocationAndSize() in the future? > > I have two reasons for wanting to avoid UpdateForObjectLocationAndSize if it is > practically possible: > 1) Performance: When we add code to UpdateForObjectLocationAndSize, we increase > the cost for all objects, even though LayoutMenuList is the only object that > needs this logic. LayoutMenuList is a good example of this because it is > particularly rare. This can be mitigated by only doing this branch for objects > that need overflow clip, but we're still having to read these extra > instructions. > 2) Code organization: This is sort of the "encapsulation" concept from object > oriented programming. LayoutMenuList closely manages it's internal subtree which > is why inner_block_ is a private member. I think we should keep the details of > this strange implementation inside LayoutMenuList. The benefit here is just for > us programmers: PaintPropertyTreeBuilder is complex and the simpler we can keep > it, by not having menulist special cases, the easier it will be to maintain. > > I'm also pragmatic though, so I'm not against using > UpdateForObjectLocationAndSize when we have to. But I'd really like to have just > one way we detect changes to properties. We could, for example, put all of the > SetNeedsPropertyUpdate calls in PaintProperTreeBuilder. Or, we could put them > all in the layout objects. I agree that encapsulation is good and the performance concern. I think the difference between our opinions was because I treated the patch as a general method instead of LayoutMenuList-specific. Also I noticed that this CL brings the following pattern into PaintPropertyTreeBuilder which is not good: if (overflow clip changed) SetNeedsPaintProertyUpdate(); It is actually updating overflow clip unconditionally. If we do this for every property, then we are updating all properties unconditionally. (However, with NeedsPaintOffsetAndVisualRectUpdate() we might have much less cases that PaintPropertyTreeBuilder is called when NeedsPaintPropertyUpdate() is false. This might make updating paint properties unconditionally not so bad. It also causes FindPropertiesNeedingUpdate less effective to find under-invalidation of paint properties. Might be worth investigation.) Uploaded a patch which adds SetNeedsPaintPropertyUpdate in LayoutMenuList. Ptal.
Description was changed from ========== Overflow clip may change without box size change LayoutMenuList::ControlClipRect() may depend on size of children. Change the logic in PaintPropertyTreeBuilder::UpdateForObjectLocationAndSize() to ensure paint property update on overflow clip change not depending on box size change. BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Overflow clip may change without box size change LayoutMenuList::ControlClipRect() may depend on size of children. Add NeedsPaintPropertyUpdate(). BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@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: This issue passed the CQ dry run.
On 2017/05/14 at 21:33:15, wangxianzhu wrote: > On 2017/05/14 19:46:01, pdr. wrote: > > On 2017/05/14 at 19:24:37, wangxianzhu wrote: > > > On 2017/05/14 19:08:01, pdr. wrote: > > > > On 2017/05/13 at 04:55:06, wangxianzhu wrote: > > > > > > > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp > > > > (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/2874413003/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1159: // > > > > Check for change of overflow clip rect to ensure paint property update. > > > > > On 2017/05/13 02:05:09, pdr. wrote: > > > > > > On 2017/05/13 at 00:16:02, Xianzhu wrote: > > > > > > > On 2017/05/12 23:20:30, pdr. wrote: > > > > > > > > In other areas of code we mark the object as needing a paint > > property > > > > update > > > > > > > > when the control clip changes, rather than detecting the change > > here. Is > > > > > > that > > > > > > > > possible in this case? > > > > > > > > > > > > > > ControlClipRect() is calculated from other properties and it's hard to > > > > detect > > > > > > its change immediately after the calculated value changes. For example, > > > > > > LayoutMenuList::ControlClipRect() depends on child geometries. For that > > to > > > > work > > > > > > we would have to detect child geometry changes, which would need complex > > and > > > > > > tricky logic. > > > > > > > > > > > > Is this just needed for ControlClipRect or is it needed for all > > > > OverflowClips? > > > > > > > > > > > > It looks like LayoutMenuList manages it's children directly so it may > > not be > > > > > > that hard to detect changes to ControlClipRect. If it's not too hard, I > > > > would > > > > > > like to avoid adding branches to a central function like > > > > > > UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. > > What > > > > about > > > > > > putting this in LayoutMenuList::AdjustInnerStyle: > > > > > > // The control clip paint property depends on the inner content > > > > > > // geometry (see: ControlClipRect). > > > > > > SetNeedsPaintPropertyUpdate(); > > > > > > > > > > SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but > > it > > > > seems not to catch all cases of ControlClipRect change. For example, > > > > LayoutMenuList::AddChild() will add child under inner_block_ which may > > affect > > > > inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also > > > > LayoutMenuList is just an example. I'm not sure if there have been or will > > be > > > > other cases like that. > > > > > > > > I looked at the other cases and I think menu list is the only one. It > > doesn't > > > > seem too hard to sprinkle the menu list code with SetNeedsPropertyUpdate(), > > even > > > > if some aren't needed. Can you see if we can just add a few > > > > SetNeedsPropertyUpdate calls? If it is more than a few, or if the logic is > > more > > > > complex than just SetNeedsPropertyUpdate, then LGTM for this patch as-is. > > > > > > Yes, SetNeedsPaintPropertyUpdate() in LayoutMenuList::AdjustInnerStyle() and > > LayoutMenuList::AddChild() can fix the bug and I can create a patch for it. > > > > > > However, I would like to discuss the general topic more: > > > > > > 1. Do you think calling SetNeedsPaintPropertyUpdate() from > > UpdateForObjectLocationAndSize() is worse than calling > > SetNeedsPaintPropertyUpdate() before PrePaint, or just think moving the overflow > > clip case out of the Size() != PreviousSize() condition is not good? > > > > > > 2. If SetNeedsPaintPropertyUpdate() in UpdateForObjectLocationAndSize() not > > good, as it has been already there, do you think we should remove > > PaintProperTreeBuilder::UpdateForObjectLocationAndSize() in the future? > > > > I have two reasons for wanting to avoid UpdateForObjectLocationAndSize if it is > > practically possible: > > 1) Performance: When we add code to UpdateForObjectLocationAndSize, we increase > > the cost for all objects, even though LayoutMenuList is the only object that > > needs this logic. LayoutMenuList is a good example of this because it is > > particularly rare. This can be mitigated by only doing this branch for objects > > that need overflow clip, but we're still having to read these extra > > instructions. > > 2) Code organization: This is sort of the "encapsulation" concept from object > > oriented programming. LayoutMenuList closely manages it's internal subtree which > > is why inner_block_ is a private member. I think we should keep the details of > > this strange implementation inside LayoutMenuList. The benefit here is just for > > us programmers: PaintPropertyTreeBuilder is complex and the simpler we can keep > > it, by not having menulist special cases, the easier it will be to maintain. > > > > I'm also pragmatic though, so I'm not against using > > UpdateForObjectLocationAndSize when we have to. But I'd really like to have just > > one way we detect changes to properties. We could, for example, put all of the > > SetNeedsPropertyUpdate calls in PaintProperTreeBuilder. Or, we could put them > > all in the layout objects. > > I agree that encapsulation is good and the performance concern. I think the difference between our opinions was because I treated the patch as a general method instead of LayoutMenuList-specific. > > Also I noticed that this CL brings the following pattern into PaintPropertyTreeBuilder which is not good: > if (overflow clip changed) > SetNeedsPaintProertyUpdate(); > It is actually updating overflow clip unconditionally. If we do this for every property, then we are updating all properties unconditionally. > > (However, with NeedsPaintOffsetAndVisualRectUpdate() we might have much less cases that PaintPropertyTreeBuilder is called when NeedsPaintPropertyUpdate() is false. This might make updating paint properties unconditionally not so bad. It also causes FindPropertiesNeedingUpdate less effective to find under-invalidation of paint properties. Might be worth investigation.) > > Uploaded a patch which adds SetNeedsPaintPropertyUpdate in LayoutMenuList. Ptal. LGTM, thanks!
Description was changed from ========== Overflow clip may change without box size change LayoutMenuList::ControlClipRect() may depend on size of children. Add NeedsPaintPropertyUpdate(). BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutMenuList::ControlClipRect may depend on size of children Add NeedsPaintPropertyUpdate(). BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494871096448380,
"parent_rev": "7afec42068281c82d60ba40ade02f86d40b28e0e", "commit_rev":
"727b9633cf8e84b3d79be12f6284248a9e129227"}
Message was sent while issue was closed.
Description was changed from ========== LayoutMenuList::ControlClipRect may depend on size of children Add NeedsPaintPropertyUpdate(). BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutMenuList::ControlClipRect may depend on size of children Add NeedsPaintPropertyUpdate(). BUG=721249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2874413003 Cr-Commit-Position: refs/heads/master@{#471828} Committed: https://chromium.googlesource.com/chromium/src/+/727b9633cf8e84b3d79be12f6284... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/727b9633cf8e84b3d79be12f6284... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
