Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031393003:
WebAudio: record whether user gesture was available and provided. (Closed)
|
Created:
4 years, 6 months ago by mlamouri (slow - plz ping) Modified:
4 years, 6 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, haraken, hongchan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebAudio: record whether user gesture was available and provided.
We are looking into a more consistent audio playback on mobile and want
to see whether implementing Safari iOS behaviour in Chrome Android would
break websites.
BUG=616807
Committed: https://crrev.com/9eeee90e6f8d52fce0b3645f933796abc13fe891
Cr-Commit-Position: refs/heads/master@{#398061}
Patch Set 1 #
Total comments: 20
Patch Set 2 : review comments #
Total comments: 6
Messages
Total messages: 21 (6 generated)
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/patch-status/2031393003/1
mlamouri@chromium.org changed reviewers: + isherman@chromium.org, rtoy@chromium.org
isherman@, PTAL at histograms usage. rtoy@, PTAL at webaudio/ changes.
https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:118: m_userGestureRequired = true; You initialized m_userGestureRequired to true in line 184. Did you mean false there? Otherwise these lines do nothing. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:127: : AbstractAudioContext(document) Why the change? It seems that it nothing to do with this CL, so I'd prefer if this change were not made. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:486: void AbstractAudioContext::recordUserGesture() I think recordUserGestureState() is a better name. When I read the current name, I think you are recording a gesture from the user. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:749: recordUserGesture(); Can you explain why this is here? startRendering() is called basically when the context is created. And you very probably don't want to call this for an offline context since no audio is ever played out in that case. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:444: context()->recordUserGesture(); Not sure if this is the right place. Say you've created a context and set up a bunch of sources. Then you schedule the source to start 1 minute from now. You'll get the user gesture recorded, but the sound wouldn't have started playing for another minute. Don't you want to know that when the sound actually starts playing that you didn't have the user gesture permission to play? https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:119: recordUserGesture(); I think you probably want to do this around line 128, after the if statement about the closed context. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:135: context()->recordUserGesture(); Same comment as for AudioBufferSource.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
histograms lgtm % nits: https://codereview.chromium.org/2031393003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2031393003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60315: + Records whether WebAudio had a user gesture requirements and whether they nit: s/requirements/requirement/ and s/they were/it was/ (or some other such grammatical change) https://codereview.chromium.org/2031393003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:90254: +<enum name="UserGestureRequirements" type="int"> nit: s/Requirements/Requirement
Comments applied. PTAL. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:118: m_userGestureRequired = true; On 2016/06/03 at 17:57:52, Raymond Toy wrote: > You initialized m_userGestureRequired to true in line 184. Did you mean false there? Otherwise these lines do nothing. I meant `false` above. I changed before uploading to test that things were working. The issue of sending a CL on Friday evening before leaving the office :) https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:127: : AbstractAudioContext(document) On 2016/06/03 at 17:57:52, Raymond Toy wrote: > Why the change? It seems that it nothing to do with this CL, so I'd prefer if this change were not made. Instead of adding yet another init in there, I think we should re-use the ctor above. Though, put the stuff back as you asked. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:486: void AbstractAudioContext::recordUserGesture() On 2016/06/03 at 17:57:52, Raymond Toy wrote: > I think recordUserGestureState() is a better name. When I read the current name, I think you are recording a gesture from the user. Done. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:749: recordUserGesture(); On 2016/06/03 at 17:57:52, Raymond Toy wrote: > Can you explain why this is here? startRendering() is called basically when the context is created. > > And you very probably don't want to call this for an offline context since no audio is ever played out in that case. This is basically added to places where WebKit does check for user gesture. The intent here being that we want to make sure that having an implementation following WebKit would not break websites (because of UA sniffing). WebKit will not allow an AudioContext to start unless it was "unlocked". I think the main reason is for ScriptProcessor but it might be for other reasons. Regarding offline contexts, I don't think WebKit does anything specific (based on the code). Would you have a test page for offline audio context? https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:444: context()->recordUserGesture(); On 2016/06/03 at 17:57:52, Raymond Toy wrote: > Not sure if this is the right place. Say you've created a context and set up a bunch of sources. Then you schedule the source to start 1 minute from now. You'll get the user gesture recorded, but the sound wouldn't have started playing for another minute. > > Don't you want to know that when the sound actually starts playing that you didn't have the user gesture permission to play? As above, I'm following WebKit's behaviour here. The rationale is that if start() is called with a gesture, it will unlock the AudioContext which will then be able to start running when startPlaying() is called. Otherwise, if `start(1000)` is called with a gesture, when the playback actually starts, there will be no gesture on the stack. Does that make sense? https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:119: recordUserGesture(); On 2016/06/03 at 17:57:52, Raymond Toy wrote: > I think you probably want to do this around line 128, after the if statement about the closed context. Done. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:135: context()->recordUserGesture(); On 2016/06/03 at 17:57:52, Raymond Toy wrote: > Same comment as for AudioBufferSource. See above. https://codereview.chromium.org/2031393003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2031393003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60315: + Records whether WebAudio had a user gesture requirements and whether they On 2016/06/03 at 21:28:06, Ilya Sherman wrote: > nit: s/requirements/requirement/ and s/they were/it was/ (or some other such grammatical change) Done.
Mostly just a few nits. I guess you want to make webaudio behave like it does on iOS mobile Safari that requires a gesture. Does desktop Safari require a gesture to start webaudio? The one sample I tried didn't require any kind of gesture to start. Are you planning on adding this only for mobile? https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:127: : AbstractAudioContext(document) On 2016/06/05 14:44:57, Mounir Lamouri (slow) wrote: > On 2016/06/03 at 17:57:52, Raymond Toy wrote: > > Why the change? It seems that it nothing to do with this CL, so I'd prefer if > this change were not made. > > Instead of adding yet another init in there, I think we should re-use the ctor > above. Though, put the stuff back as you asked. Thanks. We'll do this fix some other day; it makes sense to re-use since there's nothing different. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:749: recordUserGesture(); On 2016/06/05 14:44:57, Mounir Lamouri (slow) wrote: > On 2016/06/03 at 17:57:52, Raymond Toy wrote: > > Can you explain why this is here? startRendering() is called basically when > the context is created. > > > > And you very probably don't want to call this for an offline context since no > audio is ever played out in that case. > > This is basically added to places where WebKit does check for user gesture. The > intent here being that we want to make sure that having an implementation > following WebKit would not break websites (because of UA sniffing). WebKit will > not allow an AudioContext to start unless it was "unlocked". I think the main > reason is for ScriptProcessor but it might be for other reasons. > Ok, it makes some sense to do the same. > Regarding offline contexts, I don't think WebKit does anything specific (based > on the code). Would you have a test page for offline audio context? What kind of test? Almost all of the webaudio layout tests in third_party/WebKit/LayoutTests/webaudio use offline contexts. Almost all can just be loaded up and run in a browser. https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:444: context()->recordUserGesture(); On 2016/06/05 14:44:57, Mounir Lamouri (slow) wrote: > On 2016/06/03 at 17:57:52, Raymond Toy wrote: > > Not sure if this is the right place. Say you've created a context and set up > a bunch of sources. Then you schedule the source to start 1 minute from now. > You'll get the user gesture recorded, but the sound wouldn't have started > playing for another minute. > > > > Don't you want to know that when the sound actually starts playing that you > didn't have the user gesture permission to play? > > As above, I'm following WebKit's behaviour here. The rationale is that if > start() is called with a gesture, it will unlock the AudioContext which will > then be able to start running when startPlaying() is called. Otherwise, if > `start(1000)` is called with a gesture, when the playback actually starts, there > will be no gesture on the stack. Does that make sense? Acknowledged. https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:146: m_userGestureRequired = true; I don't think this is needed or wanted for an offline context. (Yes, this method shouldn't really be here; it's only used to construct an offline audio context.) There is never any audio output for an offline context, so it's shouldn't require a gesture to run one. https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:444: context()->recordUserGestureState(); Probably want this after all of the code that could throw exceptions. https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:135: context()->recordUserGestureState(); Should put this after all of the code that could throw exceptions.
Quick reply to answer your main question: we want to measure what would happen if we were to apply the same restrictions as Safari iOS. The measurement will happen on all platforms but we will mostly care about Android. A follow-up will be to add a flag that will enable the behaviour that Safari iOS has. This will be aiming Chrome Android. Does that make sense? https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:146: m_userGestureRequired = true; On 2016/06/06 at 15:10:08, Raymond Toy wrote: > I don't think this is needed or wanted for an offline context. (Yes, this method shouldn't really be here; it's only used to construct an offline audio context.) > > There is never any audio output for an offline context, so it's shouldn't require a gesture to run one. My understanding is that Safari iOS doesn't make any difference between offline and non-offline. If we want to measure compat, we should probably keep the same checks?
On 2016/06/06 at 15:14:46, mlamouri wrote: > Quick reply to answer your main question: we want to measure what would happen if we were to apply the same restrictions as Safari iOS. The measurement will happen on all platforms but we will mostly care about Android. > > A follow-up will be to add a flag that will enable the behaviour that Safari iOS has. This will be aiming Chrome Android. > > Does that make sense? Yes, thanks for letting me know. > > https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): > > https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:146: m_userGestureRequired = true; > On 2016/06/06 at 15:10:08, Raymond Toy wrote: > > I don't think this is needed or wanted for an offline context. (Yes, this method shouldn't really be here; it's only used to construct an offline audio context.) > > > > There is never any audio output for an offline context, so it's shouldn't require a gesture to run one. > > My understanding is that Safari iOS doesn't make any difference between offline and non-offline. If we want to measure compat, we should probably keep the same checks? Yes, I agree if compatibility is the goal. I think that Safari is doing the wrong thing for offline contexts. There's no reason to block WebAudio on offline contexts; no audio is sent to any output.
lgtm with a few nits.
https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:444: context()->recordUserGestureState(); On 2016/06/06 at 15:10:08, Raymond Toy wrote: > Probably want this after all of the code that could throw exceptions. WebKit puts it exactly here actually :( https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/2031393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:135: context()->recordUserGestureState(); On 2016/06/06 at 15:10:08, Raymond Toy wrote: > Should put this after all of the code that could throw exceptions. ditto
The CQ bit was checked by mlamouri@chromium.org
Landing as rtoy@ agreed offline to land this with checks matching Safari iOS.
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2031393003/#ps20001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031393003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== WebAudio: record whether user gesture was available and provided. We are looking into a more consistent audio playback on mobile and want to see whether implementing Safari iOS behaviour in Chrome Android would break websites. BUG=616807 ========== to ========== WebAudio: record whether user gesture was available and provided. We are looking into a more consistent audio playback on mobile and want to see whether implementing Safari iOS behaviour in Chrome Android would break websites. BUG=616807 Committed: https://crrev.com/9eeee90e6f8d52fce0b3645f933796abc13fe891 Cr-Commit-Position: refs/heads/master@{#398061} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9eeee90e6f8d52fce0b3645f933796abc13fe891 Cr-Commit-Position: refs/heads/master@{#398061} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
