[Blink, Media] Added controlsList to HTMLMediaElement

[Blink, Media] Added controlsList to HTMLMediaElement

Adds a DOMTokenList backed controlsList/controlslist attribute to HTMLMediaElement with three keywords: nodownload, nofullscreen and noremoteplayback.

Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/discussion

BUG=650174, 685018
TEST=manual+layout tests

Review-Url: https://codereview.chromium.org/2657723002
Cr-Commit-Position: refs/heads/master@{#455926}
Committed: https://chromium.googlesource.com/chromium/src/+/caca06866f37c79df89cab9a634bb7497177236e

[whywhat, 2017-01-25 02:02:51]

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

[whywhat, 2017-01-25 02:02:52]

Mounir for the initial review

[whywhat, 2017-01-25 02:02:57]

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

[commit-bot: I haz the power, 2017-01-25 02:03: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, 2017-01-25 04:15:26]

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

[commit-bot: I haz the power, 2017-01-25 04:15:27]

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

[mlamouri (slow - plz ping), 2017-01-25 05:27:32]

Looks generally good but this will require some tests :)

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/BUILD.gn (right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/BUILD.gn:112:
"HTMLMediaElementControlsList.cpp",
You are missing the header.

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp
(right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:14:
"noremoteplayback"};
probably a spec/bikeshed question but "noremote" sounded better.

[fs, 2017-01-25 09:34:16]

Drive-by (on attribute handling)

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:568:
m_controlsList->setValue(fastGetAttribute(controlsAttr));
m_controlsList->setValue(params.newValue);

(and params.oldValue != params.newValue if you don't want to reset the attribute
per the below.)

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2400:
setAttribute(controlsAttr, m_controlsList->value());
Could (should?) use setSynchronizedLazyAttribute, which means you wouldn't
re-enter attributeChanged/parseAttribute if the call originated there.

[whywhat, 2017-03-01 14:43:56]

rebased

[whywhat, 2017-03-01 16:14:33]

Addressed minor comments

[whywhat, 2017-03-01 16:17:20]

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/BUILD.gn (right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/BUILD.gn:112:
"HTMLMediaElementControlsList.cpp",
On 2017/01/25 at 05:27:31, mlamouri wrote:
> You are missing the header.

Done. It was consistent with a good half of the list though.

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:568:
m_controlsList->setValue(fastGetAttribute(controlsAttr));
On 2017/01/25 at 09:34:16, fs wrote:
> m_controlsList->setValue(params.newValue);
> 
> (and params.oldValue != params.newValue if you don't want to reset the
attribute per the below.)

Done.

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2400:
setAttribute(controlsAttr, m_controlsList->value());
On 2017/01/25 at 09:34:16, fs wrote:
> Could (should?) use setSynchronizedLazyAttribute, which means you wouldn't
re-enter attributeChanged/parseAttribute if the call originated there.

Done. Have to call updateControlsVisibility here.

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp
(right):

https://codereview.chromium.org/2657723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:14:
"noremoteplayback"};
On 2017/01/25 at 05:27:31, mlamouri wrote:
> probably a spec/bikeshed question but "noremote" sounded better.

Ack.

[mlamouri (slow - plz ping), 2017-03-01 16:20:21]

sg but it's missing tests :)

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.idl (right):

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.idl:79: [CEReactions,
SameObject, PutForwards=value] readonly attribute DOMTokenList controlsList;
Can you Measure this?

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp
(right):

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:30:
HTMLMediaElementControlsList::~HTMLMediaElementControlsList() {}
= default

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.h (right):

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.h:22: }
Sounds like you could drop the ::create()?

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.h:40: #endif
style: `#endif  // HTMLMediaElementControlsList_h`

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2657723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:705: return
false;
Can you add a histogram for this and the ones below? Alternatively, a UseCounter
could be added to check which tokens are set instead of which ones are used.

[whywhat, 2017-03-02 20:42:47]

Added layout tests

[whywhat, 2017-03-02 20:55:36]

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

[commit-bot: I haz the power, 2017-03-02 20:56: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, 2017-03-02 22:58:12]

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

[commit-bot: I haz the power, 2017-03-02 22:58:12]

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

[mlamouri (slow - plz ping), 2017-03-03 10:38:41]

sgtm but it needs to be updated to match the latest proposal I guess

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html
(right):

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html:2:
<title>Test adding keywords to controlsList hides buttons</title>
Can you put the tests in the controls/ directory? or create one if there isn't
in the http/ folder.

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html:16:
v.controlsList.add('nodownload');
By which mechanism does this create a re-layout of the controls?

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

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2450:
setSynchronizedLazyAttribute(controlsAttr, m_controlsList->value());
I guess this needs to be updated?

[whywhat, 2017-03-03 15:41:06]

s/controls/controlslist

[whywhat, 2017-03-03 15:53:58]

Updated interface expectations

[whywhat, 2017-03-03 16:02:31]

Moved the layout tests to controls/

[whywhat, 2017-03-03 16:04:55]

PTAL

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html
(right):

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html:2:
<title>Test adding keywords to controlsList hides buttons</title>
On 2017/03/03 at 10:38:41, mlamouri wrote:
> Can you put the tests in the controls/ directory? or create one if there isn't
in the http/ folder.

Done.

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/controls-list-add-hide.html:16:
v.controlsList.add('nodownload');
On 2017/03/03 at 10:38:41, mlamouri wrote:
> By which mechanism does this create a re-layout of the controls?

Each time it changes, controlsList object tells HTMLMediaElement it's updated
and HTMLMediaElement updates controls visibility.
Does this answer your question?

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

https://codereview.chromium.org/2657723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2450:
setSynchronizedLazyAttribute(controlsAttr, m_controlsList->value());
On 2017/03/03 at 10:38:41, mlamouri wrote:
> I guess this needs to be updated?

Yes, done.

[whywhat, 2017-03-03 16:04:58]

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

[commit-bot: I haz the power, 2017-03-03 16:05:08]

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, 2017-03-03 16:07:48]

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

[commit-bot: I haz the power, 2017-03-03 16:07:48]

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

[whywhat, 2017-03-03 16:19:50]

Rebased and readded the tests

[whywhat, 2017-03-03 16:22:58]

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

[commit-bot: I haz the power, 2017-03-03 16:23:19]

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

[mlamouri (slow - plz ping), 2017-03-03 17:32:44]

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2456:
updateControlsVisibility();
What about calling:
```
if (mediaControls())
  mediaControls->reset();
```

Bonus point for having `onControlsListUpdated()` on the MediaControls class and
call `reset()` in there for now.

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp
(right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:16: bool
isTokenSupported(const AtomicString& token) {
Why is this an anonymous method if it's used in only one place?

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:30:
HTMLMediaElementControlsList::~HTMLMediaElementControlsList() {}
= default;

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:705: return
false;
Can you add a histogram that reports how many time these are used? Maybe a
UseCounter would be enough.

[commit-bot: I haz the power, 2017-03-03 18:21:33]

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

[commit-bot: I haz the power, 2017-03-03 18:21:34]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-03-07 12:58:56]

Fixed comments

[whywhat, 2017-03-07 12:59:46]

UseCounters is a pain to rebase -- I imagine it's a pain to merge too :)

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2456:
updateControlsVisibility();
On 2017/03/03 at 17:32:44, mlamouri wrote:
> What about calling:
> ```
> if (mediaControls())
>   mediaControls->reset();
> ```
> 
> Bonus point for having `onControlsListUpdated()` on the MediaControls class
and call `reset()` in there for now.

Done.

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp
(right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:16: bool
isTokenSupported(const AtomicString& token) {
On 2017/03/03 at 17:32:44, mlamouri wrote:
> Why is this an anonymous method if it's used in only one place?

Done.

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElementControlsList.cpp:30:
HTMLMediaElementControlsList::~HTMLMediaElementControlsList() {}
On 2017/03/03 at 17:32:44, mlamouri wrote:
> = default;

Done.

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2657723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:705: return
false;
On 2017/03/03 at 17:32:44, mlamouri wrote:
> Can you add a histogram that reports how many time these are used? Maybe a
UseCounter would be enough.

Done.

[whywhat, 2017-03-07 12:59:52]

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

[commit-bot: I haz the power, 2017-03-07 13:00:20]

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

[whywhat, 2017-03-07 14:31:04]

Description was changed from

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Uploading a WIP patch to implement controlsList in HTMLMediaElement.
Spec change is discussed here: https://github.com/whatwg/html/issues/2293

BUG=None
TEST=manual
==========

to

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Uploading a WIP patch to implement controlsList in HTMLMediaElement.
Spec change is discussed here: https://github.com/whatwg/html/issues/2293

BUG=650174,685018
TEST=manual+layout tests
==========

[commit-bot: I haz the power, 2017-03-07 15:19:24]

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

[commit-bot: I haz the power, 2017-03-07 15:19:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jochen (gone - plz use gerrit), 2017-03-09 16:09:58]

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

[jochen (gone - plz use gerrit), 2017-03-09 16:09:59]

can you add a link to the intent to ship to the CL description please?

[whywhat, 2017-03-09 16:13:18]

Description was changed from

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Uploading a WIP patch to implement controlsList in HTMLMediaElement.
Spec change is discussed here: https://github.com/whatwg/html/issues/2293

BUG=650174,685018
TEST=manual+layout tests
==========

to

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Uploading a WIP patch to implement controlsList in HTMLMediaElement.
Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/...

BUG=650174,685018
TEST=manual+layout tests
==========

[whywhat, 2017-03-09 16:13:28]

On 2017/03/09 at 16:09:59, jochen wrote:
> can you add a link to the intent to ship to the CL description please?

Done.

[whywhat, 2017-03-09 16:36:11]

Description was changed from

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Uploading a WIP patch to implement controlsList in HTMLMediaElement.
Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/...

BUG=650174,685018
TEST=manual+layout tests
==========

to

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Adds a DOMTokenList backed controlsList/controlslist attribute to
HTMLMediaElement with three keywords: nodownload, nofullscreen and
noremoteplayback.

Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/...

BUG=650174,685018
TEST=manual+layout tests
==========

[mlamouri (slow - plz ping), 2017-03-09 16:45:45]

lgtm with comments applied

\o/

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:607:
updateControlsVisibility();
should this be:
```
if (mediaControls())
  mediaControls()->onControlsListUpdated();
```

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2454: if
(fastGetAttribute(controlslistAttr) != m_controlsList->value()) {
nit: early return is da best :)

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:418:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEventsInShadow;
I think you can drop that

[whywhat, 2017-03-09 16:58:43]

Addressed nits

[whywhat, 2017-03-09 16:59:43]

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:607:
updateControlsVisibility();
On 2017/03/09 at 16:45:45, mlamouri wrote:
> should this be:
> ```
> if (mediaControls())
>   mediaControls()->onControlsListUpdated();
> ```

Done.

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2454: if
(fastGetAttribute(controlslistAttr) != m_controlsList->value()) {
On 2017/03/09 at 16:45:45, mlamouri wrote:
> nit: early return is da best :)

Done.

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2657723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:418:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEventsInShadow;
On 2017/03/09 at 16:45:45, mlamouri wrote:
> I think you can drop that

Done.

[whywhat, 2017-03-09 16:59:53]

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

[commit-bot: I haz the power, 2017-03-09 17:00:26]

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, 2017-03-09 17:17:00]

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

[commit-bot: I haz the power, 2017-03-09 17:17:01]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[DaleCurtis, 2017-03-09 18:45:47]

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

[DaleCurtis, 2017-03-09 18:45:49]

https://codereview.chromium.org/2657723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2657723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:71: if
(mediaElement.controlsList()->tokens().contains("nofullscreen")) {
Do you really want callers to have to know about token names? Seems fragile and
prone to typos. Why not have an explicit controlsList().allowFullscreenbutton()
or something similarly named?

[whywhat, 2017-03-09 19:55:10]

Encapsulated the keywords

[whywhat, 2017-03-09 19:56:45]

Thanks for the drive-by!

https://codereview.chromium.org/2657723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2657723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:71: if
(mediaElement.controlsList()->tokens().contains("nofullscreen")) {
On 2017/03/09 at 18:45:48, DaleCurtis wrote:
> Do you really want callers to have to know about token names? Seems fragile
and prone to typos. Why not have an explicit
controlsList().allowFullscreenbutton() or something similarly named?

Good suggestion! Done.

[whywhat, 2017-03-09 19:56:54]

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

[commit-bot: I haz the power, 2017-03-09 19:57:32]

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

[Rick Byers, 2017-03-09 20:00:57]

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

[Rick Byers, 2017-03-09 20:00:58]

virtual/webexposed LGTM

https://codereview.chromium.org/2657723002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.idl (right):

https://codereview.chromium.org/2657723002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.idl:79: [SameObject,
PutForwards=value] readonly attribute DOMTokenList controlsList;
nit please add the spec link in a comment above this line.  Our automated spec
conformance checking tools will use it.

[whywhat, 2017-03-09 20:03:34]

The CQ bit was unchecked by avayvod@chromium.org

[whywhat, 2017-03-09 20:05:20]

Added a link to the spec.

[DaleCurtis, 2017-03-09 20:05:35]

Cool, non-owner lgtm!

[whywhat, 2017-03-09 20:10:14]

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

[whywhat, 2017-03-09 20:10:18]

The CQ bit was unchecked by avayvod@chromium.org

[whywhat, 2017-03-09 20:10:25]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-03-09 20:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2657723002/#ps220001
(title: "Added a link to the spec.")

[commit-bot: I haz the power, 2017-03-09 20:10:34]

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, 2017-03-09 20:40:47]

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

[commit-bot: I haz the power, 2017-03-09 20:40:48]

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

[whywhat, 2017-03-09 20:43:34]

avayvod@chromium.org changed reviewers:
+ jwd@chromium.org
- jochen@chromium.org

[whywhat, 2017-03-09 20:43:35]

+jwd for histograms.xml changes

[whywhat, 2017-03-09 20:49:33]

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

[whywhat, 2017-03-09 20:49:34]

+haraken

[haraken, 2017-03-09 21:51:16]

histograms.xml LGTM

[whywhat, 2017-03-09 21:55:07]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2017-03-09 21:55:36]

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, 2017-03-10 00:40:02]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489096507813690,
"parent_rev": "5bf97f6bf369fc1d5b5bb77585f5c13dc218c216", "commit_rev":
"caca06866f37c79df89cab9a634bb7497177236e"}

[commit-bot: I haz the power, 2017-03-10 00:40:44]

Description was changed from

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Adds a DOMTokenList backed controlsList/controlslist attribute to
HTMLMediaElement with three keywords: nodownload, nofullscreen and
noremoteplayback.

Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/...

BUG=650174,685018
TEST=manual+layout tests
==========

to

==========
[Blink, Media] Added controlsList to HTMLMediaElement

Adds a DOMTokenList backed controlsList/controlslist attribute to
HTMLMediaElement with three keywords: nodownload, nofullscreen and
noremoteplayback.

Spec change is discussed here: https://github.com/whatwg/html/issues/2293
Spec change PR is here: https://github.com/whatwg/html/pull/2426
WICG repo for the API is here: https://github.com/WICG/controls-list
Intent to ship is here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/...

BUG=650174,685018
TEST=manual+layout tests

Review-Url: https://codereview.chromium.org/2657723002
Cr-Commit-Position: refs/heads/master@{#455926}
Committed:
https://chromium.googlesource.com/chromium/src/+/caca06866f37c79df89cab9a634b...
==========

[commit-bot: I haz the power, 2017-03-10 00:40:46]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/caca06866f37c79df89cab9a634b...