Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773593005:
Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager (Closed)
|
Created:
3 years, 9 months ago by yigu Modified:
3 years, 8 months ago CC:
chromium-reviews, dtapuska+blinkwatch_chromium.org, kinuko+watch, Navid Zolghadr, dtapuska+chromiumwatch_chromium.org, dshwang, blink-reviews-paint_chromium.org, blink-reviews, cc-bugs_chromium.org, blink-reviews-frames_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove logic of recording main thread scrolling reasons from cc to
blink::ScrollManager
Due to crbug.com/701355, all style related main thread scrolling reasons
stop being recorded. This patch moves the logic of storing the style
related reasons to PLSA and move the recording logic to ScrollManager
because the information on the compositor side is insufficient. The
disabled test due to the previous bug has been enabled again with minor
modification.
BUG=704805
TEST=All/NonCompositedMainThreadScrollingReasonTest.*;
EventHandlerTest.NonCompositedMainThreadScrollingReason*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2773593005
Cr-Original-Commit-Position: refs/heads/master@{#464073}
Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b4845166e70c9349
Review-Url: https://codereview.chromium.org/2773593005
Cr-Commit-Position: refs/heads/master@{#464461}
Committed: https://chromium.googlesource.com/chromium/src/+/b797bf2e48843177bc5657622bb7fd1f09bb9e24
Patch Set 1 #Patch Set 2 : Unit test update #Patch Set 3 : nit #
Total comments: 13
Patch Set 4 : Address comments #Patch Set 5 : Bug fix #
Total comments: 11
Patch Set 6 : Update recording logic for non composited reasons #
Total comments: 15
Patch Set 7 : nit #Patch Set 8 : Address comments #Patch Set 9 : bug fix #
Total comments: 5
Patch Set 10 : Update recording logic #
Total comments: 5
Patch Set 11 : Remove the UnknownNonCompositedRegion bucket #
Total comments: 22
Patch Set 12 : Add unit tests #
Total comments: 20
Patch Set 13 : Split unit tests into smaller ones #
Total comments: 32
Patch Set 14 : Add readability to unit tests && handle blink reformat #
Total comments: 4
Patch Set 15 : nit #
Total comments: 19
Patch Set 16 : Address comments #
Total comments: 2
Patch Set 17 : nit #Patch Set 18 : fix incorrect test for android devices #Messages
Total messages: 63 (22 generated)
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager BUG= ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + ajuma@chromium.org, flackr@chromium.org, pdr@chromium.org, tdresser@chromium.org
yigu@chromium.org changed reviewers: + bokan@chromium.org - pdr@chromium.org
PTAL. Thanks!
Description was changed from ========== Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest InputHandlerProxyTest.MainThreadScrollingHistograms CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
cc/ lgtm
Ok, I think I understand this a little better now. The reasons you added could more accurately be called "NonCompositedScrollReasons" (lets rename where appropriate). In that case, I think it's simpler to leave in place all the existing scroll reasons recording as it happens on CC and only record on the main thread these NonCompositedScrollReasons. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:216: curElement = documentElement; Document-scrolling is always composited so we don't need to worry about it on the main thread. If we need to main thread scroll it - the existing machinery will set the reasons on its GraphicsLayer/cc::Layer and it'll be reported on the compositor (see my note in input_handler_proxy) https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:228: reasons |= curNode->layoutBoxModelObject() Given that I think since we'll record the other main thread scrolling reasons on the compositor (i.e. from main thread, we'll only record reasons due to non-compositing) I don't think you have to walk up the ancestor chain. Instead, you should walk up the LayoutBox tree to the first node that's PLSA::isScrollable and record the "main thread scrolling reasons" there. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:299: if (reasons) Move all the additions above into recordMainThreadScrollingReasons and early-out there if you don't need to record. recordMainThreadScrollingReasons should be private. This means you'll need to adjust the tests in EventHandlerTest -- we should really be loading some HTML that isn't composited, scrolling it, and making sure the appropriate stats are recorded. Given that I'm not sure unit tests are appropriately hooked up with compositing - it's possible in a unit test everything is marked as non-composited. If you can't make it work there, you may need to create a SimTest instead but lets cross that bridge when we get to it. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.h:94: // Record main thread scrolling reasons. This should be no-op; Why does this say it should be a no-op? Recording a histogram is an op :) https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1842: resetStyleRelatedMainThreadScrollingReasons(); We should reset the reasons inside computeNeedsCompositedScrolling. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:484: void resetStyleRelatedMainThreadScrollingReasons() { m_reasons = 0; } reset only gets called from inside PLSA. Remove this method and just replace with m_reasons = 0. Also, rename m_reasons to m_mainThreadScrollingReasons https://codereview.chromium.org/2773593005/diff/40001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (left): https://codereview.chromium.org/2773593005/diff/40001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:575: for (uint32_t i = 0; Sorry, on second thought, we shouldn't remove this. The reasons you've added are really "NonCompositedScrollerReasons". This records other reasons we might scroll on the main thread (all the reasons captured in MainThreadCanSetScrollReasons()) for scrollers that are still composited. And there's also compositor set reasons (CompositorCanSetScrollReasons()) that the main-thread doesn't know about. All the "style-related" reasons you've added you should split out of MainThreadCanSetScrollReasons() into a separate method called NonCompositedScrollReasons(). DCHECK that we're never recording one here. They're not really MainThreadCanSetScrollReasons since we never set them on a ScrollNode/cc::Layer.
Thanks for the feedback! I've updated the patch based on it. PTAL. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:216: curElement = documentElement; On 2017/03/24 17:55:54, bokan wrote: > Document-scrolling is always composited so we don't need to worry about it on > the main thread. If we need to main thread scroll it - the existing machinery > will set the reasons on its GraphicsLayer/cc::Layer and it'll be reported on the > compositor (see my note in input_handler_proxy) Done. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:228: reasons |= curNode->layoutBoxModelObject() On 2017/03/24 17:55:54, bokan wrote: > Given that I think since we'll record the other main thread scrolling reasons on > the compositor (i.e. from main thread, we'll only record reasons due to > non-compositing) I don't think you have to walk up the ancestor chain. > > Instead, you should walk up the LayoutBox tree to the first node that's > PLSA::isScrollable and record the "main thread scrolling reasons" there. Done. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.h:94: // Record main thread scrolling reasons. This should be no-op; On 2017/03/24 17:55:54, bokan wrote: > Why does this say it should be a no-op? Recording a histogram is an op :) Done. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1842: resetStyleRelatedMainThreadScrollingReasons(); On 2017/03/24 17:55:54, bokan wrote: > We should reset the reasons inside computeNeedsCompositedScrolling. Done. https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2773593005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:484: void resetStyleRelatedMainThreadScrollingReasons() { m_reasons = 0; } On 2017/03/24 17:55:55, bokan wrote: > reset only gets called from inside PLSA. Remove this method and just replace > with m_reasons = 0. Also, rename m_reasons to m_mainThreadScrollingReasons Done. https://codereview.chromium.org/2773593005/diff/40001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (left): https://codereview.chromium.org/2773593005/diff/40001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:575: for (uint32_t i = 0; On 2017/03/24 17:55:55, bokan wrote: > Sorry, on second thought, we shouldn't remove this. The reasons you've added are > really "NonCompositedScrollerReasons". This records other reasons we might > scroll on the main thread (all the reasons captured in > MainThreadCanSetScrollReasons()) for scrollers that are still composited. And > there's also compositor set reasons (CompositorCanSetScrollReasons()) that the > main-thread doesn't know about. > > All the "style-related" reasons you've added you should split out of > MainThreadCanSetScrollReasons() into a separate method called > NonCompositedScrollReasons(). DCHECK that we're never recording one here. > They're not really MainThreadCanSetScrollReasons since we never set them on a > ScrollNode/cc::Layer. Done.
Tim, Rob, any thoughts on my comment in ScrollManager? I don't think we have enough information in either Blink or CC to do this perfectly accurately without regressing performance. https://codereview.chromium.org/2773593005/diff/80001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/80001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:85: // contains any of the following non composited scroll reasons. The comment doesn't add anything - it says the exact same thing as the code. Describe "why"/"what", not "how". In this case, I'd say that these are the "reasons that prevent a scroller from being composited" or something along those lines. https://codereview.chromium.org/2773593005/diff/80001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:87: uint32_t reasons_set_by_main_thread_only = reasons_set_by_main_thread_only -> non_composited_reasons https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:93: data.scrollBegin.targetViewport = true; This should be false. It's used in Android WebView to send scrolls directly to the viewport layers, skipping hit tests. https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:488: "<div class='border box'><div class='spacer'></div></div>"); Please add at least one case that checks that a composited (and maybe main thread scrolled) box doesn't add to the counts. I'd also add a case where you have a composited scroller over a non-composited scroller. I believe that should cause the scroll events to go to the main thread even if scrolling over the composited box. https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:499: document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin); You should send a ScrollEnd after each scrollBegin, just to make sure we don't confuse the EventHandler. https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:208: if (scrollableArea->isScrollable()) { On second look, I don't think this is enough. If we have a non-composited scroller, the region that scroller occupies is added to the FrameView's non-fast scroll regions. There's no notion of overlap there, meaning that if the non-composited scroller is behind a composited scroller, it'll cause scrolls over the latter to go to the main thread. Worse still, the non-composited scroller may not be in the scroll chain of the composited, so we have no way to get back to the original reason we were sent to main. e.g.: <div id="a" style="width: 100px; height: 100px; overflow:scroll; opacity: 0.5; will-change: transform"> ... </div> <div id="b" style="width: 100px; height: 100px; overflow:scroll; position: absolute; background-color: white; contains: paint"> ... </div> In this case #b is composited and shows up overtop of #a. Scrolling it doesn't cause scroll to chain up to #a but #a's presence means that the scrolls will be sent to main. We'll miss the reason in this case. I'm not sure on how to proceed here - another idea might be to attach the reason to each shape in the NonFastScrollableRegion so that we record all this on CC (again). This seemed like a good idea at first, but taking a look it seems there'd be some significant changes in how we store the NonFastScrollRegions. I believe we'll union together contained regions as an optimisation - we'd have to stop doing that and store a complete list of rects/shapes in Region with a reason attached to each. I think that'd work but could have a perf impact on detecting when we're in a non-fast scroll region, I'm not sure how critical that code is. Perhaps adding an "Unknown" bucket and just accepting some inaccuracy in these cases is preferable. I'd like to hear what others think. https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.h:114: // Record non composited main thread scrolling reasons. Nit: comment unneeded. https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1778: // before continuing computeNeedsCompositedScrolling Comment unneeded.
https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:208: if (scrollableArea->isScrollable()) { On 2017/03/28 15:30:15, bokan wrote: > On second look, I don't think this is enough. If we have a non-composited > scroller, the region that scroller occupies is added to the FrameView's non-fast > scroll regions. There's no notion of overlap there, meaning that if the > non-composited scroller is behind a composited scroller, it'll cause scrolls > over the latter to go to the main thread. Worse still, the non-composited > scroller may not be in the scroll chain of the composited, so we have no way to > get back to the original reason we were sent to main. e.g.: > > <div id="a" style="width: 100px; height: 100px; overflow:scroll; opacity: 0.5; > will-change: transform"> > ... > </div> > <div id="b" style="width: 100px; height: 100px; overflow:scroll; position: > absolute; background-color: white; contains: paint"> > ... > </div> > > In this case #b is composited and shows up overtop of #a. Scrolling it doesn't > cause scroll to chain up to #a but #a's presence means that the scrolls will be > sent to main. We'll miss the reason in this case. > > I'm not sure on how to proceed here - another idea might be to attach the reason > to each shape in the NonFastScrollableRegion so that we record all this on CC > (again). This seemed like a good idea at first, but taking a look it seems > there'd be some significant changes in how we store the NonFastScrollRegions. I > believe we'll union together contained regions as an optimisation - we'd have to > stop doing that and store a complete list of rects/shapes in Region with a > reason attached to each. I think that'd work but could have a perf impact on > detecting when we're in a non-fast scroll region, I'm not sure how critical that > code is. > > Perhaps adding an "Unknown" bucket and just accepting some inaccuracy in these > cases is preferable. I'd like to hear what others think. Can you describe with a bit more detail how we'd identify when we're in the "Unknown" case? My intuition is that using an unknown bucket would be a good first step.
https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:208: if (scrollableArea->isScrollable()) { On 2017/03/28 17:10:22, tdresser wrote: > On 2017/03/28 15:30:15, bokan wrote: > > On second look, I don't think this is enough. If we have a non-composited > > scroller, the region that scroller occupies is added to the FrameView's > non-fast > > scroll regions. There's no notion of overlap there, meaning that if the > > non-composited scroller is behind a composited scroller, it'll cause scrolls > > over the latter to go to the main thread. Worse still, the non-composited > > scroller may not be in the scroll chain of the composited, so we have no way > to > > get back to the original reason we were sent to main. e.g.: > > > > <div id="a" style="width: 100px; height: 100px; overflow:scroll; opacity: 0.5; > > will-change: transform"> > > ... > > </div> > > <div id="b" style="width: 100px; height: 100px; overflow:scroll; position: > > absolute; background-color: white; contains: paint"> > > ... > > </div> > > > > In this case #b is composited and shows up overtop of #a. Scrolling it doesn't > > cause scroll to chain up to #a but #a's presence means that the scrolls will > be > > sent to main. We'll miss the reason in this case. > > > > I'm not sure on how to proceed here - another idea might be to attach the > reason > > to each shape in the NonFastScrollableRegion so that we record all this on CC > > (again). This seemed like a good idea at first, but taking a look it seems > > there'd be some significant changes in how we store the NonFastScrollRegions. > I > > believe we'll union together contained regions as an optimisation - we'd have > to > > stop doing that and store a complete list of rects/shapes in Region with a > > reason attached to each. I think that'd work but could have a perf impact on > > detecting when we're in a non-fast scroll region, I'm not sure how critical > that > > code is. > > > > Perhaps adding an "Unknown" bucket and just accepting some inaccuracy in these > > cases is preferable. I'd like to hear what others think. > > Can you describe with a bit more detail how we'd identify when we're in the > "Unknown" case? My intuition is that using an unknown bucket would be a good > first step. We do this walk up the containingBlock chain until we get to the first non-composited scroller and use its main thread scroll reason. If we hit the root and all the scrollers in the chain are composited, it must be because of an element not in the containing block chain, so we'd put these scrolls into a separate bucket. I think this works since I can't find a way to position a child scroller outside a parent scroller but keep them scroll chained, changing position schemes changes the containingBlock hierarchy. I'm not sure how common that "non-composited underlay" will be but at least it might give us an idea if we need to improve the metric collection or not to worry about it, before we do intrusive rearchitecting of NonFastScrollRegions.
https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:208: if (scrollableArea->isScrollable()) { On 2017/03/28 18:23:02, bokan wrote: > On 2017/03/28 17:10:22, tdresser wrote: > > On 2017/03/28 15:30:15, bokan wrote: > > > On second look, I don't think this is enough. If we have a non-composited > > > scroller, the region that scroller occupies is added to the FrameView's > > non-fast > > > scroll regions. There's no notion of overlap there, meaning that if the > > > non-composited scroller is behind a composited scroller, it'll cause scrolls > > > over the latter to go to the main thread. Worse still, the non-composited > > > scroller may not be in the scroll chain of the composited, so we have no way > > to > > > get back to the original reason we were sent to main. e.g.: > > > > > > <div id="a" style="width: 100px; height: 100px; overflow:scroll; opacity: > 0.5; > > > will-change: transform"> > > > ... > > > </div> > > > <div id="b" style="width: 100px; height: 100px; overflow:scroll; position: > > > absolute; background-color: white; contains: paint"> > > > ... > > > </div> > > > > > > In this case #b is composited and shows up overtop of #a. Scrolling it > doesn't > > > cause scroll to chain up to #a but #a's presence means that the scrolls will > > be > > > sent to main. We'll miss the reason in this case. > > > > > > I'm not sure on how to proceed here - another idea might be to attach the > > reason > > > to each shape in the NonFastScrollableRegion so that we record all this on > CC > > > (again). This seemed like a good idea at first, but taking a look it seems > > > there'd be some significant changes in how we store the > NonFastScrollRegions. > > I > > > believe we'll union together contained regions as an optimisation - we'd > have > > to > > > stop doing that and store a complete list of rects/shapes in Region with a > > > reason attached to each. I think that'd work but could have a perf impact on > > > detecting when we're in a non-fast scroll region, I'm not sure how critical > > that > > > code is. > > > > > > Perhaps adding an "Unknown" bucket and just accepting some inaccuracy in > these > > > cases is preferable. I'd like to hear what others think. > > > > Can you describe with a bit more detail how we'd identify when we're in the > > "Unknown" case? My intuition is that using an unknown bucket would be a good > > first step. > > We do this walk up the containingBlock chain until we get to the first > non-composited scroller and use its main thread scroll reason. If we hit the > root and all the scrollers in the chain are composited, it must be because of an > element not in the containing block chain, so we'd put these scrolls into a > separate bucket. I think this works since I can't find a way to position a child > scroller outside a parent scroller but keep them scroll chained, changing > position schemes changes the containingBlock hierarchy. > > I'm not sure how common that "non-composited underlay" will be but at least it > might give us an idea if we need to improve the metric collection or not to > worry about it, before we do intrusive rearchitecting of NonFastScrollRegions. Thanks. Yeah, considering that bucket separately SGTM.
Hi David. I've reorganized the recording logic for noncompositedreaons and updated the unit tests accordingly. PTAL.Thanks!
yigu@chromium.org changed reviewers: + jwd@chromium.org
lgtm
Did you determine where will-change: transform forces compositing? Will we clear the NonComposited reasons in that case? https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:46: // Scrolling a composited element which is on top of a non-composited Nit: newline above https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:90: // Usually a scroller won't be composited with the following reasons If we force promotion though, we remove these reasons right? https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:147: if (reasons & MainThreadScrollingReason::kUnknownNonCompositedRegion) I think this should never actually be stored/set on a ScrollableArea or GraphicsLayer, right? We basically only use it for reporting to UMA that we don't know why but we ended up on the main thread because of *some* non-composited scroller. In that case, perhaps we should just DCHECK here that it's not this reason as I think this function is only used to print out reasons attached to layers/areas. https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:201: // composited. super-nit: No need for an early line break here, wrap the below line on this line. Also, I think we generally put a comment like this inside the function. https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:205: // chain to find the first non composited region and record reaons accordingly. nit: reaons->reasons https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:206: // If no such region is found or it doesn't have any main thread scorlling nit: scorlling->scrolling https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:219: if (auto* scrollableArea = curBox->getScrollableArea()) { The repeated indents here make the code harder to follow. You can make it simpler by using a for loop instead and avoiding the outermost block using: auto* scrollableArea = curBox->getScrollableArea(); if (!scrollableArea) continue; https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:222: return 0; I don't think you need this early return since it might prevent us from recording a NonComposited reason that's caused by a parent scroller. In that case, I think we'd want to record both the mainTreadScrollingReason (which was done in CC) and the NonCompositedReason from the parent since if we optimised Chrome to fix the mainThreadScrollingReason so we could scroll on the compositor, the scroller still wouldn't be scrolling on main because of the NonFastScrollingRegion. So I think we want to record both reasons in that case. https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:224: compositedMainThreadScrollForUnknownReasons = true; We want to record "Unknown" if and only if we didn't hit any non-composited scrollers on the walk *and* none of the composited scrollers in the chain had a mainThreadScrollingReason (since in that case, we were sent to main for that reason). Given that, I'd call this bool "haveMainThreadScrollingReason" and change the code to match the name. This name better describes the "why" of recording the Unknown reason and the code will read more naturally because of it. https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:226: reasons = scrollableArea->getNonCompositedMainThreadScrollingReasons(); Add a DCHECK here that we have a reason. https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:236: return reasons; Add a DCHECK here that reasons only contains NonCompositedScrollReasons https://codereview.chromium.org/2773593005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:253: i < MainThreadScrollingReason::kMainThreadScrollingReasonCount; ++i) { You only need to loop over the NonCompositedScrollReasons here. Add a NonCompositedReasonsFirst, and NonCompositedReasonsLast in the enum and use that.
PS, please remember to wrap the commit message at 72 chars
Description was changed from ========== Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest InputHandlerProxyTest.MainThreadScrollingHistograms CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest InputHandlerProxyTest.MainThreadScrollingHistograms CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.MainThreadScrollingReasonRecordTest InputHandlerProxyTest.MainThreadScrollingHistograms CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.* EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Thanks David for the feedback! PTAL. https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:90: // Usually a scroller won't be composited with the following reasons On 2017/03/31 16:55:52, bokan wrote: > If we force promotion though, we remove these reasons right? Actually we don't remove these reasons for PLSA. If the PLSA is composited, we won't record these reasons.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.* EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel; master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel; master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:90: // Usually a scroller won't be composited with the following reasons On 2017/03/31 19:18:55, yigu wrote: > On 2017/03/31 16:55:52, bokan wrote: > > If we force promotion though, we remove these reasons right? > > Actually we don't remove these reasons for PLSA. If the PLSA is composited, we > won't record these reasons. We won't *record* the reasons, but will they still be *set* on the PLSA itself? I would also reword the comment to make the meaning clearer. e.g. "Returns true if the MainThreadScrollingReason is one that prevented a scroller from being composited". https://codereview.chromium.org/2773593005/diff/160001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/160001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:152: DCHECK(!(reasons & MainThreadScrollingReason::kUnknownNonCompositedRegion)); Add a comment here as to why we don't have a string for this case https://codereview.chromium.org/2773593005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:227: !!(m_frame->view()->mainThreadScrollingReasons()); There's no need to use !!, the value will be implicitly converted to bool. Also, I think this shouldn't be against m_frame->view()? Also also, this needs to look for only "MainThreadCanSetReasons" and "CompositorCanSetReasons" and ignore "NonCompositedScrollReasons" though that should be the case if usesCompositedScrolling is true so please add a DCHECK that it doesn't have a NonCompositedScrollReason https://codereview.chromium.org/2773593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:230: if (!reasons) This `if` violates the rule that a non-composited scroller must always have a NonCompositedMainThreadScrollingReason. https://codereview.chromium.org/2773593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:232: break; hmm...I think it would be beneficial to keep walking up even if we do have a NonComposited reason. So if there's two non-composited scrollers in a chain - both cause main thread scrolling so we should probably record both. flackr@, does that sound reasonable? https://codereview.chromium.org/2773593005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:236: if (usesCompositedScrolling && !reasons && !hasMainThreadScrollingReason) I think this is still off. usesCompositedScrolling should instead be usesNonCompositedScrolling since we need to make sure we didn't hit any *non-composited* scrollers before returning Unknown. (If we hit a non-composited scroller, we shouldn't return Unknown since we should use the reason for that scroller).
PTAL. Thanks! https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/100001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:90: // Usually a scroller won't be composited with the following reasons On 2017/04/04 18:14:26, bokan wrote: > On 2017/03/31 19:18:55, yigu wrote: > > On 2017/03/31 16:55:52, bokan wrote: > > > If we force promotion though, we remove these reasons right? > > > > Actually we don't remove these reasons for PLSA. If the PLSA is composited, we > > won't record these reasons. > > We won't *record* the reasons, but will they still be *set* on the PLSA itself? > > I would also reword the comment to make the meaning clearer. e.g. "Returns true > if the MainThreadScrollingReason is one that prevented a scroller from being > composited". When composited, the reasons won't be set on PLSA.
Ok, I think the current logic is fine for a single frame, but this doesn't address the iframe case we talked about yesterday, right? I'm wondering, the Unknown bucket isn't really buying us that much. We can still miss counting non-composited scrollers if they're overlapped by other scrollers that have some main thread reason so it's not getting us to perfect accuracy. I think if we removed the unknown reason the iframe case would become much easier. WDYT? https://codereview.chromium.org/2773593005/diff/180001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/180001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:52: // not in the chain in which case the following reason is added. Nit: the reason isn't added, it's reported. https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:205: // containing scroll chain to find the first non composited region and record I think this line and the section below are out of date. We don't "find the first" anymore, we look at all the scrollers in the chain. In general, you should avoid comments that are basically pseudo code for what's happening below, the reader can follow the code to find out what it's doing. Instead: describe why you're doing it and/or provide missing context. You're lines above are great, the fact that the other mainThreadScrollingReasons are recorded on CC is missing context and needed to understand what we're doing here. What we're trying to do here is "record scrolls that occurred on main due to a non-composited scroller". How that's done is described below by the code. https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:214: uint32_t reasons = 0; rename to nonCompositedReasons or similar. https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:229: DCHECK(!nonCompositedMainThreadScrollingReasons); You shouldn't branch just because you have a DCHECK. Instead, DCHECK(!scrollableArea->usesCompositedScrolling() || !nonCompositedMainThreadScrollingReasons). And if nonCompositedMainThreadScrollingReasons == 0 in the usesCompositedScrolling case, you can just |= it into reasons unconditionally. https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:237: return MainThreadScrollingReason::kUnknownNonCompositedRegion; Add a comment here describing this reason. i.e. There must be a non-composited scroller in the region the scroll event occurred in but it's overlapped by another scroller that has no reason to go to main.
On 2017/04/06 14:25:24, bokan wrote: > Ok, I think the current logic is fine for a single frame, but this doesn't > address the iframe case we talked about yesterday, right? > > I'm wondering, the Unknown bucket isn't really buying us that much. We can still > miss counting non-composited scrollers if they're overlapped by other scrollers > that have some main thread reason so it's not getting us to perfect accuracy. I > think if we removed the unknown reason the iframe case would become much easier. > WDYT? > > https://codereview.chromium.org/2773593005/diff/180001/cc/input/main_thread_s... > File cc/input/main_thread_scrolling_reason.h (right): > > https://codereview.chromium.org/2773593005/diff/180001/cc/input/main_thread_s... > cc/input/main_thread_scrolling_reason.h:52: // not in the chain in which case > the following reason is added. > Nit: the reason isn't added, it's reported. > > https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): > > https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:205: // containing scroll > chain to find the first non composited region and record > I think this line and the section below are out of date. We don't "find the > first" anymore, we look at all the scrollers in the chain. > > In general, you should avoid comments that are basically pseudo code for what's > happening below, the reader can follow the code to find out what it's doing. > Instead: describe why you're doing it and/or provide missing context. You're > lines above are great, the fact that the other mainThreadScrollingReasons are > recorded on CC is missing context and needed to understand what we're doing > here. > > What we're trying to do here is "record scrolls that occurred on main due to a > non-composited scroller". How that's done is described below by the code. > > https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:214: uint32_t reasons = > 0; > rename to nonCompositedReasons or similar. > > https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:229: > DCHECK(!nonCompositedMainThreadScrollingReasons); > You shouldn't branch just because you have a DCHECK. Instead, > DCHECK(!scrollableArea->usesCompositedScrolling() || > !nonCompositedMainThreadScrollingReasons). And if > nonCompositedMainThreadScrollingReasons == 0 in the usesCompositedScrolling > case, you can just |= it into reasons unconditionally. > > https://codereview.chromium.org/2773593005/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:237: return > MainThreadScrollingReason::kUnknownNonCompositedRegion; > Add a comment here describing this reason. i.e. There must be a non-composited > scroller in the region the scroll event occurred in but it's overlapped by > another scroller that has no reason to go to main. Thanks David for the feedback! I've removed the UnknownNonCompositedRegion bucket. PTAL.
Ok, logic is looking good now. There's a few fixes, comments, and I'd like to see expanded test coverage. But there's light at the end of the tunnel :) https://codereview.chromium.org/2773593005/diff/200001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/200001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:87: // unless we force the promotion. e.g. Apply will-change:transform. I would remove the first two lines. To me it implies that the reason may be set even if we force promotion. The lines below capture all that's needed. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:529: Please add a case with some nested scrollers, an example: A <--NonCompositedReasonA B <--Composited C <--NonCompositingReasonB To make sure we report both A and B As well as a case in which all the nested scrollers are composited, make sure we don't report anything. Another good one would be to make sure we don't report anything if there's non scrollable boxes either in between scrollers, or there are no scrollers at all. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:202: // composited. Either way, we have recorded either the reasons that nit: "reasons that stored" -> "reasons stored" https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:221: nonCompositedMainThreadScrollingReasons = this should be |=, right? https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:238: DCHECK(MainThreadScrollingReason::NonCompositedScrollReasons(reasons)); It wouldn't hurt to DCHECK that we don't have any mainThread or CompositorSet reasons. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:240: uint32_t mainThreadScrollingReasonEnumMax = unused https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1055: webViewImpl()->settings()->setPreferCompositingToLCDTextEnabled(false); We should probably have a test that runs with this flag on. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1062: // Add or remove style related attribute from a scroller shouldn't Nothing below has to do with the main frame so I'm not sure what this comment is about. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1084: container = document->getElementById("scroller1"); better to have two variables in this test, scroller1 and scroller2, rather than reusing container for both. Ditto for the ScrollableAreas. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1199: Please add a test to make sure that a style that would otherwise be non-composited, wont have a NonCompositedScrollReason when will-change transform is added. e.g. .style { opacity: 0.5; will-change: transform; } Will composite so should not have a NonCompositing reason.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=StyleRelatedMainThreadScrollingReason.*; EventHandlerTest.NonCompositedMainThreadScrollingReasonTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel; master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Thanks David for helping me to get away from the dark:) PTAL. https://codereview.chromium.org/2773593005/diff/200001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/200001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:87: // unless we force the promotion. e.g. Apply will-change:transform. On 2017/04/06 21:45:09, bokan wrote: > I would remove the first two lines. To me it implies that the reason may be set > even if we force promotion. The lines below capture all that's needed. Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:529: On 2017/04/06 21:45:09, bokan wrote: > Please add a case with some nested scrollers, an example: > > A <--NonCompositedReasonA > B <--Composited > C <--NonCompositingReasonB > > To make sure we report both A and B > > As well as a case in which all the nested scrollers are composited, make sure we > don't report anything. > > Another good one would be to make sure we don't report anything if there's non > scrollable boxes either in between scrollers, or there are no scrollers at all. Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:202: // composited. Either way, we have recorded either the reasons that On 2017/04/06 21:45:09, bokan wrote: > nit: "reasons that stored" -> "reasons stored" Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:221: nonCompositedMainThreadScrollingReasons = On 2017/04/06 21:45:09, bokan wrote: > this should be |=, right? Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:238: DCHECK(MainThreadScrollingReason::NonCompositedScrollReasons(reasons)); On 2017/04/06 21:45:09, bokan wrote: > It wouldn't hurt to DCHECK that we don't have any mainThread or CompositorSet > reasons. Current DCHECK guarantees that the reason doesn't include MainThreadCanSet or CompositedCanSet reasons. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:240: uint32_t mainThreadScrollingReasonEnumMax = On 2017/04/06 21:45:10, bokan wrote: > unused It's used in DEFINE_STATIC_LOCAL. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1055: webViewImpl()->settings()->setPreferCompositingToLCDTextEnabled(false); On 2017/04/06 21:45:10, bokan wrote: > We should probably have a test that runs with this flag on. The last part of this function tests with the flag on. The LCDTextRelatedReasons would only be set when the flag is off. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1062: // Add or remove style related attribute from a scroller shouldn't On 2017/04/06 21:45:10, bokan wrote: > Nothing below has to do with the main frame so I'm not sure what this comment is > about. Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1084: container = document->getElementById("scroller1"); On 2017/04/06 21:45:10, bokan wrote: > better to have two variables in this test, scroller1 and scroller2, rather than > reusing container for both. Ditto for the ScrollableAreas. Done. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1199: On 2017/04/06 21:45:10, bokan wrote: > Please add a test to make sure that a style that would otherwise be > non-composited, wont have a NonCompositedScrollReason when will-change transform > is added. e.g. > > .style { > opacity: 0.5; > will-change: transform; > } > > Will composite so should not have a NonCompositing reason. Done.
BTW, if you're going to rebase your local branch, rebase first and upload the rebased version of your old patch first - then apply new changes. This makes it easier for reviewers to see at a glance what your changes from the last patchset are (currently diffing against the last patch includes unrelated changes that happened on the tree). https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:238: DCHECK(MainThreadScrollingReason::NonCompositedScrollReasons(reasons)); On 2017/04/07 16:35:32, yigu wrote: > On 2017/04/06 21:45:09, bokan wrote: > > It wouldn't hurt to DCHECK that we don't have any mainThread or CompositorSet > > reasons. > > Current DCHECK guarantees that the reason doesn't include MainThreadCanSet or > CompositedCanSet reasons. Ah, my bad, I didn't pay close enough attention to how these methods work. Now that I take a look though I think we should change it to my understanding (which is what the other call site uses). See my comment in the function definition. It means you'll have to also DCHECK here that we don't have any of the other types here. https://codereview.chromium.org/2773593005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:240: uint32_t mainThreadScrollingReasonEnumMax = On 2017/04/07 16:35:32, yigu wrote: > On 2017/04/06 21:45:10, bokan wrote: > > unused > > It's used in DEFINE_STATIC_LOCAL. Doh https://codereview.chromium.org/2773593005/diff/220001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/220001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:94: (non_composited_reasons == (reasons | non_composited_reasons)); This function is slightly different from the two above. Those want to check if the given reasons are the *only* reasons. I think this one is more useful to check if we have *any* NonComposited. Right now, I think it's checking that it has a reason of any kind AND all reasons are NonComposited reasons. Lets call it hasNonCompositedScrollReason and change the condition to: return (reasons & non_composited_reasons) And also update the comment to reflect this. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (left): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:799: MainThreadScrollingReasons mainThreadScrollingReasons() const; We should probably DCHECK in this and the below method they don't have any non-composited reasons. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:535: box->setAttribute("class", "hidden"); This is testing my request for non-scrollable areas right? Please split it out into a separate test. Long tests that test multiple things are hard to reason about and difficult to debug when things break. Prefer many small, specific tests to few large ones. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:544: histogramTester.expectBucketCount("Renderer4.MainThreadWheelScrollReason", 17, Please use names for the bucket constants here. You can just define a function above that converts the enums to bucket index. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:551: "<style>.container { overflow:scroll; width: 200px; height: 200px; }" nit: .container should be on new line https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:560: " <div class='box with-border-radius'>" You're testing the non-scrollable case here as well as mixing composited/non-composited. Split up into separate tests. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:588: container->setAttribute("class", "composited"); Ditto for the will-change bit here. Split it out into a separate test. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1073: // Different scrollable area should keep unaffected. nit: keep -> remain https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1114: container->setAttribute("style", "will-change:transform"); Again, split this out into a separate test that runs with the flag turned off. https://codereview.chromium.org/2773593005/diff/220001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2773593005/diff/220001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:561: DCHECK(!cc::MainThreadScrollingReason::NonCompositedScrollReasons(reasons)); I think this check isn't what you want. The function currently currently means (has any reason && all reasons are non-composited) so you're checking: !(has any reason) || !(all reasons are non-composited) Which is equivalent to: No reasons || has some composited reasons See my comment in NonCompositedScrollReasons() for how to address this.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel; master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Thanks David. PTAL. https://codereview.chromium.org/2773593005/diff/220001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/220001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:94: (non_composited_reasons == (reasons | non_composited_reasons)); On 2017/04/07 17:39:03, bokan wrote: > This function is slightly different from the two above. Those want to check if > the given reasons are the *only* reasons. > > I think this one is more useful to check if we have *any* NonComposited. Right > now, I think it's checking that it has a reason of any kind AND all reasons are > NonComposited reasons. > > Lets call it hasNonCompositedScrollReason and change the condition to: > > return (reasons & non_composited_reasons) > > And also update the comment to reflect this. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (left): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:799: MainThreadScrollingReasons mainThreadScrollingReasons() const; On 2017/04/07 17:39:03, bokan wrote: > We should probably DCHECK in this and the below method they don't have any > non-composited reasons. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:535: box->setAttribute("class", "hidden"); On 2017/04/07 17:39:03, bokan wrote: > This is testing my request for non-scrollable areas right? Please split it out > into a separate test. Long tests that test multiple things are hard to reason > about and difficult to debug when things break. Prefer many small, specific > tests to few large ones. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:544: histogramTester.expectBucketCount("Renderer4.MainThreadWheelScrollReason", 17, On 2017/04/07 17:39:03, bokan wrote: > Please use names for the bucket constants here. You can just define a function > above that converts the enums to bucket index. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:551: "<style>.container { overflow:scroll; width: 200px; height: 200px; }" On 2017/04/07 17:39:03, bokan wrote: > nit: .container should be on new line Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:560: " <div class='box with-border-radius'>" On 2017/04/07 17:39:03, bokan wrote: > You're testing the non-scrollable case here as well as mixing > composited/non-composited. Split up into separate tests. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:588: container->setAttribute("class", "composited"); On 2017/04/07 17:39:03, bokan wrote: > Ditto for the will-change bit here. Split it out into a separate test. Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1073: // Different scrollable area should keep unaffected. On 2017/04/07 17:39:03, bokan wrote: > nit: keep -> remain Done. https://codereview.chromium.org/2773593005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1114: container->setAttribute("style", "will-change:transform"); On 2017/04/07 17:39:03, bokan wrote: > Again, split this out into a separate test that runs with the flag turned off. Done. https://codereview.chromium.org/2773593005/diff/220001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2773593005/diff/220001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:561: DCHECK(!cc::MainThreadScrollingReason::NonCompositedScrollReasons(reasons)); On 2017/04/07 17:39:03, bokan wrote: > I think this check isn't what you want. The function currently currently means > (has any reason && all reasons are non-composited) so you're checking: > > !(has any reason) || !(all reasons are non-composited) > > Which is equivalent to: > > No reasons || has some composited reasons > > See my comment in NonCompositedScrollReasons() for how to address this. Done.
Ok, everything looks good. The tests are just a little hard to follow so I've left some comments on how to make them more reader-friendly. Otherwise, I think we're good to go. https://codereview.chromium.org/2773593005/diff/240001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/240001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:86: // Returns true if we have any NonCompostedScrollReasons. Rename the function HasNonCompositedScrollReasons. The comment should be more than the function name, e.g. "Returns true if there are any reasons that prevented the scroller from being composited" https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:118: if (reason & (reason - 1)) I had to google what this does. A clearer way to ensure we only have a single reason is: int index = 1; while (!(reason & 1)) { reason >>= 1; ++index; } DCHECK(reason == 1); return index; And DCHECK is preferable to returning error condition. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:527: document().frame()->eventHandler().handleGestureEvent(touchScrollEnd); I'd throw two GestureScrollUpdates between this, just to be sure that we aren't counting in each Update. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:539: "Renderer4.MainThreadWheelScrollReason", You can improve readability of your expectations with a macro #define EXPECT_WHEEL_BUCKET(reason, count) \ histogramTester.expectBucketCount("Renderer4.MainThreadScrollReason", \ getBucketIndex(MainThreadScrollingReason::##reason), count); So that your checks look like this: EXPECT_WHEEL_BUCKET(kHasOpacityAndLCDText, 1); And equivalently for touch. *disclaimer: my macro skills are rusty so you may have to jig the above a bit. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:544: MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText), Why do we get this reason for touch but not wheel? Oh, or do we but you just didn't check above? You should check it for touch as well. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:563: document().view()->setParentVisible(true); Why is setParentVisible and setSelfVisible needed? I've never seen these used. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:573: document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd); Another way to make this more readable is to wrap up these wheel and touch events into a method on the class (you can subclass EventHandlerTest into NonCompositedMainThreadScrollingReasonTest, might help shorted your test names too). You should also pass in the Element you want to scroll so it's clear what the scroll is targeting, e.g.: Element* box = document().getElementById("box"); wheelScroll(box); And the method can figure out the Element's position. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:582: document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd); Lets also EXPECT box's PLSA::getNonCompositedMainThreadScrollReasons is 0 https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:585: getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1); It'd be safer to check that the total count didn't change here. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:618: histogramTester.expectBucketCount( Check total count. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:656: histogramTester.expectBucketCount( This test is a little more complex, some comments above each expectation would help the reader know where each one is coming from. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:658: getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1); This one comes from "translucent box" right? Isn't that one unscrollable? Since it's the same size as its child and its child is scaled down even...? https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:662: MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText), This one comes from all of them, right? https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1098: !(reason & ~m_LCDTextRelatedReasons)) { We always provide just one reason right? This if could probably just check the first part of the && https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1140: uint32_t reason = MainThreadScrollingReason::kHasClipRelatedProperty; reason -> clipReason or similar. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1216: ASSERT_FALSE(scrollableArea->getNonCompositedMainThreadScrollingReasons() & Just ASSERT_FALSE(getNonCompositedMainThreadScrollingReasons())
Hi David. Thanks for the feedback especially for the readability part! The unit tests look much better now. PTAL. https://codereview.chromium.org/2773593005/diff/240001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/240001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:86: // Returns true if we have any NonCompostedScrollReasons. On 2017/04/07 21:05:56, bokan wrote: > Rename the function HasNonCompositedScrollReasons. The comment should be more > than the function name, e.g. "Returns true if there are any reasons that > prevented the scroller from being composited" Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:118: if (reason & (reason - 1)) On 2017/04/07 21:05:57, bokan wrote: > I had to google what this does. A clearer way to ensure we only have a single > reason is: > > int index = 1; > while (!(reason & 1)) { > reason >>= 1; > ++index; > } > DCHECK(reason == 1); > return index; > > And DCHECK is preferable to returning error condition. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:527: document().frame()->eventHandler().handleGestureEvent(touchScrollEnd); On 2017/04/07 21:05:56, bokan wrote: > I'd throw two GestureScrollUpdates between this, just to be sure that we aren't > counting in each Update. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:539: "Renderer4.MainThreadWheelScrollReason", On 2017/04/07 21:05:57, bokan wrote: > You can improve readability of your expectations with a macro > > #define EXPECT_WHEEL_BUCKET(reason, count) \ > histogramTester.expectBucketCount("Renderer4.MainThreadScrollReason", \ > getBucketIndex(MainThreadScrollingReason::##reason), count); > > So that your checks look like this: > > EXPECT_WHEEL_BUCKET(kHasOpacityAndLCDText, 1); > > And equivalently for touch. > > *disclaimer: my macro skills are rusty so you may have to jig the above a bit. It looks MUCH better with this improvement. Thanks for the suggestion! btw, here we should remove [##] ahead of reason otherwise we would get an error message "pasting formed '::kHasOpacityAndLCDText', an invalid preprocessing token" https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:544: MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText), On 2017/04/07 21:05:57, bokan wrote: > Why do we get this reason for touch but not wheel? Oh, or do we but you just > didn't check above? You should check it for touch as well. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:563: document().view()->setParentVisible(true); On 2017/04/07 21:05:57, bokan wrote: > Why is setParentVisible and setSelfVisible needed? I've never seen these used. We have to use these to make sure the test box gets composited. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:573: document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd); On 2017/04/07 21:05:57, bokan wrote: > Another way to make this more readable is to wrap up these wheel and touch > events into a method on the class (you can subclass EventHandlerTest into > NonCompositedMainThreadScrollingReasonTest, might help shorted your test names > too). You should also pass in the Element you want to scroll so it's clear what > the scroll is targeting, e.g.: > > Element* box = document().getElementById("box"); > wheelScroll(box); > > And the method can figure out the Element's position. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:582: document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd); On 2017/04/07 21:05:57, bokan wrote: > Lets also EXPECT box's PLSA::getNonCompositedMainThreadScrollReasons is 0 Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:585: getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1); On 2017/04/07 21:05:57, bokan wrote: > It'd be safer to check that the total count didn't change here. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:618: histogramTester.expectBucketCount( On 2017/04/07 21:05:57, bokan wrote: > Check total count. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:656: histogramTester.expectBucketCount( On 2017/04/07 21:05:57, bokan wrote: > This test is a little more complex, some comments above each expectation would > help the reader know where each one is coming from. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:658: getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1); On 2017/04/07 21:05:57, bokan wrote: > This one comes from "translucent box" right? Isn't that one unscrollable? Since > it's the same size as its child and its child is scaled down even...? It's actually scrollable (see jsbin.com/muluhes). https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:662: MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText), On 2017/04/07 21:05:57, bokan wrote: > This one comes from all of them, right? In this case yes. Usually we set this reason whenever we have a not opaque background. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1098: !(reason & ~m_LCDTextRelatedReasons)) { On 2017/04/07 21:05:57, bokan wrote: > We always provide just one reason right? This if could probably just check the > first part of the && We may pass multiple reasons like the test LCDTextEnabledTest below. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1140: uint32_t reason = MainThreadScrollingReason::kHasClipRelatedProperty; On 2017/04/07 21:05:57, bokan wrote: > reason -> clipReason or similar. Done. https://codereview.chromium.org/2773593005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1216: ASSERT_FALSE(scrollableArea->getNonCompositedMainThreadScrollingReasons() & On 2017/04/07 21:05:57, bokan wrote: > Just ASSERT_FALSE(getNonCompositedMainThreadScrollingReasons()) Done.
Ok, this is lgtm from me with two final changes. Please have either tdresser@ or flackr@ look it over to get their ok for the metrics gathering point of view. https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:101: const WebGestureDevice = kWebGestureDeviceTouchpad); No default param. It should be clear at the call site which device we're scrolling on. https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:176: IntPoint(box->Location().X().ToInt() + box->size().Width().ToInt() / 2, Location() is relative to its container, which is not what you want. You need relative to the root frame. In that case, you should use Element::getBoundingClientRect
https://codereview.chromium.org/2773593005/diff/280001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/280001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:92: kHasClipRelatedProperty | kHasBoxShadowFromNonRootLayer; Can we just do some math with the first & last values? https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:42: count); I'm not sure it's worth factoring these out. Could we declare a few constants (histogram name, bucket index), instead? https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:598: uint32_t non_composited_main_thread_scrolling_reasons_; Is "non composited" redundant with "main thread"? https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1078: Element* container_2 = document->GetElementById("scroller2"); I think container2 would be more typical. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1084: ASSERT_FALSE( Many of the ASSERTs in this file should be EXPECTs. Might as well at least use EXPECT where appropriate for new usages.
https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:42: count); On 2017/04/11 18:50:47, tdresser wrote: > I'm not sure it's worth factoring these out. > Could we declare a few constants (histogram name, bucket index), instead? I asked for this so I'll explain my reasoning. I like the fact that a macro like this clearly stands out from the surrounding code as checking an expectation, whereas histogram_tester.ExpectTotalCount(...) blends in with control code. It also reduces the amount of boilerplate so is much easier to read. Constants help somewhat with the latter (though harder to keep to one line) but not the former. Given that expectation macros are quite common in our tests (in fact, all gtest.h checks are macros) I'm more partial to keeping these, WDYT?
https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:223: } What is the state when we handle these events? If compositing inputs are clean, which it seems like they should be if we're checking for compositing reasons, then I think you can replace this loop with scroll_gesture_handling_node_->enclosingLayer()->ancestorScrollingLayer() https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1862: Can you add a check that the reasons are one of the non composited reasons? Otherwise we'd have a gap in recording them right?
https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:223: } On 2017/04/11 19:35:35, flackr wrote: > What is the state when we handle these events? If compositing inputs are clean, > which it seems like they should be if we're checking for compositing reasons, > then I think you can replace this loop with > scroll_gesture_handling_node_->enclosingLayer()->ancestorScrollingLayer() It looks like recomputing the scroll chain actually computes all of the ancestor scrollers in an upwards walk to the root too, so this shouldn't be too bad for performance. It is worth noting however that if compositing inputs are dirty the reasons are probably not current either, but this may be fine. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:254: main_thread_scrolling_reason_enum_max)); I notice the device in this case is only kWebGestureDeviceTouchpad, does that include mouse wheel scrolls?
https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:42: count); On 2017/04/11 19:23:26, bokan wrote: > On 2017/04/11 18:50:47, tdresser wrote: > > I'm not sure it's worth factoring these out. > > Could we declare a few constants (histogram name, bucket index), instead? > > I asked for this so I'll explain my reasoning. I like the fact that a macro like > this clearly stands out from the surrounding code as checking an expectation, > whereas histogram_tester.ExpectTotalCount(...) blends in with control code. It > also reduces the amount of boilerplate so is much easier to read. > > Constants help somewhat with the latter (though harder to keep to one line) but > not the former. Given that expectation macros are quite common in our tests (in > fact, all gtest.h checks are macros) I'm more partial to keeping these, WDYT? In general, use of macros like this is discouraged (https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros). Ideally we'd be able to write a function that returned an assertion result here, but the way histogram tester is written, we can't really do that. This would let us write: EXPECT_THAT(TouchTotalEquals(5)); Which is the best of both worlds, IMO. If you prefer the macros here, fine by me.
Thanks for the feedback! PTAL. https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:101: const WebGestureDevice = kWebGestureDeviceTouchpad); On 2017/04/11 17:30:52, bokan wrote: > No default param. It should be clear at the call site which device we're > scrolling on. Done. https://codereview.chromium.org/2773593005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:176: IntPoint(box->Location().X().ToInt() + box->size().Width().ToInt() / 2, On 2017/04/11 17:30:52, bokan wrote: > Location() is relative to its container, which is not what you want. You need > relative to the root frame. In that case, you should use > Element::getBoundingClientRect Done. https://codereview.chromium.org/2773593005/diff/280001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2773593005/diff/280001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:92: kHasClipRelatedProperty | kHasBoxShadowFromNonRootLayer; On 2017/04/11 18:50:47, tdresser wrote: > Can we just do some math with the first & last values? As discussed, introducing other logic doesn't benefit that much. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:42: count); On 2017/04/11 20:33:27, tdresser wrote: > On 2017/04/11 19:23:26, bokan wrote: > > On 2017/04/11 18:50:47, tdresser wrote: > > > I'm not sure it's worth factoring these out. > > > Could we declare a few constants (histogram name, bucket index), instead? > > > > I asked for this so I'll explain my reasoning. I like the fact that a macro > like > > this clearly stands out from the surrounding code as checking an expectation, > > whereas histogram_tester.ExpectTotalCount(...) blends in with control code. It > > also reduces the amount of boilerplate so is much easier to read. > > > > Constants help somewhat with the latter (though harder to keep to one line) > but > > not the former. Given that expectation macros are quite common in our tests > (in > > fact, all gtest.h checks are macros) I'm more partial to keeping these, WDYT? > > In general, use of macros like this is discouraged > (https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros). > > Ideally we'd be able to write a function that returned an assertion result here, > but the way histogram tester is written, we can't really do that. > This would let us write: > EXPECT_THAT(TouchTotalEquals(5)); > > Which is the best of both worlds, IMO. > > If you prefer the macros here, fine by me. Macros help to locate the exact line that causes crashes or fails expectations. For a test maybe we can just use them. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:254: main_thread_scrolling_reason_enum_max)); On 2017/04/11 20:13:03, flackr wrote: > I notice the device in this case is only kWebGestureDeviceTouchpad, does that > include mouse wheel scrolls? Mouse wheel scrolling uses kWebGestureDeviceTouchpad as device type. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1862: On 2017/04/11 19:35:36, flackr wrote: > Can you add a check that the reasons are one of the non composited reasons? > Otherwise we'd have a gap in recording them right? Done. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:598: uint32_t non_composited_main_thread_scrolling_reasons_; On 2017/04/11 18:50:47, tdresser wrote: > Is "non composited" redundant with "main thread"? This is to differentiate the reasons that caused main thread scrolling for an element but we also "composite" that element. e.g. element with fixed background attachment and "will-change:transform;". https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1078: Element* container_2 = document->GetElementById("scroller2"); On 2017/04/11 18:50:48, tdresser wrote: > I think container2 would be more typical. Done. https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1084: ASSERT_FALSE( On 2017/04/11 18:50:48, tdresser wrote: > Many of the ASSERTs in this file should be EXPECTs. > > Might as well at least use EXPECT where appropriate for new usages. Done.
https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773593005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:42: count); On 2017/04/11 20:33:27, tdresser wrote: > On 2017/04/11 19:23:26, bokan wrote: > > On 2017/04/11 18:50:47, tdresser wrote: > > > I'm not sure it's worth factoring these out. > > > Could we declare a few constants (histogram name, bucket index), instead? > > > > I asked for this so I'll explain my reasoning. I like the fact that a macro > like > > this clearly stands out from the surrounding code as checking an expectation, > > whereas histogram_tester.ExpectTotalCount(...) blends in with control code. It > > also reduces the amount of boilerplate so is much easier to read. > > > > Constants help somewhat with the latter (though harder to keep to one line) > but > > not the former. Given that expectation macros are quite common in our tests > (in > > fact, all gtest.h checks are macros) I'm more partial to keeping these, WDYT? > > In general, use of macros like this is discouraged > (https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros). > > Ideally we'd be able to write a function that returned an assertion result here, > but the way histogram tester is written, we can't really do that. > This would let us write: > EXPECT_THAT(TouchTotalEquals(5)); > > Which is the best of both worlds, IMO. > > If you prefer the macros here, fine by me. Yes, histogram_tester is unfortunate in this way. IMHO, the macro arguments in the style guide don't apply as strongly to tests and missing line numbers in test failures is unacceptable so I think macros are preferable in this case.
https://codereview.chromium.org/2773593005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1865: non_composited_main_thread_scrolling_reasons_)); Actually what we want to check here is: DCHECK(!(non_composited_main_thread_scrolling_reasons_ & ~BitMaskOfAllNonCompositedMainThreadScrollingReaons)); I.e. there are only non composited main thread scrolling reasons set.
ui/events/blink/input_handler_proxy.cc LGTM
Hi Rob. Have updated the DCHECK by adding a combination variable in main_thread_scrolling_reasons.h. PTAL. Thanks. https://codereview.chromium.org/2773593005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2773593005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1865: non_composited_main_thread_scrolling_reasons_)); On 2017/04/11 22:31:21, flackr wrote: > Actually what we want to check here is: > DCHECK(!(non_composited_main_thread_scrolling_reasons_ & > ~BitMaskOfAllNonCompositedMainThreadScrollingReaons)); > > I.e. there are only non composited main thread scrolling reasons set. Done.
lgtm
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org, jwd@chromium.org, bokan@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2773593005/#ps320001 (title: "nit")
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": 320001, "attempt_start_ts": 1492011256944620,
"parent_rev": "6051193a1a2f5d7ad673326ba398324ad4975841", "commit_rev":
"76d0929033d6cd3ec36b96d1b4845166e70c9349"}
Message was sent while issue was closed.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773593005 Cr-Commit-Position: refs/heads/master@{#464073} Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2816973002/ by jdoerrie@chromium.org. The reason for reverting is: Likely cause of webkit_unit_tests failing on chromium.webkit/WebKit Android (Nexus4). BUG=711190 .
Message was sent while issue was closed.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773593005 Cr-Commit-Position: refs/heads/master@{#464073} Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484... ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773593005 Cr-Commit-Position: refs/heads/master@{#464073} Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484... ==========
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, bokan@chromium.org, ajuma@chromium.org, tdresser@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2773593005/#ps340001 (title: "fix incorrect test for android devices")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/13 08:09:17, jdoerrie wrote: > A revert of this CL (patchset #17 id:320001) has been created in > https://codereview.chromium.org/2816973002/ by mailto:jdoerrie@chromium.org. > > The reason for reverting is: Likely cause of webkit_unit_tests failing on > chromium.webkit/WebKit Android (Nexus4). > > BUG=711190 > . In the unit test that failed, we had an element that was not scrollable on Android devices which caused the failure. Making it scrollable fixes the problem.
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492098462147210,
"parent_rev": "1560052b7f6f7ccd994b0fd03c3516cd625aeab9", "commit_rev":
"b797bf2e48843177bc5657622bb7fd1f09bb9e24"}
Message was sent while issue was closed.
Description was changed from ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773593005 Cr-Commit-Position: refs/heads/master@{#464073} Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484... ========== to ========== Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager Due to crbug.com/701355, all style related main thread scrolling reasons stop being recorded. This patch moves the logic of storing the style related reasons to PLSA and move the recording logic to ScrollManager because the information on the compositor side is insufficient. The disabled test due to the previous bug has been enabled again with minor modification. BUG=704805 TEST=All/NonCompositedMainThreadScrollingReasonTest.*; EventHandlerTest.NonCompositedMainThreadScrollingReason* CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773593005 Cr-Original-Commit-Position: refs/heads/master@{#464073} Committed: https://chromium.googlesource.com/chromium/src/+/76d0929033d6cd3ec36b96d1b484... Review-Url: https://codereview.chromium.org/2773593005 Cr-Commit-Position: refs/heads/master@{#464461} Committed: https://chromium.googlesource.com/chromium/src/+/b797bf2e48843177bc5657622bb7... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/b797bf2e48843177bc5657622bb7... |
