Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082463002:
Measure the usage of navigator.vibrate in for user gesture & subframe. (Closed)
|
Created:
4 years, 6 months ago by Bin Lu Modified:
4 years, 5 months ago Reviewers:
rkaplow, mlamouri (slow - plz ping), kenjibaheux, Michael van Ouwerkerk, Nate Chapin, ojan CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, haraken, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMeasure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe.
BUG=621397
R=ojan@chromium.org,japhet@chromium.org
Committed: https://crrev.com/318a9ac4bee905bae95b33a67f75a3e4a5214116
Cr-Commit-Position: refs/heads/master@{#402601}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Record more detailed usage of vibrate for user gesture, subframe and cross-origin subframe. Also removing the old metrics. #
Total comments: 6
Patch Set 3 : Fix issues mentioned in comments. #
Total comments: 1
Patch Set 4 : Keep depcreated UserCounter metrics and suffix the labels with (obsolete). #
Total comments: 3
Patch Set 5 : Fix the issues raised in new comments. #Patch Set 6 : Change the new function to static to fix compile failure. #Patch Set 7 : Add back the NavigatorVibrate UserCounter in case we'd like to know the percentage of vibrate being… #Patch Set 8 : Add back NavigatorVibrateSubFrame UserCounter since we decide to go ahead to remove vibrate from if… #
Messages
Total messages: 46 (19 generated)
Hi Ojan and Nate, Will this work? Thanks!
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: } Would that make sense to integrate with the NavigatorVibrate and NavigatorVibrateSubFrame above? FWIW, NavigatorVibrateSubFrame already has a very low value. The issue here is that vibrate is abused a lot on mobile with popups (so they are top frames).
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:86: if (frame->isCrossOrigin()) { Recording whether a user gesture is present would also be useful if it's not cross origin. https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:88: crossOriginVibrateHistogram.count(UserGestureIndicator::processingUserGesture() ? 1 : 0); To make the data consistent with the use counters above, this should all go before the early return for page visibility.
ojan@chromium.org changed reviewers: + kenjibaheux@chromium.org
Kenji, can you help Bin: 1. Propose this at the WICG and get some draft spec text together. 2. Write up an intent to ship? I think we can close this code review given that the existing user counters have enough data to move forward with. https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: } On 2016/06/20 at 10:39:20, Mounir Lamouri (very slow) wrote: > Would that make sense to integrate with the NavigatorVibrate and NavigatorVibrateSubFrame above? FWIW, NavigatorVibrateSubFrame already has a very low value. I didn't realize we had NavigatorVibrateSubFrame already. Looking at our internal numbers, ~0.16% of android page views use NavigatorVibrate and ~0.0064% hit NavigatorVibrateSubFrame. I suspect a large percentage of the latter are ads that are user annoyance. IMO, we don't need more numbers. We should put together an intent to implement and ship to disable vibrate inside cross origin frames. The intent should clarify that we'll allow it again in the future once we figure out the shape of the permissions API. > The issue here is that vibrate is abused a lot on mobile with popups (so they are top frames). Ugh. That's something we'll need to solve in a different way I think. I think changing cross-origin frames to disallow vibrate by default is still a valuable change though, yeah?
(someone should implement an autosave mechanism in the rietvield UI, losing comments is painful :() Second attempt. As discussed with Ojan offline, the API is mostly abused in the top frame: malicious ads will do click jacking and open a popup that will vibrate. Anecdotally, I haven't seen this API abused on iframes but seen it multiple times on top frames. The low usage on iframes might confirm this. A corollary to this is that blocking the API on iframes is of low risk but might be of low benefit. We might want to reach out to other UAs given that there is no rush. Probably via opening an intervention issue and ping some folks? The main issue here is that top frames without user gestures are the normal use case and also the abused use cases so if we want to block abuses we might need to implement more complicated heuristics which will be less predictable.
Thanks all for the discussion. We did notice that malicious ads creative occasionally call navigator.vibrate to scare/trick users, e.g, 500+ ad creatives served via Google were calling it in Safe Browsing scans in the past 15 days. Examples: http://ictis/wdjxd8ix8n, http://ictis/9mhbmqbd4j. PLX Query: https://plx.corp.google.com/script/#a=cp%7Ci=google%253A%253Ascript_c5._93c01.... This is just for ads served via Google, and I guess it's used more often by ads served via 3rd-party ad networks. So if those ads are rendered in cross-origin iframes and we block them, it's still a big win for safer ads. > We might want to reach out to other UAs given that there is > no rush. Probably via opening an intervention issue and ping some folks? Sure, sounds a good idea to me. I'll work with Ojan and Kenji on it. > The main issue here is that top frames without user gestures are the normal use > case and also the abused use cases so if we want to block abuses we might need > to implement more complicated heuristics which will be less predictable. How about I modify this CL to record also the numbers of vibrating w/o user gesture in top frames? So we can have a better understanding of the issue. Btw: what do you mean by "integrate with the NavigatorVibrate and NavigatorVibrateSubFrame above"? Thanks, Bin
You could measure vibrations w/o user gesture but I don't think it will help much because normal use cases will also be w/o user gestures. Otherwise, by integrate, I meant by doing this in the same block instead of having two different places with metrics collection.
Description was changed from ========== Measure the usage of navigator.vibrate in cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org ========== to ========== Measure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org ==========
binlu@google.com changed reviewers: + rkaplow@chromium.org
Made changes to record more detailed usage of vibrate for user gesture, same-origin subframe and cross-origin subframe. Also removing the old metrics. +rkaplow@ for the owner review of histograms.xml. PTAL. Thanks!
It looks good in general. Couple of minor comments below. https://codereview.chromium.org/2082463002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: NavigatorVibrateHistogram.count(vibrateParams); We usually use proper enum to guarantee enum/histogram matching. https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62351: +<histogram name="WebModules.NavigatorVibrate" I wouldn't use a WebModules prefix but instead maybe something like "Vibration.Context". Though, I'm no vibration nor metrics owner :) https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82306: +<enum name="NavigatorVibratePermissions" type="int"> Not sure if "permissions" is the right term.
Fixed the issues. PTAL. Thanks. https://codereview.chromium.org/2082463002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: NavigatorVibrateHistogram.count(vibrateParams); On 2016/06/27 12:34:02, Mounir Lamouri (slow) wrote: > We usually use proper enum to guarantee enum/histogram matching. Done. Added enum. https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62351: +<histogram name="WebModules.NavigatorVibrate" On 2016/06/27 12:34:03, Mounir Lamouri (slow) wrote: > I wouldn't use a WebModules prefix but instead maybe something like > "Vibration.Context". Though, I'm no vibration nor metrics owner :) Done. https://codereview.chromium.org/2082463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82306: +<enum name="NavigatorVibratePermissions" type="int"> On 2016/06/27 12:34:02, Mounir Lamouri (slow) wrote: > Not sure if "permissions" is the right term. Done. Changed to "NavigatorVibrationType".
lgtm https://codereview.chromium.org/2082463002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2082463002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74788: - <int value="850" label="NavigatorVibrate"/> can you leave, and suffix with (obsolete)?
Done. Thanks. On 2016/06/27 18:00:39, rkaplow wrote: > lgtm > > https://codereview.chromium.org/2082463002/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (left): > > https://codereview.chromium.org/2082463002/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:74788: - <int value="850" > label="NavigatorVibrate"/> > can you leave, and suffix with (obsolete)?
The CQ bit was checked by binlu@google.com to run a CQ dry run
The CQ bit was unchecked by binlu@google.com
https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:24: #include "core/frame/UseCounter.h" Delete this as it is no longer used. https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:76: const unsigned userGestureBit = 0x1; I don't find this section very readable, using bit manipulation. Why not have some conditionals that set the actual enum value directly? You could get rid of the static_cast as well that way. https://codereview.chromium.org/2082463002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2082463002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82309: + <int value="1" label="Main frame vibrate, user gesture"/> nit: "with user gesture" here and below, to contrast with "no user gesture"
Done. PTAL. Thanks. On 2016/06/28 11:45:53, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right): > > https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:24: #include > "core/frame/UseCounter.h" > Delete this as it is no longer used. > > https://codereview.chromium.org/2082463002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:76: const > unsigned userGestureBit = 0x1; > I don't find this section very readable, using bit manipulation. Why not have > some conditionals that set the actual enum value directly? You could get rid of > the static_cast as well that way. > > https://codereview.chromium.org/2082463002/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2082463002/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:82309: + <int value="1" label="Main > frame vibrate, user gesture"/> > nit: "with user gesture" here and below, to contrast with "no user gesture"
lgtm thanks
The CQ bit was checked by binlu@google.com 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by binlu@google.com 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 binlu@google.com
The CQ bit was checked by binlu@google.com to run a CQ dry run
Added back the NavigatorVibrate UserCounter in case we'd like to know the percentage of vibrate being called on page views. Please let me know if there is any problem. Thanks.
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 binlu@google.com 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...
Also added back NavigatorVibrateSubFrame UserCounter since we decide to go ahead to remove vibrate from iframes. Internal discussion: https://groups.google.com/a/google.com/forum/#!topic/chrome-intervention-team.... On 2016/06/28 17:57:59, Bin Lu wrote: > Added back the NavigatorVibrate UserCounter in case we'd like to know the > percentage of vibrate being called on page views. > Please let me know if there is any problem. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by binlu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2082463002/#ps140001 (title: "Add back NavigatorVibrateSubFrame UserCounter since we decide to go ahead to remove vibrate from if…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Measure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org ========== to ========== Measure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Measure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org ========== to ========== Measure detailed usage of navigator.vibrate for user gesture, subframe & cross-origin iframe. BUG=621397 R=ojan@chromium.org,japhet@chromium.org Committed: https://crrev.com/318a9ac4bee905bae95b33a67f75a3e4a5214116 Cr-Commit-Position: refs/heads/master@{#402601} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/318a9ac4bee905bae95b33a67f75a3e4a5214116 Cr-Commit-Position: refs/heads/master@{#402601} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
