Web Audio: record autoplay status when an AudioContext is destroyed.

Web Audio: record autoplay status when an AudioContext is destroyed.

This is recording if an AudioContext with autoplay restrictions was
block, allowed to play and also record whether allowing start() to
activate the AudioContext would have allowed playback.

BUG=614115
TEST=BaseAudioContextTest.cpp (unit tests)

Committed: https://crrev.com/86a60cb0983a8bd9689d5c0ce7684f04799ada02
Cr-Commit-Position: refs/heads/master@{#425966}

[mlamouri (slow - plz ping), 2016-10-09 19:31:38]

mlamouri@chromium.org changed reviewers:
+ rtoy@chromium.org

[mlamouri (slow - plz ping), 2016-10-09 20:31:39]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 20:31:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-09 22:04:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-09 22:04:38]

Dry run: This issue passed the CQ dry run.

[Raymond Toy, 2016-10-10 17:05:10]

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:149:
recordAutoplayStatus();
I think if the user closes a tab, everything goes away, without necessarily
calling the destructor.  Is that ok for recording the status?

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:202: LOG(INFO)
<< "hasPendingActivity() => " << !m_isCleared;
Do you really want this log?  Just asking.

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:352: enum
AutoplayStatus {
"AutoPlay" or "Autoplay"? If you had written complete words, it would be
"AutoPlay", I think.

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:353:
AutoplayStatusFailed = 0,
This doesn't seem to quite match the histogram enum description.  What does this
really mean? Maybe add comments on what this really means?

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:355:
AutoplayStatusSucceeded = 3,
Why 3 and not 2?

Plus, this doesn't match the histogram values, which includes 2.

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:402: // Record the
current autoplay status and clear it.
It seems this is intended to be called only when the context is being
destructed. If so, add a comment here with this restriction.  

Or just get rid of this function and put the body into the destructor.

https://codereview.chromium.org/2404743002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2404743002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:101425: +  <int value="2" label="Failed
to autoplay but start() called on user gesture"/>
What is the intended difference between 1 and 2 and how does that match up with
the implementation?

[mlamouri (slow - plz ping), 2016-10-10 18:44:40]

mlamouri@chromium.org changed reviewers:
+ haraken@chromium.org

[mlamouri (slow - plz ping), 2016-10-10 18:44:40]

PTAL

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:149:
recordAutoplayStatus();
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> I think if the user closes a tab, everything goes away, without necessarily
calling the destructor.  Is that ok for recording the status?

I guess if the object is marked as GCFinalized, it should call the dtor. At
least according to the doc. I wonder if there are downsides with this or if
recording in contextDestroyed would work most of the time.

+haraken@ to give some guidance.

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:202: LOG(INFO)
<< "hasPendingActivity() => " << !m_isCleared;
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> Do you really want this log?  Just asking.

No :)

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:352: enum
AutoplayStatus {
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> "AutoPlay" or "Autoplay"? If you had written complete words, it would be
"AutoPlay", I think.

We use "autoplay" in one word in Chrome (and most products nowadays)

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:353:
AutoplayStatusFailed = 0,
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> This doesn't seem to quite match the histogram enum description.  What does
this really mean? Maybe add comments on what this really means?

Sorry about this, I forgot to update the histograms.xml file.

https://codereview.chromium.org/2404743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:355:
AutoplayStatusSucceeded = 3,
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> Why 3 and not 2?
> 
> Plus, this doesn't match the histogram values, which includes 2.

Oups. Same cause, I changed histograms and forgot to update. Sorry :-/

https://codereview.chromium.org/2404743002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2404743002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:101425: +  <int value="2" label="Failed
to autoplay but start() called on user gesture"/>
On 2016/10/10 at 17:05:10, Raymond Toy wrote:
> What is the intended difference between 1 and 2 and how does that match up
with the implementation?

I've updated the list.

[Raymond Toy, 2016-10-10 20:11:15]

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:303: // gesture
while the AudioContext is locked.
What does "locked" mean here?  You mean the context is waiting for a gesture,
right?

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:353:
AutoplayStatusFailed = 0,
Some comments on what these actually mean would be useful. It's hard to tell
from the name exactly what these mean.

[mlamouri (slow - plz ping), 2016-10-10 21:26:32]

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:303: // gesture
while the AudioContext is locked.
On 2016/10/10 at 20:11:14, Raymond Toy wrote:
> What does "locked" mean here?  You mean the context is waiting for a gesture,
right?

Used "requires a user gesture" which matches the m_userGestureRequired member.

https://codereview.chromium.org/2404743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:353:
AutoplayStatusFailed = 0,
On 2016/10/10 at 20:11:15, Raymond Toy wrote:
> Some comments on what these actually mean would be useful. It's hard to tell
from the name exactly what these mean.

Done.

[Raymond Toy, 2016-10-10 21:30:09]

lgtm for the core webaudio parts.

You should have haraken@ look at BaseAudioContextTests to see if the oilpan
(tracing) parts are correct.

[haraken, 2016-10-11 03:45:48]

LGTM

https://codereview.chromium.org/2404743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
(right):

https://codereview.chromium.org/2404743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:165:
ScriptState::Scope scope(getScriptStateFrom(childDocument()));

Or you can use V8TestingScope, which will provide basic functionalities to test
things that involve V8.

[mlamouri (slow - plz ping), 2016-10-14 23:55:55]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 23:56:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-15 04:22:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-15 04:22:26]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-10-15 18:23:25]

mlamouri@chromium.org changed reviewers:
+ isherman@chromium.org

[mlamouri (slow - plz ping), 2016-10-15 18:23:26]

+isherman@ for tools/metrics/

[haraken, 2016-10-17 00:57:33]

https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:150:
recordAutoplayStatus();

This is safe but in general it's not encouraged to do something complicated in a
destructor.

Would there be any place other than the destructor to call
recordAutoplayStatus()?

[mlamouri (slow - plz ping), 2016-10-17 11:34:45]

https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:150:
recordAutoplayStatus();
On 2016/10/17 at 00:57:33, haraken wrote:
> This is safe but in general it's not encouraged to do something complicated in
a destructor.
> 
> Would there be any place other than the destructor to call
recordAutoplayStatus()?

I can call this in "uninitialize" which I did first with a DCHECH() in the dtor.
However, the DCHECK would get hit by some unit tests which makes me think that
"uninitialize" is only called in some circumstances.

The alternative is to record the histogram in uninitialize, reset the autoplay
status then record in dtor if the status is  set. However, I'm not sure it makes
this any better and makes the logic more complex and flaky.

WDYT?

[haraken, 2016-10-17 12:56:25]

On 2016/10/17 11:34:45, mlamouri wrote:
>
https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/2404743002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:150:
> recordAutoplayStatus();
> On 2016/10/17 at 00:57:33, haraken wrote:
> > This is safe but in general it's not encouraged to do something complicated
in
> a destructor.
> > 
> > Would there be any place other than the destructor to call
> recordAutoplayStatus()?
> 
> I can call this in "uninitialize" which I did first with a DCHECH() in the
dtor.
> However, the DCHECK would get hit by some unit tests which makes me think that
> "uninitialize" is only called in some circumstances.
> 
> The alternative is to record the histogram in uninitialize, reset the autoplay
> status then record in dtor if the status is  set. However, I'm not sure it
makes
> this any better and makes the logic more complex and flaky.
> 
> WDYT?

Conceptually uninitialize() should be always called.

If we can move the UMA reporting to uninitialize(), that is better. (Although
the UMA reporting in the destructor is safe in this specific case.)

[Ilya Sherman, 2016-10-17 23:31:18]

metrics lgtm

[mlamouri (slow - plz ping), 2016-10-18 12:32:55]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-10-18 12:32:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rtoy@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2404743002/#ps100001
(title: "move recording")

[commit-bot: I haz the power, 2016-10-18 12:33:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 15:28:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-18 15:30:11]

Description was changed from

==========
Web Audio: record autoplay status when an AudioContext is destroyed.

This is recording if an AudioContext with autoplay restrictions was
block, allowed to play and also record whether allowing start() to
activate the AudioContext would have allowed playback.

BUG=614115
TEST=BaseAudioContextTest.cpp (unit tests)
==========

to

==========
Web Audio: record autoplay status when an AudioContext is destroyed.

This is recording if an AudioContext with autoplay restrictions was
block, allowed to play and also record whether allowing start() to
activate the AudioContext would have allowed playback.

BUG=614115
TEST=BaseAudioContextTest.cpp (unit tests)

Committed: https://crrev.com/86a60cb0983a8bd9689d5c0ce7684f04799ada02
Cr-Commit-Position: refs/heads/master@{#425966}
==========

[commit-bot: I haz the power, 2016-10-18 15:30:12]

Patchset 6 (id:??) landed as
https://crrev.com/86a60cb0983a8bd9689d5c0ce7684f04799ada02
Cr-Commit-Position: refs/heads/master@{#425966}