Add a UseCounter for HTMLMediaElement.controller

Add a UseCounter for HTMLMediaElement.controller

MediaController isn't getting the attention it needs right now. Count
usage to help decide whether or not removal is feasible. Feasible does
not imply desirable, but the data can inform the decision.

Count a non-null controller without distinguishing how it was set
(markup or script) to measure the usage that affects behavior.

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166651

[philipj_slow, 2014-02-06 05:58:02]

For your consideration. If removal just isn't on the table, please reject this
review so that we don't waste more time on the topic :)

[nessy, 2014-02-06 11:23:52]

I assume your aim is to actually remove MediaController and
@mediagroup. We might as well have this discussion now, since these
are currently broken and therefore it can't be expected that the use
counter will lead to many hits.

I actually don't believe MediaController should be removed from the
API. People have continuously been asking for means to synchronize
independent media streams with each other and been frustrated that
@mediagroup does not work as expected. @mediagroup is particularly
necessary for synchronizing sign language video to another video, but
I've also heard people ask for it to synchronize different audio
tracks to each other or a video with an different language sound
track.

There are several bugs for broken MediaController and @mediagroup
(e.g. issues 171403, 141337, 171480, 143112). IMO, the bugs should be
fixed and not the feature removed.

Silvia.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[philipj_slow, 2014-02-06 17:33:15]

This started as a private email from me to Aaron and Andrew, but I'll try to
summarize.

MediaController is not a bad feature, I don't think it should be removed from
the spec. However, I think we should consider removing the implementation from
Blink, because:

1. It's incomplete and seems not be getting any love since the WebKit/Blink
fork. In particular, the media framework integration hasn't been done, so media
elements are not actually kept in sync. Proper sync between media elements is
the whole point of the feature!

2. Because of the lack of sync, Web authors aren't really getting anything they
couldn't do without the API.

3. The implementation is made of code, and that amounts to a small but non-zero
maintenance cost to keep the code working at its current level.

Of course, one could fix the bugs instead, but my assumption is that no one is
going to do that anytime soon. Because it's not implemented in Firefox or IE
yet, it seems plausible that removal is still possible. It could be revived at
the time when someone is willing to do the hard work with media framework
integration to make it truly useful.

Aaron was not keen on removing, but Andrew was more positive, and suggested use
counters to see if it's an option or not.

[acolwell GONE FROM CHROMIUM, 2014-02-06 17:45:53]

lgtm

I think this is worth seeing much this is used. We can use that information to
see whether its a big deal to remove this feature OR monitor the increase in
usage as we improve the implementation. Either way I think it is worth
collecting some data.

[scherkus (not reviewing), 2014-02-06 18:26:13]

lgtm

On 2014/02/06 17:45:53, acolwell wrote:
> lgtm
> 
> I think this is worth seeing much this is used. We can use that information to
> see whether its a big deal to remove this feature OR monitor the increase in
> usage as we improve the implementation. Either way I think it is worth
> collecting some data.

yeah I'm all for collecting data either way

[philipj_slow, 2014-02-06 18:26:43]

On 2014/02/06 17:45:53, acolwell wrote:
> lgtm
> 
> I think this is worth seeing much this is used. We can use that information to
> see whether its a big deal to remove this feature OR monitor the increase in
> usage as we improve the implementation. Either way I think it is worth
> collecting some data.

I realized that my first patch wouldn't count the declarative case (mediagroup
attribute), so I simplified this into a single counter which should give us a
good idea. Still LGTM?

[philipj_slow, 2014-02-06 18:26:45]

On 2014/02/06 17:45:53, acolwell wrote:
> lgtm
> 
> I think this is worth seeing much this is used. We can use that information to
> see whether its a big deal to remove this feature OR monitor the increase in
> usage as we improve the implementation. Either way I think it is worth
> collecting some data.

I realized that my first patch wouldn't count the declarative case (mediagroup
attribute), so I simplified this into a single counter which should give us a
good idea. Still LGTM?

[acolwell GONE FROM CHROMIUM, 2014-02-06 18:39:22]

Yes. LGTM v2

[philipj_slow, 2014-02-06 18:43:13]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-02-06 18:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/156493002/80001

[commit-bot: I haz the power, 2014-02-06 18:43:25]

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

[commit-bot: I haz the power, 2014-02-06 18:43:26]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 317.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
683242def9ed4cd134002b01f41d59a816d2f266..d7a2523aa798970ebf4704f390bc1a4634a88938
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -317,6 +317,7 @@ public:
         EventSourceURL,
         WebSocketURL,
         UnsafeEvalBlocksCSSOM,
+        HTMLMediaElementControllerNotNull,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         NumberOfFeatures, // This enum value must be last.

[philipj_slow, 2014-02-06 18:47:38]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-02-06 18:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/156493002/190001

[commit-bot: I haz the power, 2014-02-06 21:17:36]

Change committed as 166651