Deprecating AutoplayExperimentHelper

Deprecating AutoplayExperimentHelper

This CL deprecates AutoplayExperimentHelper as it is stale and has been
superceeded by the latest autoplay changes. Changes include:
* Removing AutoplayUmaHelper
* Removing legacy UMAs recorded by AutoplayUmaHelper

BUG=666370
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/87b9e3b5bcf706b2a3313bfc44606325614b9eac
Cr-Commit-Position: refs/heads/master@{#435424}

[Zhiqiang Zhang (Slow), 2016-11-18 13:29:20]

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

[commit-bot: I haz the power, 2016-11-18 13:29:30]

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-11-18 13:57:49]

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

[commit-bot: I haz the power, 2016-11-18 13:57:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-11-21 20:50:44]

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

[commit-bot: I haz the power, 2016-11-21 20:51:11]

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-11-21 22:52:56]

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

[commit-bot: I haz the power, 2016-11-21 22:52:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-11-22 12:05:46]

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

[commit-bot: I haz the power, 2016-11-22 12:06:06]

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-11-22 12:10:22]

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

[commit-bot: I haz the power, 2016-11-22 12:10:22]

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_...)

[Zhiqiang Zhang (Slow), 2016-11-22 13:11:19]

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

[commit-bot: I haz the power, 2016-11-22 13:11:30]

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

[Zhiqiang Zhang (Slow), 2016-11-22 13:13:26]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-22 13:13:27]

PTAL

Anton, I'm sending all reviews to you this week but they are not urgent, so take
your time :)

[commit-bot: I haz the power, 2016-11-22 15:19:53]

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

[commit-bot: I haz the power, 2016-11-22 15:19:53]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-11-22 23:14:40]

avayvod@chromium.org changed reviewers:
+ liberato@chromium.org

[whywhat, 2016-11-22 23:14:41]

+Frank who IIRC wrote this originally - maybe there's some extra cleanup to do

otherwise, rubberstamp lgtm

[liberato (no reviews please), 2016-11-22 23:25:21]

i think that's all of it.  thanks for removing it.

lgtm.

-fl

[Zhiqiang Zhang (Slow), 2016-11-23 11:44:24]

zqzhang@chromium.org changed reviewers:
+ foolip@chromium.org, jwd@chromium.org, nasko@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-23 11:44:25]

+foolip: Webkit/
+nasko: content/ (incl. IPC)
+jwd: histograms.xml

[foolip, 2016-11-23 12:40:56]

third_party/WebKit/ LGTM % nits

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:676:
UserGestureIndicator::utilizeUserGesture()) {
This isn't what I had expected, but https://codereview.chromium.org/1799253002
explains why it's different from before the experiment.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: const
RecordMetricsBehavior recordMetrics) {
Remove the now unused argument.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2040: if (m_autoplaying
&& m_paused && autoplay()) {
This now ends up looking a bit silly and not trivial to understand. I suggest:

if (document().isSandboxed(SandboxAutomaticFeatures))
  return false;
return m_autoplaying && m_paused && autoplay();

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2173:
UserGestureIndicator::utilizeUserGesture();
Optional nit: hoist utilizeUserGesture() to above. After
https://codereview.chromium.org/2529593002 the above processingUserGesture()
would otherwise be the only call. If you like I can include it in my CL, but
it'll conflict with this one.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:306: // TODO(zqzhang):
remove these callbacks and related plumbing in layout code
Can you clean this up now or send a CL based on this showing what it amounts to?
Looks pretty easy to remove.

[Zhiqiang Zhang (Slow), 2016-11-23 17:18:28]

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:676:
UserGestureIndicator::utilizeUserGesture()) {
On 2016/11/23 12:40:55, foolip wrote:
> This isn't what I had expected, but https://codereview.chromium.org/1799253002
> explains why it's different from before the experiment.

This logic was flattened from AutoplayExperimentalHelper. Can you elaborate how
it's different and what do you expect?

[nasko, 2016-11-23 17:54:12]

content/ LGTM

[Zhiqiang Zhang (Slow), 2016-11-23 18:10:13]

Description was changed from

==========
Deprecating AutoplayExperimentHelper

This CL deprecates AutoplayExperimentHelper as it is stale and has been
superceeded by the latest autoplay changes. Changes include:
* Removing AutoplayUmaHelper
* Removing legacy UMAs recorded by AutoplayUmaHelper

BUG=666370
==========

to

==========
Deprecating AutoplayExperimentHelper

This CL deprecates AutoplayExperimentHelper as it is stale and has been
superceeded by the latest autoplay changes. Changes include:
* Removing AutoplayUmaHelper
* Removing legacy UMAs recorded by AutoplayUmaHelper

BUG=666370
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Zhiqiang Zhang (Slow), 2016-11-23 18:21:27]

Patchset #5 (id:80001) has been deleted

[Zhiqiang Zhang (Slow), 2016-11-23 18:27:33]

zqzhang@chromium.org changed reviewers:
+ ojan@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-23 18:27:34]

+ojan for the layout code changes, which is pretty much of an undo of
https://codereview.chromium.org/1370723002

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: const
RecordMetricsBehavior recordMetrics) {
On 2016/11/23 12:40:55, foolip wrote:
> Remove the now unused argument.

Done.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2040: if (m_autoplaying
&& m_paused && autoplay()) {
On 2016/11/23 12:40:55, foolip wrote:
> This now ends up looking a bit silly and not trivial to understand. I suggest:
> 
> if (document().isSandboxed(SandboxAutomaticFeatures))
>   return false;
> return m_autoplaying && m_paused && autoplay();

Done.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2173:
UserGestureIndicator::utilizeUserGesture();
On 2016/11/23 12:40:55, foolip wrote:
> Optional nit: hoist utilizeUserGesture() to above. After
> https://codereview.chromium.org/2529593002 the above processingUserGesture()
> would otherwise be the only call. If you like I can include it in my CL, but
> it'll conflict with this one.

Did you mean to move it to utilizeUserGesture() to the if statement (replacing
the processingUserGesture())?
Please do it in your CL and I'll do rebase later :)

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:306: // TODO(zqzhang):
remove these callbacks and related plumbing in layout code
On 2016/11/23 12:40:55, foolip wrote:
> Can you clean this up now or send a CL based on this showing what it amounts
to?
> Looks pretty easy to remove.

Done.

[ojan, 2016-11-23 18:54:33]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-11-23 18:54:33]

lgtm

<3

[ojan, 2016-11-23 18:54:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, liberato@chromium.org, nasko@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2510353004/#ps120001
(title: "cleaned up the layout code no longer used")

[commit-bot: I haz the power, 2016-11-23 18:55:13]

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

[foolip, 2016-11-23 19:15:55]

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:676:
UserGestureIndicator::utilizeUserGesture()) {
On 2016/11/23 17:18:28, Zhiqiang Zhang wrote:
> On 2016/11/23 12:40:55, foolip wrote:
> > This isn't what I had expected, but
https://codereview.chromium.org/1799253002
> > explains why it's different from before the experiment.
> 
> This logic was flattened from AutoplayExperimentalHelper. Can you elaborate
how
> it's different and what do you expect?

Originally it was processingUserGesture(), so that's what I expected. Took me
some digging to figure out where utilizeUserGesture() had come from, commented
as an FYI if anyone else was surprised.

https://codereview.chromium.org/2510353004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2173:
UserGestureIndicator::utilizeUserGesture();
On 2016/11/23 18:27:34, Zhiqiang Zhang wrote:
> On 2016/11/23 12:40:55, foolip wrote:
> > Optional nit: hoist utilizeUserGesture() to above. After
> > https://codereview.chromium.org/2529593002 the above processingUserGesture()
> > would otherwise be the only call. If you like I can include it in my CL, but
> > it'll conflict with this one.
> 
> Did you mean to move it to utilizeUserGesture() to the if statement (replacing
> the processingUserGesture())?
> Please do it in your CL and I'll do rebase later :)

Will do, but will base it on your CL since it's going through CQ now.

[commit-bot: I haz the power, 2016-11-23 20:24:00]

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

[commit-bot: I haz the power, 2016-11-23 20:24:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jwd, 2016-11-24 16:16:31]

https://codereview.chromium.org/2510353004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2510353004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4180: -<histogram
name="Blink.MediaElement.Autoplay" enum="MediaElementAutoPlay">
Can you mark as obsolete, rather than delete these.

[Zhiqiang Zhang (Slow), 2016-11-24 16:23:50]

PTAL for jwd@

https://codereview.chromium.org/2510353004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2510353004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4180: -<histogram
name="Blink.MediaElement.Autoplay" enum="MediaElementAutoPlay">
On 2016/11/24 16:16:31, jwd wrote:
> Can you mark as obsolete, rather than delete these.

Done.

[Zhiqiang Zhang (Slow), 2016-11-24 16:24:44]

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

[commit-bot: I haz the power, 2016-11-24 16:25:13]

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-11-24 16:27:53]

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

[commit-bot: I haz the power, 2016-11-24 16:27:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Zhiqiang Zhang (Slow), 2016-11-24 18:34:44]

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

[commit-bot: I haz the power, 2016-11-24 18:35:05]

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-11-24 20:10:50]

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

[commit-bot: I haz the power, 2016-11-24 20:10:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-11-29 15:56:07]

Ping! jwd@, can you PTAL histograms.xml?

[jwd, 2016-11-30 19:40:39]

lgtm

[Zhiqiang Zhang (Slow), 2016-11-30 19:54:37]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-30 19:54:37]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
liberato@chromium.org, nasko@chromium.org, avayvod@chromium.org,
ojan@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2510353004/#ps180001
(title: "rebased")

[commit-bot: I haz the power, 2016-11-30 19:55:06]

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-11-30 21:33:57]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480535677148200,
"parent_rev": "f57cd0a3838585ad3b4f76513738d7274d910ed5", "commit_rev":
"d781e748f04763b03c1923892c1e8b03a47420f8"}

[commit-bot: I haz the power, 2016-11-30 21:34:29]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-11-30 21:37:46]

Description was changed from

==========
Deprecating AutoplayExperimentHelper

This CL deprecates AutoplayExperimentHelper as it is stale and has been
superceeded by the latest autoplay changes. Changes include:
* Removing AutoplayUmaHelper
* Removing legacy UMAs recorded by AutoplayUmaHelper

BUG=666370
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Deprecating AutoplayExperimentHelper

This CL deprecates AutoplayExperimentHelper as it is stale and has been
superceeded by the latest autoplay changes. Changes include:
* Removing AutoplayUmaHelper
* Removing legacy UMAs recorded by AutoplayUmaHelper

BUG=666370
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/87b9e3b5bcf706b2a3313bfc44606325614b9eac
Cr-Commit-Position: refs/heads/master@{#435424}
==========

[commit-bot: I haz the power, 2016-11-30 21:37:48]

Patchset 9 (id:??) landed as
https://crrev.com/87b9e3b5bcf706b2a3313bfc44606325614b9eac
Cr-Commit-Position: refs/heads/master@{#435424}