Implement MediaStreamTrack contentHint skeleton.

Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org

Committed: https://crrev.com/a592cf9a183f3caa8f9df64c962277d57c9d5662
Cr-Commit-Position: refs/heads/master@{#438279}

[pbos, 2016-11-15 18:22:58]

rebase

[pbos, 2016-12-01 19:25:37]

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

[commit-bot: I haz the power, 2016-12-01 19:26: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-12-01 21:37:27]

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

[commit-bot: I haz the power, 2016-12-01 21:37:28]

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

[pbos, 2016-12-07 19:58:19]

skeleton done with enums instead of strings

[pbos, 2016-12-07 20:04:34]

add contentHint to global interface listing

[pbos, 2016-12-07 20:06:45]

Description was changed from

==========
Implement video MediaStreamTrack contentHints.

Also implements audio MediaStreamTracks as no-ops, implementing the
correct setters and getters, but the implementation does nothing with
the hint.

WIP, not ready for review, uploaded to send patches around easily.

BUG=chromium:653531
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are passed to
audio/video sinks inside Chromium.

BUG=chromium:653531
==========

[pbos, 2016-12-07 20:06:45]

pbos@chromium.org changed reviewers:
+ guidou@webrtc.org

[pbos, 2016-12-07 20:08:29]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are passed to
audio/video sinks inside Chromium.

BUG=chromium:653531
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(under a flag) and passed to audio/video sinks inside Chromium.

BUG=chromium:653531
==========

[pbos, 2016-12-07 20:09:41]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(under a flag) and passed to audio/video sinks inside Chromium.

BUG=chromium:653531
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
==========

[pbos, 2016-12-07 20:09:51]

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

[commit-bot: I haz the power, 2016-12-07 20:10:39]

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

[pbos, 2016-12-07 20:11:23]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org
==========

[pbos, 2016-12-07 20:11:24]

pbos@chromium.org changed reviewers:
+ esprehn@chromium.org

[pbos, 2016-12-07 20:12:32]

PTAL, I think this is ready for review.

I'd like to do the part that handles content hints in the WebRTC video sink as a
separate CL.

FWIW: This is technically spec compliant as the hint may be ignored by sinks.

[commit-bot: I haz the power, 2016-12-07 21:06:40]

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

[commit-bot: I haz the power, 2016-12-07 21:06:41]

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

[pbos, 2016-12-07 23:01:43]

add getter + fix windows

[pbos, 2016-12-07 23:02:07]

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

[commit-bot: I haz the power, 2016-12-07 23:03:00]

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-12-07 23:06:48]

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

[commit-bot: I haz the power, 2016-12-07 23:06:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[pbos, 2016-12-08 01:58:04]

rebase

[pbos, 2016-12-08 01:59:38]

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

[commit-bot: I haz the power, 2016-12-08 02:00:08]

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

[esprehn, 2016-12-08 04:41:14]

What's the plan for onion souping content/renderer/media so we don't need all of
this plumbing?

[commit-bot: I haz the power, 2016-12-08 05:25:01]

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

[commit-bot: I haz the power, 2016-12-08 05:25:02]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2016-12-08 10:13:38]

guidou@chromium.org changed reviewers:
+ guidou@chromium.org
- guidou@webrtc.org

[Guido Urdaneta, 2016-12-08 10:22:56]

On 2016/12/08 04:41:14, esprehn wrote:
> What's the plan for onion souping content/renderer/media so we don't need all
of
> this plumbing?

We want to onion soup it, but the stuff in content layer is quite complex and
used not only by Blink, but also by Pepper.
The current plan is to first close some important spec-compliance bugs and port
all IPC to Mojo (already under way).
Once this is done, we plan to start removing layers and moving as much as
possible to blink, but it will take some time.

[Guido Urdaneta, 2016-12-08 10:35:36]

https://codereview.chromium.org/2501623003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2501623003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:112:
DEFINE_STATIC_LOCAL(String, speech, ("speech"));
Do you really need these? You are only using them once.
Maybe it would be better to define them as char[] and reuse them also in
setContentHint.

[pbos, 2016-12-08 16:58:27]

const chars

[pbos, 2016-12-08 16:58:32]

PTAL

https://codereview.chromium.org/2501623003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2501623003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:112:
DEFINE_STATIC_LOCAL(String, speech, ("speech"));
On 2016/12/08 10:35:36, Guido Urdaneta wrote:
> Do you really need these? You are only using them once.
> Maybe it would be better to define them as char[] and reuse them also in
> setContentHint.

Done, do you prefer this over inlining the strings?

[Guido Urdaneta, 2016-12-08 17:04:48]

lgtm

[pbos, 2016-12-08 17:53:35]

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

[commit-bot: I haz the power, 2016-12-08 17:54:14]

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

[pbos, 2016-12-08 18:20:45]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org
==========

[pbos, 2016-12-08 18:20:45]

pbos@chromium.org changed reviewers:
+ jam@chromium.org

[pbos, 2016-12-08 18:20:55]

+R jam@ for content/public/renderer/media_stream_sink.h

[commit-bot: I haz the power, 2016-12-08 19:42:10]

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

[commit-bot: I haz the power, 2016-12-08 19:42:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2016-12-08 23:44:15]

lgtm

[esprehn, 2016-12-09 02:30:09]

https://codereview.chromium.org/2501623003/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/2501623003/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:84: DVLOG(1) <<
"MediaStreamAudioTrack@" << this << "::SetContentHint()";
Do you want traces instead? In general the media code seems to spam the console
a lot which is probably bad.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html
(right):

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:4:
<canvas id="canvas"/>
</canvas>

/ does nothing on a tag, this is actually putting the <script> element inside
the <canvas> element.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:7:
createAudioTrack = function() {
function createAudioTrack

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:18:
createVideoTrack = function() {
ditto

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:47: static
const char contentHintStringNone[] = "";
k prefix constants

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:149: //
TODO(pbos): Log warning?
What does the spec say you should do with an invalid value?

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:161: //
TODO(pbos): Log warning?
ditto

[pbos, 2016-12-09 17:34:41]

review feedback

[pbos, 2016-12-09 17:34:46]

PTAL

https://codereview.chromium.org/2501623003/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/2501623003/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:84: DVLOG(1) <<
"MediaStreamAudioTrack@" << this << "::SetContentHint()";
On 2016/12/09 02:30:09, esprehn wrote:
> Do you want traces instead? In general the media code seems to spam the
console
> a lot which is probably bad.

Not sure I actually want this at all. Removed it, let me know if you disagree.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html
(right):

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:4:
<canvas id="canvas"/>
On 2016/12/09 02:30:09, esprehn wrote:
> </canvas>
> 
> / does nothing on a tag, this is actually putting the <script> element inside
> the <canvas> element.

Done.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:7:
createAudioTrack = function() {
On 2016/12/09 02:30:09, esprehn wrote:
> function createAudioTrack

Done.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-contentHint.html:18:
createVideoTrack = function() {
On 2016/12/09 02:30:09, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:47: static
const char contentHintStringNone[] = "";
On 2016/12/09 02:30:09, esprehn wrote:
> k prefix constants

Done.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:149: //
TODO(pbos): Log warning?
On 2016/12/09 02:30:09, esprehn wrote:
> What does the spec say you should do with an invalid value?

Ignore, similar to enums. I put a comment in.

https://codereview.chromium.org/2501623003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:161: //
TODO(pbos): Log warning?
On 2016/12/09 02:30:09, esprehn wrote:
> ditto

Done.

[pbos, 2016-12-09 17:34:53]

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

[commit-bot: I haz the power, 2016-12-09 17:35:29]

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

[pbos, 2016-12-09 22:33:49]

audio -> video in comment

[pbos, 2016-12-13 16:19:22]

rebase

[pbos, 2016-12-13 16:21:23]

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

[commit-bot: I haz the power, 2016-12-13 16:21:57]

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-12-13 18:03:04]

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

[commit-bot: I haz the power, 2016-12-13 18:03:05]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-12-13 18:24:25]

lgtm

https://codereview.chromium.org/2501623003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2501623003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:93:
BLINK_PLATFORM_EXPORT ContentHint contentHint() const;
Due to the big blink rename this will need to become GetContentHint() or you
need to rename the type ContentHintType.

[pbos, 2016-12-13 19:10:34]

ContentHint -> ContentHintType

[pbos, 2016-12-13 19:11:06]

https://codereview.chromium.org/2501623003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2501623003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:93:
BLINK_PLATFORM_EXPORT ContentHint contentHint() const;
On 2016/12/13 18:24:25, esprehn wrote:
> Due to the big blink rename this will need to become GetContentHint() or you
> need to rename the type ContentHintType.

Done (->ContentHintType).

[pbos, 2016-12-13 19:11:19]

The CQ bit was checked by pbos@chromium.org

[pbos, 2016-12-13 19:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
jam@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2501623003/#ps220001
(title: "ContentHint -> ContentHintType")

[commit-bot: I haz the power, 2016-12-13 19:11:43]

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-12-13 21:30:20]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481656279051210,
"parent_rev": "f62de52ee32db074173945c99f24158f50b662b4", "commit_rev":
"1c7366363377817ed0e31edc741b088cd6525807"}

[commit-bot: I haz the power, 2016-12-13 21:30:44]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org

Review-Url: https://codereview.chromium.org/2501623003
==========

[commit-bot: I haz the power, 2016-12-13 21:30:45]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-12-13 21:32:21]

Description was changed from

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org

Review-Url: https://codereview.chromium.org/2501623003
==========

to

==========
Implement MediaStreamTrack contentHint skeleton.

No sinks yet handle MediaStreamTrack content hints, but they are exposed
(as a runtime-enabled feature) and passed to audio/video sinks inside
Chromium.

BUG=chromium:653531
R=esprehn@chromium.org, guidou@chromium.org, jam@chromium.org

Committed: https://crrev.com/a592cf9a183f3caa8f9df64c962277d57c9d5662
Cr-Commit-Position: refs/heads/master@{#438279}
==========

[commit-bot: I haz the power, 2016-12-13 21:32:23]

Patchset 12 (id:??) landed as
https://crrev.com/a592cf9a183f3caa8f9df64c962277d57c9d5662
Cr-Commit-Position: refs/heads/master@{#438279}

[mcasas, 2016-12-13 21:58:07]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2016-12-13 21:58:08]

https://codereview.chromium.org/2501623003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right):

https://codereview.chromium.org/2501623003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:45:
[RuntimeEnabled=MediaStreamTrackContentHint] attribute DOMString contentHint;
This should be in a file on its own since [1]
defines a partial interface. Please add a link 
to the wicg spec in that file. It's fine to do it
in a follow-up patch.


[1] https://wicg.github.io/mst-content-hint/#extension-to-mediastreamtrack