Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631113003:
Add metric to know how user use scrollbar (Closed)
DescriptionAdd metric to know how user use scrollbar
This patch is adding metric to know how user use scrollbar separately for
scrollbar button, clicking on the track, or dragging the thumb in different
scrollbar.
BUG=675593
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2631113003
Cr-Commit-Position: refs/heads/master@{#444550}
Committed: https://chromium.googlesource.com/chromium/src/+/f7734aba4cc737cdb271dd31867baad2efb83fbb
Patch Set 1 #
Total comments: 4
Patch Set 2 : bokan comments addressed #
Total comments: 1
Patch Set 3 : move to framehost #Patch Set 4 : move to UseCounter #
Total comments: 1
Patch Set 5 : move to PaintInvalidationCapableScrollableArea #Patch Set 6 : add null checker #Patch Set 7 : rebase #
Messages
Total messages: 46 (27 generated)
Description was changed from ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 ========== to ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right): https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:322: ScrollbarPressedPart scrollbarPressedPart = IgnorePart; Put all of this into a helper method that you call from here. https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:350: scrollbarPressedPartHistogram.count( This will still count multiple uses on a page load. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Vis... how I made sure that we only report the viewport stats once per page load by recording them at destruction time. (I'll confirm that we fixed the bug regarding UMA stats collected at destruction) https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right): https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:47: enum ScrollbarPressedPart { This should reflect the fact that it's used only for metrics. I'd name this ScrollbarUseUMA https://codereview.chromium.org/2631113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:51: VerticalScrollbarThumb, Nit: I think it be better to group all the horizontal and vertical ones together: VerticalScrollbarButton, VerticalScrollbarThumb, etc
Patchset #2 (id:20001) has been deleted
Updated, PTAL. Thank you.
https://codereview.chromium.org/2631113003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2631113003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:1109: std::bitset<ScrollbarUseUMAMax> m_sentScrollbarUseUMA; A page will have multiple FrameViews. Put this on FrameHost instead.
PTAL. Thank you.
Actually, sorry to make you redo work but I think this'll be more straight forward to use UseCounter.cpp/h If you just add each of your enums to the big UseCounter::Feature enum, you don't need to worry about keeping track of whether you've already reported a use or when to reset. Should be much simpler.
PTAL Thank you
https://codereview.chromium.org/2631113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/Widget.h (right): https://codereview.chromium.org/2631113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/Widget.h:130: virtual void sendScrollbarUseUMA(ScrollbarPart, ScrollbarOrientation) {} Rather than going through widget, I'd add a ScrollableArea::didScrollWithScrollbar and implement it in PaintInvalidationCapableScrollableArea which covers both PaintLayerScrollableArea and FrameView.
Description was changed from ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 ========== to ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Moved the logic to PaintInvalidationCapableScrollableArea. PTAL. Thank you.
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chaopeng@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, thanks.
chaopeng@chromium.org changed reviewers: + jbroman@chromium.org
jbroman@chromium.org: Please review changes. Thank you.
Description was changed from ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
bokan@chromium.org changed reviewers: + isherman@chromium.org
bokan@chromium.org changed reviewers: + asvitkine@chromium.org - isherman@chromium.org
bokan@chromium.org changed reviewers: + holte@chromium.org - asvitkine@chromium.org
You also need someone for histograms.xml. +holte@ for that
jbroman@chromium.org changed reviewers: + isherman@chromium.org - holte@chromium.org
lgtm
holte@chromium.org changed reviewers: + holte@chromium.org
histograms lgtm
The CQ bit was checked by chaopeng@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by chaopeng@chromium.org
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 chaopeng@chromium.org
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, jbroman@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2631113003/#ps140001 (title: "rebase")
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": 140001, "attempt_start_ts": 1484777317595110,
"parent_rev": "28a28f0874dce5c40e04b91bb224a242fee98830", "commit_rev":
"f7734aba4cc737cdb271dd31867baad2efb83fbb"}
Message was sent while issue was closed.
Description was changed from ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add metric to know how user use scrollbar This patch is adding metric to know how user use scrollbar separately for scrollbar button, clicking on the track, or dragging the thumb in different scrollbar. BUG=675593 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2631113003 Cr-Commit-Position: refs/heads/master@{#444550} Committed: https://chromium.googlesource.com/chromium/src/+/f7734aba4cc737cdb271dd31867b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f7734aba4cc737cdb271dd31867b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
