Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524443002:
Adding Rappor metrics for disabling cross-origin autoplay with sound experiment (Closed)
|
Created:
4 years, 1 month ago by Zhiqiang Zhang (Slow) Modified:
4 years ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama, vcarbune.chromium, Mark P Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding Rappor metrics for disabling cross-origin autoplay with sound experiment
This CL adds Rappor metrics to collect data for the cross-origin
autoplay experiment. It records the ETLD+1 of top-level frame and
iframes when a video is allowed to/blocked to autoplay and also
when the user has manually started a blocked video.
BUG=672526
Committed: https://crrev.com/d054c278da7a25db1c4646bd7e61dcb5f051bceb
Cr-Commit-Position: refs/heads/master@{#437682}
Patch Set 1 #Patch Set 2 : new implementation #
Total comments: 22
Patch Set 3 : addressed part of Anton's comments #Patch Set 4 : rebased #Patch Set 5 : s/CrossOriginExperiment/CrossOrigin #
Total comments: 8
Patch Set 6 : rebased #Patch Set 7 : addressed nits #Patch Set 8 : rebased #
Messages
Total messages: 25 (11 generated)
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
This CL has lower priority (since it depends on the previous CL). You can take a look if you have spare time.
On 2016/11/25 16:37:42, Zhiqiang Zhang wrote: > This CL has lower priority (since it depends on the previous CL). You can take a > look if you have spare time. I talked with Mounir and we decided to land the previous CL first and work on several solutions to the issue. This CL is ready to be reviewed now :)
Description was changed from ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment BUG=<WIP> ========== to ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=665456 ==========
https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:100: if (!m_element->isHTMLVideoElement()) nit: Should we avoid calling this for non-video elements and turn this into a DCHECK? https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:102: if (!m_element->isCrossOrigin()) nit: ditto https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:104: if (m_recordedCrossOriginExperimentMetrics.count(metric)) nit: add a comment why we only record each result once (per element I assume)? https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:126: if (!m_recordedCrossOriginExperimentMetrics.count( nit: a comment would help why this check is here. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:141: m_recordedCrossOriginExperimentMetrics.insert(metric); nit: put this next to the check if the metric is already recorded since these two are coupled logically? https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:41: enum class AutoplayCrossOriginExperimentMetric { nit: I think we could avoid having experiment and metric in the metric enum names First, we may want to upgrade the code to stable if the experiment is successful (and keep the metrics) - you probably don't want to rename everything then. Second, Metric is not descriptive for what is being measured. Maybe something like CrossOriginAutoplayResult? Similar comment applies to other names in this and the dependent patch. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:534: m_lockedPendingUserGestureIfCrossOriginExperimentEnabled = true; should you make it false in the case the new document is not cross-origin? what about cross-origin to cross-origin move where the element was unlocked in the old document? could we just do m_lockedPendingUserGesture = computeLockedPendingUserGesture(document()) and check if the document is cross-origin? https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2282: m_autoplayUmaHelper->recordCrossOriginExperimentMetric( seems like we'll record some metrics when the experiment is disabled, is that intended? https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3968: bool HTMLMediaElement:: how is this different from the isGestureNeededForPlayback from above? add a check for MediaStream that you probably want and autoplay helper (which is being removed) and it's identicall, no? I thought you would implement it as something like return isGestureNeededForPlayback() && isCrossOrigin() && isCrossOriginExperimentEnabled(); Or rather merge it to isGestureNeededForPlayback as suggested above. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:293: bool isCrossOrigin() const; nit: add frame to the name? it could be cross-origin if the src url is cross-origin too. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:524: bool isLockedPendingUserGestureIfCrossOriginExperimentEnabled() const; can't this be part of the method above instead? it's meant to be a universal method. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:537: bool isGestureNeededForPlaybackIfCrossOriginExperimentEnabled() const; ditto
PTAL https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:100: if (!m_element->isHTMLVideoElement()) On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: Should we avoid calling this for non-video elements and turn this into a > DCHECK? I think that would introduce a lot of duplicate if statements, so I prefer not. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:104: if (m_recordedCrossOriginExperimentMetrics.count(metric)) On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: add a comment why we only record each result once (per element I assume)? Done. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:126: if (!m_recordedCrossOriginExperimentMetrics.count( On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: a comment would help why this check is here. Done. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:141: m_recordedCrossOriginExperimentMetrics.insert(metric); On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: put this next to the check if the metric is already recorded since these > two are coupled logically? Done. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:41: enum class AutoplayCrossOriginExperimentMetric { On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: I think we could avoid having experiment and metric in the metric enum > names > First, we may want to upgrade the code to stable if the experiment is successful > (and keep the metrics) - you probably don't want to rename everything then. > Second, Metric is not descriptive for what is being measured. > > Maybe something like CrossOriginAutoplayResult? > Similar comment applies to other names in this and the dependent patch. Done. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:534: m_lockedPendingUserGestureIfCrossOriginExperimentEnabled = true; On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > should you make it false in the case the new document is not cross-origin? > what about cross-origin to cross-origin move where the element was unlocked in > the old document? > > could we just do m_lockedPendingUserGesture = > computeLockedPendingUserGesture(document()) and check if the document is > cross-origin? Please see the other reply. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2282: m_autoplayUmaHelper->recordCrossOriginExperimentMetric( On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > seems like we'll record some metrics when the experiment is disabled, is that > intended? Yes :) Since the experiment might break many sites if it's enabled, we are recording metrics "pretending" the experiment is enabled. This also answers why I added "isLockedPendingUserGestureIfCrossOriginExperimentEnabled()" by side of "isLockedPendingUserGesture()". Ditto for some other of your comments. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3968: bool HTMLMediaElement:: On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > how is this different from the isGestureNeededForPlayback from above? > add a check for MediaStream that you probably want and autoplay helper (which is > being removed) and it's identicall, no? Done. Added the missing checks. > I thought you would implement it as something like > > return isGestureNeededForPlayback() && isCrossOrigin() && > isCrossOriginExperimentEnabled(); > > Or rather merge it to isGestureNeededForPlayback as suggested above. The goal is to record metrics pretending that the flag is enabled even if it's not. The best solution I can come up with is the current solution. https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:293: bool isCrossOrigin() const; On 2016/11/29 21:42:04, whywhat_OOO_till_Mon_Nov_28 wrote: > nit: add frame to the name? it could be cross-origin if the src url is > cross-origin too. s/isCrossOrigin/isInCrossOriginFrame https://codereview.chromium.org/2524443002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:524: bool isLockedPendingUserGestureIfCrossOriginExperimentEnabled() const; On 2016/11/29 21:42:05, whywhat_OOO_till_Mon_Nov_28 wrote: > can't this be part of the method above instead? it's meant to be a universal > method. Please see the other reply.
lgtm I still think it would be nice to get rid of duplicated long named booleans and methods but we can change it later if needed.
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org, mpearson@chromium.org
+mlamouri: RS HTMLMediaElement.* +mpearson: rappor.xml
Description was changed from ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=665456 ========== to ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=672526 ==========
lgtm with comments applied! :) https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:140: "Media.Autoplay.CrossOrigin.PlayedWithGesture.ChildFrame", Should the rapport name be "PlayedWithGestureAfterBlock" then? https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3853: return false; What's that for? https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1008: <rappor-metric name="Media.Autoplay.CrossOrigin.Allowed.ChildFrame" s/Allowed/Blocked/ https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1019: <rappor-metric name="Media.Autoplay.CrossOrigin.Allowed.ChildFrame" s/Allowed/PlayedWithGesture/ ... same for the TopLevelFrame ones below
https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:140: "Media.Autoplay.CrossOrigin.PlayedWithGesture.ChildFrame", On 2016/12/08 17:17:06, mlamouri wrote: > Should the rapport name be "PlayedWithGestureAfterBlock" then? Done. https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2524443002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3853: return false; On 2016/12/08 17:17:06, mlamouri wrote: > What's that for? This mostly copied from isGestureNeededForPlayback(). We should avoid this duplicate code. I made the common part a separate helper function. https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1008: <rappor-metric name="Media.Autoplay.CrossOrigin.Allowed.ChildFrame" On 2016/12/08 17:17:06, mlamouri wrote: > s/Allowed/Blocked/ Done. https://codereview.chromium.org/2524443002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1019: <rappor-metric name="Media.Autoplay.CrossOrigin.Allowed.ChildFrame" On 2016/12/08 17:17:06, mlamouri wrote: > s/Allowed/PlayedWithGesture/ > > ... same for the TopLevelFrame ones below Done.
On 2016/12/08 14:04:24, Zhiqiang Zhang wrote: > +mlamouri: RS HTMLMediaElement.* > +mpearson: rappor.xml I do not do rappor.xml reviews. I'll nominate someone else. --mark
mpearson@chromium.org changed reviewers: + holte@chromium.org - mpearson@chromium.org
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
+holte for rappor.xml
lgtm
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mlamouri@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2524443002/#ps130001 (title: "rebased")
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": 130001, "attempt_start_ts": 1481318401744090,
"parent_rev": "177026b5b7563bc7433d63429489be4fb5abc250", "commit_rev":
"e05a1d85f3027e2e4b84c8f91770bf7e2935f697"}
Message was sent while issue was closed.
Description was changed from ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=672526 ========== to ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=672526 Review-Url: https://codereview.chromium.org/2524443002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=672526 Review-Url: https://codereview.chromium.org/2524443002 ========== to ========== Adding Rappor metrics for disabling cross-origin autoplay with sound experiment This CL adds Rappor metrics to collect data for the cross-origin autoplay experiment. It records the ETLD+1 of top-level frame and iframes when a video is allowed to/blocked to autoplay and also when the user has manually started a blocked video. BUG=672526 Committed: https://crrev.com/d054c278da7a25db1c4646bd7e61dcb5f051bceb Cr-Commit-Position: refs/heads/master@{#437682} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d054c278da7a25db1c4646bd7e61dcb5f051bceb Cr-Commit-Position: refs/heads/master@{#437682} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
