Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2311233002:
Warn about Web Audio without user gesture on Android no longer allowed on cross origin iframes. (Closed)
|
Created:
4 years, 3 months ago by mlamouri (slow - plz ping) Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, haraken, hongchan, Raymond Toy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWarn about Web Audio without user gesture on Android no longer allowed on cross origin iframes.
This is a CL to send to M54 to warn users of Web Audio in cross origin iframes.
Note that because of how Web Audio work this warning (and the use counter) are
larger than reality: every `new AudioContext` in a cross origin iframe will warn
if not inside an event handler even if they are not meant to be used yet.
Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-VAAAJ
BUG=617090
Committed: https://crrev.com/8e94d8acbaf7ffca80e06429422fe8cbd311f781
Cr-Commit-Position: refs/heads/master@{#418201}
Patch Set 1 #
Total comments: 5
Patch Set 2 : apply foolip comment #
Dependent Patchsets: Messages
Total messages: 33 (17 generated)
mlamouri@chromium.org changed reviewers: + rtoy@chromium.org
rtoy@, PTAL. It's the CL to start warning the users WRT to the intervention.
Description was changed from ========== Deprecation: Web Audio without user gesture on Android no longer allowed on cross origin iframes. BUG=617090 ========== to ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. TODO: add link to intent to intervene. BUG=617090 ==========
Description was changed from ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. TODO: add link to intent to intervene. BUG=617090 ========== to ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. TODO: add link to intent to intervene. BUG=617090 ==========
https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:558: DCHECK(m_userGestureRequired); Don't quite understand why m_userGestureRequired has to be true here. In line 550, m_userGestureRequired is tested (for false), but the body doesn't ever seem to set it to anything and doesn't always return, so m_userGestureRequired could be false at line 558. What am I missing?
https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:558: DCHECK(m_userGestureRequired); On 2016/09/06 at 15:32:32, Raymond Toy wrote: > Don't quite understand why m_userGestureRequired has to be true here. In line 550, m_userGestureRequired is tested (for false), but the body doesn't ever seem to set it to anything and doesn't always return, so m_userGestureRequired could be false at line 558. > > What am I missing? It always return :) I added the DCHECK() because I was a bit surprised by this code (that I wrote myself).
https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:558: DCHECK(m_userGestureRequired); On 2016/09/06 15:43:00, mlamouri (slow) wrote: > On 2016/09/06 at 15:32:32, Raymond Toy wrote: > > Don't quite understand why m_userGestureRequired has to be true here. In line > 550, m_userGestureRequired is tested (for false), but the body doesn't ever seem > to set it to anything and doesn't always return, so m_userGestureRequired could > be false at line 558. > > > > What am I missing? > > It always return :) > > I added the DCHECK() because I was a bit surprised by this code (that I wrote > myself). Oh, duh! I read the return as inside the else. lgtm for the webaudio parts.
mlamouri@chromium.org changed reviewers: + foolip@chromium.org, isherman@chromium.org
+isherman@ for tools/metrics/ +foolip@ for Blink.
Description was changed from ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. TODO: add link to intent to intervene. BUG=617090 ========== to ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-V... BUG=617090 ==========
The CQ bit was checked by mlamouri@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
histograms.xml lgtm
Description was changed from ========== Intervention: Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-V... BUG=617090 ========== to ========== Warn about Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-V... BUG=617090 ==========
Deprecating and committing to removal without knowing the usage is a bit scary, do we have other things that bound the usage of this so that we can be almost certain it'll work out? https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:385: return willBeRemoved("Web Audio autoplay (without user gesture) from cross origin iframes", 55, "6406908126691328"); cross-origin with a hyphen wins a grep fight and is how the specs I checked seem to spell it.
On 2016/09/08 at 12:58:53, foolip wrote: > Deprecating and committing to removal without knowing the usage is a bit scary, do we have other things that bound the usage of this so that we can be almost certain it'll work out? We have metric of AudioContext creation in cross origin iframes on Android. It is low. We don't know how many of them are actually used without a user gesture exactly. We can extrapolate but in general, it's hard to tell because we would trigger "no user gesture" if `new AudioContext` is created without a user gesture. Basically, I'm adding this UseCounter to keep better track of things but I doubt it will be super useful. I'm fine with having a warning without counter.
On 2016/09/08 13:04:22, mlamouri wrote: > On 2016/09/08 at 12:58:53, foolip wrote: > > Deprecating and committing to removal without knowing the usage is a bit > scary, do we have other things that bound the usage of this so that we can be > almost certain it'll work out? > > We have metric of AudioContext creation in cross origin iframes on Android. It > is low. We don't know how many of them are actually used without a user gesture > exactly. We can extrapolate but in general, it's hard to tell because we would > trigger "no user gesture" if `new AudioContext` is created without a user > gesture. > > Basically, I'm adding this UseCounter to keep better track of things but I doubt > it will be super useful. I'm fine with having a warning without counter. Is that another use counter, or a histogram? Can't find it.
On 2016/09/08 at 13:09:04, foolip wrote: > On 2016/09/08 13:04:22, mlamouri wrote: > > On 2016/09/08 at 12:58:53, foolip wrote: > > > Deprecating and committing to removal without knowing the usage is a bit > > scary, do we have other things that bound the usage of this so that we can be > > almost certain it'll work out? > > > > We have metric of AudioContext creation in cross origin iframes on Android. It > > is low. We don't know how many of them are actually used without a user gesture > > exactly. We can extrapolate but in general, it's hard to tell because we would > > trigger "no user gesture" if `new AudioContext` is created without a user > > gesture. > > > > Basically, I'm adding this UseCounter to keep better track of things but I doubt > > it will be super useful. I'm fine with having a warning without counter. > > Is that another use counter, or a histogram? Can't find it. https://www.chromestatus.com/metrics/feature/popularity#AudioContextCrossOrig...
On 2016/09/08 at 13:13:49, mlamouri wrote: > On 2016/09/08 at 13:09:04, foolip wrote: > > On 2016/09/08 13:04:22, mlamouri wrote: > > > On 2016/09/08 at 12:58:53, foolip wrote: > > > > Deprecating and committing to removal without knowing the usage is a bit > > > scary, do we have other things that bound the usage of this so that we can be > > > almost certain it'll work out? > > > > > > We have metric of AudioContext creation in cross origin iframes on Android. It > > > is low. We don't know how many of them are actually used without a user gesture > > > exactly. We can extrapolate but in general, it's hard to tell because we would > > > trigger "no user gesture" if `new AudioContext` is created without a user > > > gesture. > > > > > > Basically, I'm adding this UseCounter to keep better track of things but I doubt > > > it will be super useful. I'm fine with having a warning without counter. > > > > Is that another use counter, or a histogram? Can't find it. > > https://www.chromestatus.com/metrics/feature/popularity#AudioContextCrossOrig... Based on internal data, for Chrome Android beta, Web Audio in usage of cross origin iframes usage is for 0.23% of page load. A fraction of these will break with the intervention. What we don't know is how much.
foolip@, PTAL https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2311233002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:385: return willBeRemoved("Web Audio autoplay (without user gesture) from cross origin iframes", 55, "6406908126691328"); On 2016/09/08 at 12:58:53, foolip wrote: > cross-origin with a hyphen wins a grep fight and is how the specs I checked seem to spell it. Done.
The CQ bit was checked by mlamouri@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.
On 2016/09/08 13:26:32, mlamouri wrote: > On 2016/09/08 at 13:13:49, mlamouri wrote: > > On 2016/09/08 at 13:09:04, foolip wrote: > > > On 2016/09/08 13:04:22, mlamouri wrote: > > > > On 2016/09/08 at 12:58:53, foolip wrote: > > > > > Deprecating and committing to removal without knowing the usage is a bit > > > > scary, do we have other things that bound the usage of this so that we can > be > > > > almost certain it'll work out? > > > > > > > > We have metric of AudioContext creation in cross origin iframes on > Android. It > > > > is low. We don't know how many of them are actually used without a user > gesture > > > > exactly. We can extrapolate but in general, it's hard to tell because we > would > > > > trigger "no user gesture" if `new AudioContext` is created without a user > > > > gesture. > > > > > > > > Basically, I'm adding this UseCounter to keep better track of things but I > doubt > > > > it will be super useful. I'm fine with having a warning without counter. > > > > > > Is that another use counter, or a histogram? Can't find it. > > > > > https://www.chromestatus.com/metrics/feature/popularity#AudioContextCrossOrig... > > Based on internal data, for Chrome Android beta, Web Audio in usage of cross > origin iframes usage is for 0.23% of page load. A fraction of these will break > with the intervention. What we don't know is how much. That's a rather big number, but it's been discussed on blink-dev now. LGTM.
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2311233002/#ps20001 (title: "apply foolip comment")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Warn about Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-V... BUG=617090 ========== to ========== Warn about Web Audio without user gesture on Android no longer allowed on cross origin iframes. This is a CL to send to M54 to warn users of Web Audio in cross origin iframes. Note that because of how Web Audio work this warning (and the use counter) are larger than reality: every `new AudioContext` in a cross origin iframe will warn if not inside an event handler even if they are not meant to be used yet. Intent to Intervene: https://groups.google.com/a/chromium.org/d/msg/blink-dev/51WbTwn0M_Y/VZuwn8-V... BUG=617090 Committed: https://crrev.com/8e94d8acbaf7ffca80e06429422fe8cbd311f781 Cr-Commit-Position: refs/heads/master@{#418201} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8e94d8acbaf7ffca80e06429422fe8cbd311f781 Cr-Commit-Position: refs/heads/master@{#418201} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
