Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(330)

Issue 2137483003: Add UMA metrics for root scroller intervention to track forcing passive breakage. (Closed)

Created:
4 years, 5 months ago by dtapuska
Modified:
4 years, 5 months ago
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kinuko+serviceworker, krit, tzik, serviceworker-reviews, kouhei+svg_chromium.org, fs, nhiroki, falken, tommyw+watchlist_chromium.org, f(malita), asvitkine+watch_chromium.org, horo+watch_chromium.org, blink-reviews, gyuyoung2, Stephen Chennney, pdr+svgwatchlist_chromium.org, mcasas+watch+mediastream_chromium.org, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add UMA metrics for root scroller intervention to track forcing passive breakage. Calculate the number of event listener invocations that we didn't actually end up allowing preventDefault to actually execute. The denominator in this metric is the total number of invocations that we force passive on. BUG=625675 Committed: https://crrev.com/7a44b9eb2df493236df608d21681b563dbd74539 Cr-Commit-Position: refs/heads/master@{#406002}

Patch Set 1 #

Patch Set 2 : Add comment to clarify implementation point #

Total comments: 7

Patch Set 3 : Add ability to test histograms from layout tests #

Total comments: 9

Patch Set 4 : Address nits #

Total comments: 2

Patch Set 5 : Use HistogramTester #

Total comments: 1

Patch Set 6 : Change layout test to a unit test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+197 lines, -23 lines) Patch
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h View 1 2 3 1 chunk +35 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.cpp View 1 chunk +29 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/events/Event.h View 1 2 chunks +7 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/events/Event.cpp View 2 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/events/EventListenerMap.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/events/EventListenerMap.cpp View 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/events/EventTarget.h View 4 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/events/EventTarget.cpp View 1 2 3 8 chunks +24 lines, -5 lines 0 comments Download
A third_party/WebKit/Source/core/events/EventTargetTest.cpp View 1 2 3 4 5 1 chunk +50 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/events/RegisteredEventListener.h View 5 chunks +13 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGElement.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/mediastream/MediaStream.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/mediastream/MediaStream.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 2 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (8 generated)
dtapuska
PTAL; it dawned on me last night that we could do this and haven't done ...
4 years, 5 months ago (2016-07-08 17:04:36 UTC) #3
tdresser
LGTM. Fix description: demoninator -> denominator. https://codereview.chromium.org/2137483003/diff/20001/third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h File third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h (right): https://codereview.chromium.org/2137483003/diff/20001/third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h#newcode12 third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h:12: class CORE_EXPORT AddEventListenerOptionsResolved ...
4 years, 5 months ago (2016-07-08 17:37:11 UTC) #4
dtapuska
https://codereview.chromium.org/2137483003/diff/20001/third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h File third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h (right): https://codereview.chromium.org/2137483003/diff/20001/third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h#newcode12 third_party/WebKit/Source/core/events/AddEventListenerOptionsResolved.h:12: class CORE_EXPORT AddEventListenerOptionsResolved : public AddEventListenerOptions { On 2016/07/08 ...
4 years, 5 months ago (2016-07-08 19:03:54 UTC) #6
tdresser
Thanks for the test! I don't have a better idea for the name, but I ...
4 years, 5 months ago (2016-07-08 19:09:50 UTC) #7
Rick Byers
LGTM with nits https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html File third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html (right): https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html#newcode22 third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html:22: assert_equals(countBefore + 1, silentHistogramCount(0)); nit: rather ...
4 years, 5 months ago (2016-07-11 14:41:52 UTC) #8
dtapuska
isherman@ can you please pay special attention to Ricks comment on Histograms.cpp https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html File third_party/WebKit/LayoutTests/fast/events/event-target-passive-histograms.html ...
4 years, 5 months ago (2016-07-11 15:46:05 UTC) #10
Ilya Sherman
https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/Source/platform/Histogram.cpp File third_party/WebKit/Source/platform/Histogram.cpp (right): https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/Source/platform/Histogram.cpp#newcode38 third_party/WebKit/Source/platform/Histogram.cpp:38: bool EnumerationHistogram::GetHistogramCount(const char* name, base::HistogramBase::Sample bucket, int32_t* count) On ...
4 years, 5 months ago (2016-07-11 18:52:31 UTC) #11
dtapuska
On 2016/07/11 18:52:31, Ilya Sherman wrote: > https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/Source/platform/Histogram.cpp > File third_party/WebKit/Source/platform/Histogram.cpp (right): > > https://codereview.chromium.org/2137483003/diff/40001/third_party/WebKit/Source/platform/Histogram.cpp#newcode38 ...
4 years, 5 months ago (2016-07-12 19:14:43 UTC) #12
Rick Byers
On 2016/07/12 19:14:43, dtapuska wrote: > On 2016/07/11 18:52:31, Ilya Sherman wrote: > > > ...
4 years, 5 months ago (2016-07-12 21:20:54 UTC) #13
Ilya Sherman
Thanks! Good to know there's already a blink wrapper for the HistogramTester class =) https://codereview.chromium.org/2137483003/diff/80001/third_party/WebKit/Source/platform/testing/HistogramTester.h ...
4 years, 5 months ago (2016-07-12 21:26:18 UTC) #14
dtapuska
On 2016/07/12 21:26:18, Ilya Sherman wrote: > Thanks! Good to know there's already a blink ...
4 years, 5 months ago (2016-07-12 21:39:53 UTC) #15
Ilya Sherman
On 2016/07/12 21:39:53, dtapuska wrote: > On 2016/07/12 21:26:18, Ilya Sherman wrote: > > Thanks! ...
4 years, 5 months ago (2016-07-12 21:52:56 UTC) #16
dtapuska
On 2016/07/12 21:52:56, Ilya Sherman wrote: > On 2016/07/12 21:39:53, dtapuska wrote: > > On ...
4 years, 5 months ago (2016-07-12 21:56:22 UTC) #17
Rick Byers
On 2016/07/12 21:52:56, Ilya Sherman wrote: > On 2016/07/12 21:39:53, dtapuska wrote: > > On ...
4 years, 5 months ago (2016-07-12 21:59:23 UTC) #18
haraken
On 2016/07/12 21:59:23, Rick Byers wrote: > On 2016/07/12 21:52:56, Ilya Sherman wrote: > > ...
4 years, 5 months ago (2016-07-13 01:27:57 UTC) #19
dtapuska
On 2016/07/12 21:59:23, Rick Byers wrote: > On 2016/07/12 21:52:56, Ilya Sherman wrote: > > ...
4 years, 5 months ago (2016-07-13 13:21:05 UTC) #20
Rick Byers
Yeah this testing strategy seems better to me - thanks! LGTM
4 years, 5 months ago (2016-07-13 13:39:47 UTC) #21
Ilya Sherman
metrics lgtm, thanks
4 years, 5 months ago (2016-07-13 23:57:35 UTC) #22
lanwei
LGTM!
4 years, 5 months ago (2016-07-14 01:46:06 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2137483003/100001
4 years, 5 months ago (2016-07-18 14:20:29 UTC) #26
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 5 months ago (2016-07-18 15:47:37 UTC) #28
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-18 15:47:56 UTC) #29
commit-bot: I haz the power
4 years, 5 months ago (2016-07-18 15:50:44 UTC) #31
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/7a44b9eb2df493236df608d21681b563dbd74539
Cr-Commit-Position: refs/heads/master@{#406002}

Powered by Google App Engine
This is Rietveld 408576698