Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773893003:
Record size of scroller that user scrolls (Closed)
DescriptionRecord size of scroller that user scrolls
This patch adds 2 UMA metrics that monitors the sizes of scrollers that
user scrolls. One metric is for wheel scroll and the other is for touch
scroll. Given that the purpose of this experiment is focused on small
scrollers, we don't care about any scroller that is larger than 200000
squared px.
BUG=684631
TEST=
EventHandlerTest.ScrollerSizeOfMainThreadScrollingHistogramRecordingTest;
LayerTreeHostImplTest.ScrollerSizeOfCCScrollingHistogramRecordingTest;
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2773893003
Cr-Commit-Position: refs/heads/master@{#466315}
Committed: https://chromium.googlesource.com/chromium/src/+/43525ec7a4ebf72cc88ad3edfad53b5be3c59b61
Patch Set 1 #Patch Set 2 : Use scrollBegin gesture event to test UMA metrics #
Total comments: 2
Patch Set 3 : Redefine metrics using suffixes in histograms.xml #
Total comments: 14
Patch Set 4 : Address comments #Patch Set 5 : bug fix #
Total comments: 5
Patch Set 6 : nit #
Total comments: 2
Patch Set 7 : Change constants location #Patch Set 8 : nit #Patch Set 9 : Move the constants to a separate header #Patch Set 10 : Add scrollEnd event after scrollBegin in test #
Total comments: 13
Patch Set 11 : Logic update #
Total comments: 14
Patch Set 12 : Address comments && add unit tests #
Total comments: 13
Patch Set 13 : Rename forwarding headers && update recording condition checks #Patch Set 14 : nit #
Total comments: 14
Patch Set 15 : Address comments #
Total comments: 1
Patch Set 16 : nit #
Total comments: 8
Patch Set 17 : nit #Messages
Total messages: 71 (24 generated)
Description was changed from ========== Record size of scroller that user scrolls BUG= ========== to ========== Record size of scroller that user scrolls BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Record size of scroller that user scrolls BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 90000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
yigu@chromium.org changed reviewers: + bokan@chromium.org, flackr@chromium.org, jwd@chromium.org, weiliangc@chromium.org
PTAL. Thanks!
https://codereview.chromium.org/2773893003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16613: + composite them even we are able to do so. Since the summary is basically duplicated, can you redefine these two histograms using histogram suffixes.
This metrics doesn't look at scrollers that never has scroll begin event. Why do we use ScrollBegin event as the place to do the counting? How does the number in scrollBegin differs from actual # of scroller on the page? Also please format your CL description: first line should be title, no more than 72 characters per line. Something like this: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
On 2017/03/27 17:45:46, weiliangc wrote: > This metrics doesn't look at scrollers that never has scroll begin event. Why do > we use ScrollBegin event as the place to do the counting? How does the number in > scrollBegin differs from actual # of scroller on the page? > > Also please format your CL description: first line should be title, no more than > 72 characters per line. Something like this: > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html We only care about the size of the scrollers that users actually scroll in this patch. There is gonna be another patch that records all scrollers on page load.
On 2017/03/27 17:45:46, weiliangc wrote: > This metrics doesn't look at scrollers that never has scroll begin event. Why do > we use ScrollBegin event as the place to do the counting? How does the number in > scrollBegin differs from actual # of scroller on the page? > > Also please format your CL description: first line should be title, no more than > 72 characters per line. Something like this: > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html We only care about the size of the scrollers that users actually scroll in this patch. There is gonna be another patch that records all scrollers on page load.
Description was changed from ========== This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 90000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 90000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
yigu@chromium.org changed reviewers: + tdresser@chromium.org
The histograms.xml has been updated with suffixes. PTAL. Thanks! https://codereview.chromium.org/2773893003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16613: + composite them even we are able to do so. On 2017/03/27 17:28:34, jwd wrote: > Since the summary is basically duplicated, can you redefine these two histograms > using histogram suffixes. Done.
https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:487: "<div class='box'><div id='content' class='spacer'></div></div>"); I suspect this string could be made a bit easier to read without too much hassle. https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:577: CustomCountHistogram, sizeHistogram, It looks like these macros are defining a static local with the same name. Don't these names need to be different? Why is this currently working? https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16595: +<histogram name="Event.Scroll.ScrollerSize.OnScroll" units="squared pixels"> The only histogram using "squared pixels" for the units is actually reporting the distance between two things in pixels, squared. That's pretty different from this. I think just saying this is in pixels would probably be reasonable. https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16601: + measuring the frequencies of different sizes of scrollers getting scrolled. measuring -> measure https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16603: + composite them even we are able to do so. even -> even if
https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2758: scrolling_node->scroll_clip_layer_bounds.GetArea(), 1, 90000, 50); Use a pair of constants for the 90000 and 50 and share with ScrollManager.cpp so that we don't accidentally end up reporting with different buckets. https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:578: ("Event.Scroll.ScrollerSize.OnScroll_Wheel", 1, 90000, 50)); As I mentioned in the cc file we should use constants for the max and number of buckets. I'm not sure if 90000 is the right maximum for small scrollers, I'd recommend about 200000 to cover any scroller that's smaller than 500x400 at and above which size you could argue it is intended to be a/the main scroller. Alternately, we could have a maximum that goes well above what we would consider to be large scrollers which would help us to determine the relative proportion of small scroller scrolling vs large scroller scrolling. How will we tell if there are small scrollers that do not get scrolled (i.e. prime candidates to not save layers for)? I think we should also record scroller sizes that we see occur on pages. https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16595: +<histogram name="Event.Scroll.ScrollerSize.OnScroll" units="squared pixels"> On 2017/03/28 13:30:12, tdresser wrote: > The only histogram using "squared pixels" for the units is actually reporting > the distance between two things in pixels, squared. > > That's pretty different from this. > > I think just saying this is in pixels would probably be reasonable. To be clear, you mean saying the units are pixels but still measuring the area right? The nice thing about area is it is linearly correlated with GPU memory usage.
On 2017/03/28 14:40:23, flackr wrote: > https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:2758: > scrolling_node->scroll_clip_layer_bounds.GetArea(), 1, 90000, 50); > Use a pair of constants for the 90000 and 50 and share with ScrollManager.cpp so > that we don't accidentally end up reporting with different buckets. > > https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): > > https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/ScrollManager.cpp:578: > ("Event.Scroll.ScrollerSize.OnScroll_Wheel", 1, 90000, 50)); > As I mentioned in the cc file we should use constants for the max and number of > buckets. > > I'm not sure if 90000 is the right maximum for small scrollers, I'd recommend > about 200000 to cover any scroller that's smaller than 500x400 at and above > which size you could argue it is intended to be a/the main scroller. > Alternately, we could have a maximum that goes well above what we would consider > to be large scrollers which would help us to determine the relative proportion > of small scroller scrolling vs large scroller scrolling. > > How will we tell if there are small scrollers that do not get scrolled (i.e. > prime candidates to not save layers for)? I think we should also record scroller > sizes that we see occur on pages. > > https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:16595: +<histogram > name="Event.Scroll.ScrollerSize.OnScroll" units="squared pixels"> > On 2017/03/28 13:30:12, tdresser wrote: > > The only histogram using "squared pixels" for the units is actually reporting > > the distance between two things in pixels, squared. > > > > That's pretty different from this. > > > > I think just saying this is in pixels would probably be reasonable. > > To be clear, you mean saying the units are pixels but still measuring the area > right? The nice thing about area is it is linearly correlated with GPU memory > usage. Yeah. A "pixel" can be either a measure of length or area, which is a bit confusing.
yigu@chromium.org changed reviewers: + pdr@chromium.org
Have updated the patch. PTAL. Thanks! https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2758: scrolling_node->scroll_clip_layer_bounds.GetArea(), 1, 90000, 50); On 2017/03/28 14:40:23, flackr wrote: > Use a pair of constants for the 90000 and 50 and share with ScrollManager.cpp so > that we don't accidentally end up reporting with different buckets. Done. https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:487: "<div class='box'><div id='content' class='spacer'></div></div>"); On 2017/03/28 13:30:12, tdresser wrote: > I suspect this string could be made a bit easier to read without too much > hassle. Done. https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:577: CustomCountHistogram, sizeHistogram, On 2017/03/28 13:30:12, tdresser wrote: > It looks like these macros are defining a static local with the same name. > > Don't these names need to be different? Why is this currently working? It's better we differentiate the names. There are existing logic that uses same histogram name (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...). Could it be static method overload? But the param type looks identical. I need to dig into the WTF::StaticSingleton. https://codereview.chromium.org/2773893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/ScrollManager.cpp:578: ("Event.Scroll.ScrollerSize.OnScroll_Wheel", 1, 90000, 50)); On 2017/03/28 14:40:23, flackr wrote: > As I mentioned in the cc file we should use constants for the max and number of > buckets. > > I'm not sure if 90000 is the right maximum for small scrollers, I'd recommend > about 200000 to cover any scroller that's smaller than 500x400 at and above > which size you could argue it is intended to be a/the main scroller. > Alternately, we could have a maximum that goes well above what we would consider > to be large scrollers which would help us to determine the relative proportion > of small scroller scrolling vs large scroller scrolling. > > How will we tell if there are small scrollers that do not get scrolled (i.e. > prime candidates to not save layers for)? I think we should also record scroller > sizes that we see occur on pages. 500*400 makes sense to me. There is gonna be another patch that adds recording scrollers on load/create and that's why I name the histograms *OnScroll*. https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16601: + measuring the frequencies of different sizes of scrollers getting scrolled. On 2017/03/28 13:30:12, tdresser wrote: > measuring -> measure Done. https://codereview.chromium.org/2773893003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16603: + composite them even we are able to do so. On 2017/03/28 13:30:12, tdresser wrote: > even -> even if Done.
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2773893003/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:173: std::make_pair(200000, 50); Name this so it's clearer what this contains. What's the first thing? What's the second? I think this will probably be clearer if we split them out of the pair, and give them both meaningful names. https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:494: // Test wheel scroll on the box Missing . https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:504: // Test touch scroll on the main frame Missing . https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollerSize.h (right): https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollerSize.h:14: #endif // ScrollerSize_h Is this file needed? https://codereview.chromium.org/2773893003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2773893003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:16600: + Record the size of a scroller upon ScrollBegin. This is intended to help us size -> area (it's just a bit clearer)
On 2017/03/30 14:41:35, tdresser wrote: > https://codereview.chromium.org/2773893003/diff/80001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2773893003/diff/80001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:173: std::make_pair(200000, 50); > Name this so it's clearer what this contains. What's the first thing? What's the > second? > > I think this will probably be clearer if we split them out of the pair, and give > them both meaningful names. > > https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:494: // Test wheel > scroll on the box > Missing . > > https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:504: // Test touch > scroll on the main frame > Missing . > > https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/scroll/ScrollerSize.h (right): > > https://codereview.chromium.org/2773893003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/scroll/ScrollerSize.h:14: #endif // > ScrollerSize_h > Is this file needed? > > https://codereview.chromium.org/2773893003/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2773893003/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:16600: + Record the size of a > scroller upon ScrollBegin. This is intended to help us > size -> area (it's just a bit clearer) Thanks Tim! Just uploaded a new patch. Unfortunately the forwarding header file "ScrollerSize.h" is necessary as using cc in Source/core is not allowed. In histograms.xml, I only changed "size" to "area" once in the description and left the metric name as-is because changing it to Event.Scroll.ScrollerArea.OnScroll may confuse people that we measure the "region" of scroller. After all, the word "area" means region in chromium most of the time. PTAL.
LGTM https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:609: static const int bucketNum = 50; Prefix with "k" https://google.github.io/styleguide/cppguide.html#Constant_Names
Please update your CL description to reflect the change in the scroller size. Also the first paragraph of your CL description is supposed to be the title of the CL. https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:609: static const int bucketNum = 50; These two are just constants. Is it possible to put it in a header file somewhere that can be included by both layer_tree_host_impl.cc and blink/core? It doesn't seem like the constants need to be living on LayerTreeHostImpl class.
Description was changed from ========== This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 90000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Record size of scroller that user scrolls This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 200000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/03/30 19:19:36, weiliangc wrote: > Please update your CL description to reflect the change in the scroller size. > > Also the first paragraph of your CL description is supposed to be the title of > the CL. > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_impl.h (right): > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_impl.h:609: static const int bucketNum = 50; > These two are just constants. Is it possible to put it in a header file > somewhere that can be included by both layer_tree_host_impl.cc and blink/core? > It doesn't seem like the constants need to be living on LayerTreeHostImpl class. Have moved the constants to input_handler.h which makes more sense?
metrics LGTM
On 2017/03/30 at 20:07:23, yigu wrote: > On 2017/03/30 19:19:36, weiliangc wrote: > > Please update your CL description to reflect the change in the scroller size. > > > > Also the first paragraph of your CL description is supposed to be the title of > > the CL. > > > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > > File cc/trees/layer_tree_host_impl.h (right): > > > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > > cc/trees/layer_tree_host_impl.h:609: static const int bucketNum = 50; > > These two are just constants. Is it possible to put it in a header file > > somewhere that can be included by both layer_tree_host_impl.cc and blink/core? > > It doesn't seem like the constants need to be living on LayerTreeHostImpl class. > > Have moved the constants to input_handler.h which makes more sense? I mean could the values live in a separate header file that is not attached to any class?
On 2017/03/30 21:05:36, weiliangc wrote: > On 2017/03/30 at 20:07:23, yigu wrote: > > On 2017/03/30 19:19:36, weiliangc wrote: > > > Please update your CL description to reflect the change in the scroller > size. > > > > > > Also the first paragraph of your CL description is supposed to be the title > of > > > the CL. > > > > > > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > > > File cc/trees/layer_tree_host_impl.h (right): > > > > > > > https://codereview.chromium.org/2773893003/diff/100001/cc/trees/layer_tree_ho... > > > cc/trees/layer_tree_host_impl.h:609: static const int bucketNum = 50; > > > These two are just constants. Is it possible to put it in a header file > > > somewhere that can be included by both layer_tree_host_impl.cc and > blink/core? > > > It doesn't seem like the constants need to be living on LayerTreeHostImpl > class. > > > > Have moved the constants to input_handler.h which makes more sense? > > I mean could the values live in a separate header file that is not attached to > any class? Added a new header to handle the constants. PTAL. Thanks!
Thanks, LGTM.
yigu@chromium.org changed reviewers: + enne@chromium.org
https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:521: 200000, 1); Why is this in the 200,000+ bucket? Isn't the frame here 300x400 whose area is 120,000? https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:588: layoutObject.enclosingLayer()->visualRect().width().toInt() * I think this misses a few edge cases. For starters, I think if you're hitting the documentElement, the enclosing layer will be the layer in which the entire document is drawn into rather than the viewport scroll layer which is what I think you want. You'll want to specifically check for whether enclosingLayer == PaintLayerCompositor::rootLayer and if it is use the FrameView->scrollingLayer(). The other issue I see is that the enclosingLayer may have been created for a reason other than overflow/scrolling. E.g. z-index causes creation of a new PaintLayer. In that case, you'd be calculating the area of the Element with z-index while the thing you're scrolling might be higher up in the layout tree. I'd look in the scrolling code to see how it bubbles up trying to scroll and duplicate that logic here. Basically, using enclosingLayer alone isn't enough. (Also, please add tests for these edge cases)
https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:588: layoutObject.enclosingLayer()->visualRect().width().toInt() * On 2017/03/31 20:23:50, bokan wrote: > I think this misses a few edge cases. For starters, I think if you're hitting > the documentElement, the enclosing layer will be the layer in which the entire > document is drawn into rather than the viewport scroll layer which is what I > think you want. You'll want to specifically check for whether enclosingLayer == > PaintLayerCompositor::rootLayer and if it is use the > FrameView->scrollingLayer(). I think this is probably the reason your frame scrolling test is returning the 200,000 bucket.
https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:521: 200000, 1); On 2017/03/31 20:23:50, bokan wrote: > Why is this in the 200,000+ bucket? Isn't the frame here 300x400 whose area is > 120,000? IntPoint(200, 200) is the scrolling start position. Given that the box is 100*100, we are scrolling the main scroller which contains the entire viewport. https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:588: layoutObject.enclosingLayer()->visualRect().width().toInt() * On 2017/03/31 20:23:50, bokan wrote: > I think this misses a few edge cases. For starters, I think if you're hitting > the documentElement, the enclosing layer will be the layer in which the entire > document is drawn into rather than the viewport scroll layer which is what I > think you want. You'll want to specifically check for whether enclosingLayer == > PaintLayerCompositor::rootLayer and if it is use the > FrameView->scrollingLayer(). > > The other issue I see is that the enclosingLayer may have been created for a > reason other than overflow/scrolling. E.g. z-index causes creation of a new > PaintLayer. In that case, you'd be calculating the area of the Element with > z-index while the thing you're scrolling might be higher up in the layout tree. > I'd look in the scrolling code to see how it bubbles up trying to scroll and > duplicate that logic here. > > Basically, using enclosingLayer alone isn't enough. > > (Also, please add tests for these edge cases) We basically don't care about the main scroller as it's too big (viewport) and we have a layer created for it anyway. The z-index thing I need to test.
cc lgtm % other people's overall review
https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:521: 200000, 1); On 2017/03/31 20:38:32, yigu wrote: > On 2017/03/31 20:23:50, bokan wrote: > > Why is this in the 200,000+ bucket? Isn't the frame here 300x400 whose area is > > 120,000? > > IntPoint(200, 200) is the scrolling start position. Given that the box is > 100*100, we are scrolling the main scroller which contains the entire viewport. Right, but the viewport is 300x400 which is < 200,000 https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:588: layoutObject.enclosingLayer()->visualRect().width().toInt() * On 2017/03/31 20:38:32, yigu wrote: > On 2017/03/31 20:23:50, bokan wrote: > > I think this misses a few edge cases. For starters, I think if you're hitting > > the documentElement, the enclosing layer will be the layer in which the entire > > document is drawn into rather than the viewport scroll layer which is what I > > think you want. You'll want to specifically check for whether enclosingLayer > == > > PaintLayerCompositor::rootLayer and if it is use the > > FrameView->scrollingLayer(). > > > > The other issue I see is that the enclosingLayer may have been created for a > > reason other than overflow/scrolling. E.g. z-index causes creation of a new > > PaintLayer. In that case, you'd be calculating the area of the Element with > > z-index while the thing you're scrolling might be higher up in the layout > tree. > > I'd look in the scrolling code to see how it bubbles up trying to scroll and > > duplicate that logic here. > > > > Basically, using enclosingLayer alone isn't enough. > > > > (Also, please add tests for these edge cases) > > We basically don't care about the main scroller as it's too big (viewport) and > we have a layer created for it anyway. The z-index thing I need to test. If we don't care about the viewport, we should avoid recording it at all rather than muddying the metric with it. You'll currently be over-reporting large scrollers because you're recording the size of the *contents* for the viewport, rather than the viewport size.
https://codereview.chromium.org/2773893003/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2766: if (!scroll_on_main_thread && scrolling_node) { if you're going to avoid recording the viewport (aka root layer) on the main thread as mentioned in ScrollManager, then you'll have to avoid recording it here as well.
https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size... cc/input/scroller_size.h:11: static const int kMaxScrollerSize = 200000; // 400px * 500px nit: Approximately 400px * 500px https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:580: if (!layoutObject.enclosingLayer()) I'm confused, is the scroll gesture handling node an arbitrary descendant of the scroller? If so, we need to walk up to the nearest ancestor scroller. It seems like this method should have a DCHECK(layoutObject.scrollableArea())
On 2017/04/12 15:31:04, flackr wrote: > https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size.h > File cc/input/scroller_size.h (right): > > https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size... > cc/input/scroller_size.h:11: static const int kMaxScrollerSize = 200000; // > 400px * 500px > nit: Approximately 400px * 500px > > https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): > > https://codereview.chromium.org/2773893003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.cpp:580: if > (!layoutObject.enclosingLayer()) > I'm confused, is the scroll gesture handling node an arbitrary descendant of the > scroller? If so, we need to walk up to the nearest ancestor scroller. It seems > like this method should have a > DCHECK(layoutObject.scrollableArea()) Yes. We should definitely walk up. Once the other patch that records main thread scrolling reasons is landed I can reuse the logic there. It's not easy to upstream.
I've updated the logic using the idea from the main thread scrolling reasons recording patch. To reduce noise, scrolling the root scroller won't be recorded. PTAL. Thanks! https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size... cc/input/scroller_size.h:11: static const int kMaxScrollerSize = 200000; // 400px * 500px On 2017/04/12 15:31:04, flackr wrote: > nit: Approximately 400px * 500px Done.
yigu@chromium.org changed reviewers: - pdr@chromium.org
https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size... cc/input/scroller_size.h:11: static const int kMaxScrollerSize = 200000; // 400px * 500px On 2017/04/13 19:11:41, yigu wrote: > On 2017/04/12 15:31:04, flackr wrote: > > nit: Approximately 400px * 500px > > Done. Eh...why "approximately" and not "exactly"? https://codereview.chromium.org/2773893003/diff/200001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/200001/cc/input/scroller_size... cc/input/scroller_size.h:12: static const int kBucketNum = 50; This name needs to be more specific. I would also replace const with constexpr. https://codereview.chromium.org/2773893003/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2800: !viewport()->MainScrollLayer()) { It'd be safer to check that the node is neither the inner, nor the outer, viewport layer -- rather than viewport()->MainScrollLayer() Also, right now, this is just checking that no viewport layer exists (where I think you mean to compare to scrolling_node) so you'll never record here. Please add a test that checks the CC side of the metric too. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:609: // Test touch scroll on the main frame. You're not trying to scroll the main frame here though, right? https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:618: Scroll(body, kWebGestureDeviceTouchscreen); In general, this won't work since Scroll() will send an event to the middle of the given box - it doesn't meant that box will be scrolled (e.g. .box or .container might be at the middle coordinate). This may or may not work in this case but you may want to add ASSERTS to check that the box that you're expecting to scroll actually got scrolled. Both for your own assurance and the reader's https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:203: int& scroller_size) { Style guide (https://google.github.io/styleguide/cppguide.html#Functions) says output parameters should be pointers. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:225: scroller_size = cur_box->VisualOverflowRect().Width().ToInt() * Are we looking to record the size of the scroller's box? Or the size of its content? i.e. in web API terminology, the clientHeight or the scrollHeight? In any case, I don't think you want to use VisualOverflowRect, that has to do with painting and clipping - it may sometimes correspond to what you want but there's lots of special cases. It's be more straightforward to use ScrollableArea::VisibleContentRect (if you want "clientHeight") or ScrollableArea::ContentsSize() (if you want "scrollHeight") https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:248: DCHECK_GT(scroller_size, 0); This can only fail if scroller_size < 0. IMO, just make the condition above `if (scroller_size > 0)`
https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/180001/cc/input/scroller_size... cc/input/scroller_size.h:11: static const int kMaxScrollerSize = 200000; // 400px * 500px On 2017/04/18 19:24:38, bokan wrote: > On 2017/04/13 19:11:41, yigu wrote: > > On 2017/04/12 15:31:04, flackr wrote: > > > nit: Approximately 400px * 500px > > > > Done. > > Eh...why "approximately" and not "exactly"? Oh I was just thinking because it's an area? It could be 800x250, 100x2000, etc.
Description was changed from ========== Record size of scroller that user scrolls This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 200000 squared px. BUG=684631 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Record size of scroller that user scrolls This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 200000 squared px. BUG=684631 TEST= EventHandlerTest.ScrollerSizeOfMainThreadScrollingHistogramRecordingTest; LayerTreeHostImplTest.ScrollerSizeOfCCScrollingHistogramRecordingTest; CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
yigu@chromium.org changed reviewers: + pdr@chromium.org
Just updated some logic based on bokan@'s feedback. PTAL:) Add pdr@ for owner approval on platform/scroll/ScrollerSize.h. https://codereview.chromium.org/2773893003/diff/200001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/200001/cc/input/scroller_size... cc/input/scroller_size.h:12: static const int kBucketNum = 50; On 2017/04/18 19:24:38, bokan wrote: > This name needs to be more specific. I would also replace const with constexpr. Done. https://codereview.chromium.org/2773893003/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2800: !viewport()->MainScrollLayer()) { On 2017/04/18 19:24:38, bokan wrote: > It'd be safer to check that the node is neither the inner, nor the outer, > viewport layer -- rather than viewport()->MainScrollLayer() > > Also, right now, this is just checking that no viewport layer exists (where I > think you mean to compare to scrolling_node) so you'll never record here. Please > add a test that checks the CC side of the metric too. Done. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:609: // Test touch scroll on the main frame. On 2017/04/18 19:24:39, bokan wrote: > You're not trying to scroll the main frame here though, right? No. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:618: Scroll(body, kWebGestureDeviceTouchscreen); On 2017/04/18 19:24:39, bokan wrote: > In general, this won't work since Scroll() will send an event to the middle of > the given box - it doesn't meant that box will be scrolled (e.g. .box or > .container might be at the middle coordinate). This may or may not work in this > case but you may want to add ASSERTS to check that the box that you're expecting > to scroll actually got scrolled. Both for your own assurance and the reader's In general it may not work. But in the example above, scrolling in the center of an element works. I added an ASSERT as you suggested in the function Scroll to guarantee that. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:203: int& scroller_size) { On 2017/04/18 19:24:39, bokan wrote: > Style guide (https://google.github.io/styleguide/cppguide.html#Functions) says > output parameters should be pointers. Done. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:225: scroller_size = cur_box->VisualOverflowRect().Width().ToInt() * On 2017/04/18 19:24:39, bokan wrote: > Are we looking to record the size of the scroller's box? Or the size of its > content? i.e. in web API terminology, the clientHeight or the scrollHeight? > > In any case, I don't think you want to use VisualOverflowRect, that has to do > with painting and clipping - it may sometimes correspond to what you want but > there's lots of special cases. It's be more straightforward to use > ScrollableArea::VisibleContentRect (if you want "clientHeight") or > ScrollableArea::ContentsSize() (if you want "scrollHeight") I think we should record the size of the scroller's box. https://codereview.chromium.org/2773893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:248: DCHECK_GT(scroller_size, 0); On 2017/04/18 19:24:39, bokan wrote: > This can only fail if scroller_size < 0. IMO, just make the condition above `if > (scroller_size > 0)` I was thinking that we should crash it if we have negative scroller_size returned. Checking scroller_size > 0 also works I think.
Overall looks good but I think we should make it clear that these new files are just for metrics. Also one question about kMaxScrollerSize. https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:5: #ifndef CC_INPUT_SCROLLER_SIZE_H_ Nit: scroller_size_metrics.h? https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px I don't understand what this constant is for. Why do we need a maximum scroller size for the metrics? https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.h:114: void ComputeScrollRelatedMetrics(uint32_t*, int*); Can you add some names here about what these inputs are? https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollerSize.h (right): https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollerSize.h:5: #ifndef ScrollerSize_h Similarly, ScrollerSizeMetrics.h?
https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px On 2017/04/19 21:03:50, pdr. wrote: > I don't understand what this constant is for. Why do we need a maximum scroller > size for the metrics? We are about to use the metrics and other stuff to decide whether it's worth it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly getting scrolled, we may not need to composite it. For big scrollers (e.g. larger than 400 * 500), they don't matter that much to this experiment so we don't record the exact size for those scrollers.
https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px On 2017/04/19 at 21:10:05, yigu wrote: > On 2017/04/19 21:03:50, pdr. wrote: > > I don't understand what this constant is for. Why do we need a maximum scroller > > size for the metrics? > > We are about to use the metrics and other stuff to decide whether it's worth it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly getting scrolled, we may not need to composite it. For big scrollers (e.g. larger than 400 * 500), they don't matter that much to this experiment so we don't record the exact size for those scrollers. Can you add that as a comment above the constant?
https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px On 2017/04/19 21:13:47, pdr. wrote: > On 2017/04/19 at 21:10:05, yigu wrote: > > On 2017/04/19 21:03:50, pdr. wrote: > > > I don't understand what this constant is for. Why do we need a maximum > scroller > > > size for the metrics? > > > > We are about to use the metrics and other stuff to decide whether it's worth > it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly getting > scrolled, we may not need to composite it. For big scrollers (e.g. larger than > 400 * 500), they don't matter that much to this experiment so we don't record > the exact size for those scrollers. > > Can you add that as a comment above the constant? Also max scroller size sounds a bit misleading. Maybe kScrollerSizeLargestBucket. We will still record larger scrollers but they will get capped into this bucket right? https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2773: InnerViewportScrollLayer() && OuterViewportScrollLayer() && Why do we need to have a inner and outer viewport scroll layer to record overflow scroller sizes?
On 2017/04/20 07:15:01, flackr wrote: > https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h > File cc/input/scroller_size.h (right): > > https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... > cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // > Approximately 400px * 500px > On 2017/04/19 21:13:47, pdr. wrote: > > On 2017/04/19 at 21:10:05, yigu wrote: > > > On 2017/04/19 21:03:50, pdr. wrote: > > > > I don't understand what this constant is for. Why do we need a maximum > > scroller > > > > size for the metrics? > > > > > > We are about to use the metrics and other stuff to decide whether it's worth > > it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly > getting > > scrolled, we may not need to composite it. For big scrollers (e.g. larger than > > 400 * 500), they don't matter that much to this experiment so we don't record > > the exact size for those scrollers. > > > > Can you add that as a comment above the constant? > > Also max scroller size sounds a bit misleading. Maybe > kScrollerSizeLargestBucket. We will still record larger scrollers but they will > get capped into this bucket right? > > https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_impl.cc:2773: InnerViewportScrollLayer() && > OuterViewportScrollLayer() && > Why do we need to have a inner and outer viewport scroll layer to record > overflow scroller sizes? We check that the scrolling node is neither inner nor outer viewport scroll layer to make sure we don't record the main scroller.
https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2773893003/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2773: InnerViewportScrollLayer() && OuterViewportScrollLayer() && On 2017/04/20 07:15:01, flackr wrote: > Why do we need to have a inner and outer viewport scroll layer to record > overflow scroller sizes? I think it's meant to null check the conditions below but this would be clearer and more correct like this: (!InnerViewportScrollLayer() || scrolling_node->element_id != InnerViewportScrollLayer()->element_id()) That said, I *think* this check is equivalent to !scrolling_node->scrolls_inner_viewport
Hi. I've updated the header files and the logic of excluding main scroller recording. PTAL. Thanks! https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h File cc/input/scroller_size.h (right): https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:5: #ifndef CC_INPUT_SCROLLER_SIZE_H_ On 2017/04/19 21:03:50, pdr. wrote: > Nit: scroller_size_metrics.h? Done. https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px On 2017/04/19 21:13:47, pdr. wrote: > On 2017/04/19 at 21:10:05, yigu wrote: > > On 2017/04/19 21:03:50, pdr. wrote: > > > I don't understand what this constant is for. Why do we need a maximum > scroller > > > size for the metrics? > > > > We are about to use the metrics and other stuff to decide whether it's worth > it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly getting > scrolled, we may not need to composite it. For big scrollers (e.g. larger than > 400 * 500), they don't matter that much to this experiment so we don't record > the exact size for those scrollers. > > Can you add that as a comment above the constant? Done. https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.h:114: void ComputeScrollRelatedMetrics(uint32_t*, int*); On 2017/04/19 21:03:50, pdr. wrote: > Can you add some names here about what these inputs are? Done. https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollerSize.h (right): https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollerSize.h:5: #ifndef ScrollerSize_h On 2017/04/19 21:03:50, pdr. wrote: > Similarly, ScrollerSizeMetrics.h? Done.
On 2017/04/20 at 18:14:22, yigu wrote: > Hi. I've updated the header files and the logic of excluding main scroller recording. PTAL. Thanks! > > https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size.h > File cc/input/scroller_size.h (right): > > https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... > cc/input/scroller_size.h:5: #ifndef CC_INPUT_SCROLLER_SIZE_H_ > On 2017/04/19 21:03:50, pdr. wrote: > > Nit: scroller_size_metrics.h? > > Done. > > https://codereview.chromium.org/2773893003/diff/220001/cc/input/scroller_size... > cc/input/scroller_size.h:11: static constexpr int kMaxScrollerSize = 200000; // Approximately 400px * 500px > On 2017/04/19 21:13:47, pdr. wrote: > > On 2017/04/19 at 21:10:05, yigu wrote: > > > On 2017/04/19 21:03:50, pdr. wrote: > > > > I don't understand what this constant is for. Why do we need a maximum > > scroller > > > > size for the metrics? > > > > > > We are about to use the metrics and other stuff to decide whether it's worth > > it to composite small scrollers. If a scroller (e.g. 50 * 50) is hardly getting > > scrolled, we may not need to composite it. For big scrollers (e.g. larger than > > 400 * 500), they don't matter that much to this experiment so we don't record > > the exact size for those scrollers. > > > > Can you add that as a comment above the constant? > > Done. > > https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/ScrollManager.h (right): > > https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/ScrollManager.h:114: void ComputeScrollRelatedMetrics(uint32_t*, int*); > On 2017/04/19 21:03:50, pdr. wrote: > > Can you add some names here about what these inputs are? > > Done. > > https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scroll/ScrollerSize.h (right): > > https://codereview.chromium.org/2773893003/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scroll/ScrollerSize.h:5: #ifndef ScrollerSize_h > On 2017/04/19 21:03:50, pdr. wrote: > > Similarly, ScrollerSizeMetrics.h? > > Done. LGTM
https://codereview.chromium.org/2773893003/diff/260001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2773893003/diff/260001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:759: SetupScrollAndContentsLayers(gfx::Size(800, 600)); Please use CreateBasicVirtualViewportLayers - it creates a layer tree closer to what we use in production. And sets the viewport size and some other stuff that you seem to be doing manually. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:586: "<style>" Please add <!DOCTYPE html> https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:587: " .container { width: 200px; height: 200px; overflow: scroll; }" I think the intent is to have the container scrollable, right? Right now it isn't since the box is only 100x100. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:590: " .spacer { height: 1000px;} body { height: 2000px; }" Place the body style on a new line (I almost missed it). https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:597: "<img src='image.png' class='fixed'></img>"); Why do you need a image? https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:609: histogram_tester.ExpectTotalCount("Event.Scroll.ScrollerSize.OnScroll_Wheel", Please also add after this 1) programmatically fully scroll "box" 2) scroll over "box" 3) check that we do record 40000 and not 10000 https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:621: Scroll(body, kWebGestureDeviceTouchscreen); This works but is fragile. The way Scroll() works is it sends a scroll to the middle of the given element. Because body is 2000px tall, the coordinate of the scroll gesture is way off screen. This is unrealistic. You can fix this by either: - Rearranging the example elements so that the middle of body is within the frame but not overlapped by other elements. - Adding a "bodyScrollTarget" div (child of body) that doesn't have scrolling itself and is used solely to target a specific part of the page - Let Scroll take a specific coordinate I think the second is the most clear to the reader.
https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:609: histogram_tester.ExpectTotalCount("Event.Scroll.ScrollerSize.OnScroll_Wheel", On 2017/04/20 19:27:58, bokan wrote: > Please also add after this > > 1) programmatically fully scroll "box" > 2) scroll over "box" > 3) check that we do record 40000 and not 10000 Never mind, ScrollBegin will always hit the top scroller so this doesn't work as I expected.
PTAL. Thanks! https://codereview.chromium.org/2773893003/diff/260001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2773893003/diff/260001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:759: SetupScrollAndContentsLayers(gfx::Size(800, 600)); On 2017/04/20 19:27:58, bokan wrote: > Please use CreateBasicVirtualViewportLayers - it creates a layer tree closer to > what we use in production. And sets the viewport size and some other stuff that > you seem to be doing manually. Done. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:586: "<style>" On 2017/04/20 19:27:58, bokan wrote: > Please add <!DOCTYPE html> Done. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:587: " .container { width: 200px; height: 200px; overflow: scroll; }" On 2017/04/20 19:27:58, bokan wrote: > I think the intent is to have the container scrollable, right? Right now it > isn't since the box is only 100x100. I think the container itself doesn't need to be scrollable. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:590: " .spacer { height: 1000px;} body { height: 2000px; }" On 2017/04/20 19:27:58, bokan wrote: > Place the body style on a new line (I almost missed it). Done. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:597: "<img src='image.png' class='fixed'></img>"); On 2017/04/20 19:27:58, bokan wrote: > Why do you need a image? Without a fix positioned image, the main scroller should be scrolled on the compositor. It doesn't affect the test here because we manually call ScrollBegin. Just remove it to be clear. https://codereview.chromium.org/2773893003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:621: Scroll(body, kWebGestureDeviceTouchscreen); On 2017/04/20 19:27:58, bokan wrote: > This works but is fragile. The way Scroll() works is it sends a scroll to the > middle of the given element. Because body is 2000px tall, the coordinate of the > scroll gesture is way off screen. This is unrealistic. You can fix this by > either: > > - Rearranging the example elements so that the middle of body is within the > frame but not overlapped by other elements. > - Adding a "bodyScrollTarget" div (child of body) that doesn't have scrolling > itself and is used solely to target a specific part of the page > - Let Scroll take a specific coordinate > > I think the second is the most clear to the reader. Done.
thanks, lgtm https://codereview.chromium.org/2773893003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2773893003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:590: " .fixed { position: fixed; }" nit: no longer needed
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2773893003/diff/300001/cc/input/scroller_size... File cc/input/scroller_size_metrics.h (right): https://codereview.chromium.org/2773893003/diff/300001/cc/input/scroller_size... cc/input/scroller_size_metrics.h:16: 200000; // Approximately 400px * 500px, 250px * 800px, etc. nit: You can remove this comment since the one above explains the size. https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:213: // fisrt scrollable area that we find during the walk up. nit: s/fisrt/first https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:247: if (scroller_size > 0) { Nit: While clearly a silly case, you might miss a 0 area scroller? i.e. maybe the scroll chains up to something 0x0? You could initialize scroller_size to -1 as a clearly invalid area. https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.h:114: void ComputeScrollRelatedMetrics( Document that scroller_size is set only if there is a non root scroller?
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Rob. https://codereview.chromium.org/2773893003/diff/300001/cc/input/scroller_size... File cc/input/scroller_size_metrics.h (right): https://codereview.chromium.org/2773893003/diff/300001/cc/input/scroller_size... cc/input/scroller_size_metrics.h:16: 200000; // Approximately 400px * 500px, 250px * 800px, etc. On 2017/04/21 00:56:35, flackr wrote: > nit: You can remove this comment since the one above explains the size. Done. https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.cpp (right): https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:213: // fisrt scrollable area that we find during the walk up. On 2017/04/21 00:56:35, flackr wrote: > nit: s/fisrt/first Done. https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.cpp:247: if (scroller_size > 0) { On 2017/04/21 00:56:35, flackr wrote: > Nit: While clearly a silly case, you might miss a 0 area scroller? i.e. maybe > the scroll chains up to something 0x0? You could initialize scroller_size to -1 > as a clearly invalid area. Done. https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/ScrollManager.h (right): https://codereview.chromium.org/2773893003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/ScrollManager.h:114: void ComputeScrollRelatedMetrics( On 2017/04/21 00:56:35, flackr wrote: > Document that scroller_size is set only if there is a non root scroller? Done.
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, jwd@chromium.org, weiliangc@chromium.org, enne@chromium.org, pdr@chromium.org, bokan@chromium.org, flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2773893003/#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": 1492777244115700,
"parent_rev": "bb89503baf314afded40d6517c8e6e18f1f89b6b", "commit_rev":
"43525ec7a4ebf72cc88ad3edfad53b5be3c59b61"}
Message was sent while issue was closed.
Description was changed from ========== Record size of scroller that user scrolls This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 200000 squared px. BUG=684631 TEST= EventHandlerTest.ScrollerSizeOfMainThreadScrollingHistogramRecordingTest; LayerTreeHostImplTest.ScrollerSizeOfCCScrollingHistogramRecordingTest; CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Record size of scroller that user scrolls This patch adds 2 UMA metrics that monitors the sizes of scrollers that user scrolls. One metric is for wheel scroll and the other is for touch scroll. Given that the purpose of this experiment is focused on small scrollers, we don't care about any scroller that is larger than 200000 squared px. BUG=684631 TEST= EventHandlerTest.ScrollerSizeOfMainThreadScrollingHistogramRecordingTest; LayerTreeHostImplTest.ScrollerSizeOfCCScrollingHistogramRecordingTest; CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2773893003 Cr-Commit-Position: refs/heads/master@{#466315} Committed: https://chromium.googlesource.com/chromium/src/+/43525ec7a4ebf72cc88ad3edfad5... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/43525ec7a4ebf72cc88ad3edfad5... |
