Adding overflow menu to media player

Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support additional actions and features.

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

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

[kdsilva, 2016-08-11 15:58:01]

Description was changed from

==========
Adding overflow menu

BUG=
==========

to

==========
Adding overflow menu

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

[kdsilva, 2016-08-19 14:35:29]

Description was changed from

==========
Adding overflow menu

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

to

==========
Adding overflow menu to media player

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

[kdsilva, 2016-08-19 14:39:48]

Description was changed from

==========
Adding overflow menu to media player

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

to

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

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

[kdsilva, 2016-08-19 15:34:40]

kdsilva@google.com changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org

[kdsilva, 2016-08-19 15:34:41]

PTAL. Tests are currently in the works :)

[whywhat, 2016-08-19 23:56:55]

Here's a first round of comments.
I'm not super confident in the media controls and their relationship to CSS
properties, you would probably want an extra review by liberato@ once we're done
with generic nits and so on.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
media-overflow-menu-button
why are there -internal and public keywords? is it related to whether the page
can access the element using css? if so, we probably want the overflow button to
be internal?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
ditto wrt internal
we should not add webkit prefixes to anything new AFAIK

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:90:
virtual String getText() { return ""; }
nit: return String();
document the method. what the text is for and what it represents

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

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:885: return
mediaElement().isPlayingRemotely() ? "Stop casting" : "Cast";
hm, it's not only "Stop casting", one could control the volume and potentially
do other stuff. Should we ask Rachel to update the mocks with a better wording?
ok to land like this (modulo i18n) but we probably want to correct the wording
later on.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:34: #include
<set>
nit: empty line between system and project includes; also I think the standard
includes precede the project ones.

#include <set>

#include "core/html/..."

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:98: String
getText() override;
This returns the user visible text, right? If so, the strings need to be
internationalized so that users get translated text if they're using Chrome with
a different language.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:146: String
getText() override { return "Captions"; }
I don't think we should inline string literals in headers - they might get
multiplied in the binaries making chrome bigger.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:182: class
MediaControlOverflowMenu final : public MediaControlInputElement {
nit: please add some comments to the new classes and at least public methods
where it makes sense

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:202: void
setOverflowMenuControls(std::set<MediaControlElementType>);
can you pass the set by (const) reference?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:241: String
getText() override { return "Full Screen"; }
nit: I believe Fullscreen is one word?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:267:
m_fullscreenOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
do we have these elements in the media element's dom tree all the time? I
thought we would add them in run-time when needed.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:719:
m_toggleClosedCaptionsButton.get(),
I thought this was fixed by another cl? Do you need to sync and rebase this cl?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:784: if
(overflowControls.size() >= 2 && !mediaElement().isFullscreen()) {
2 is a magic constant (which is not 0 or 1). please create a name for it and
define it somewhere (like at the top of the file in unnamed namespace)

is it safe to assume that in fullscreen (portrait mode) all the controls will
fit?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:840:
std::vector<MediaControlInputElement*> controlsListOverflow = {
m_muteOverflowButton, m_ClosedCaptionsOverflowButton, m_castOverflowButton,
m_fullscreenOverflowButton };
creating a new vector and passing it around seems wasteful. can this be const-ed
or cached somehow?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:94: // menu
nit: end sentences with a dot, please

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:158:
Member<MediaControlToggleClosedCaptionsButtonElement>
m_ClosedCaptionsOverflowButton;
nit: s/Closed/closed

[kdsilva, 2016-08-24 05:33:24]

kdsilva@google.com changed reviewers:
+ liberato@chromium.org

[kdsilva, 2016-08-24 05:33:26]

kdsilva@google.com changed reviewers:
+ liberato@chromium.org

[kdsilva, 2016-08-24 05:33:26]

kdsilva@google.com changed reviewers:
+ liberato@chromium.org

[kdsilva, 2016-08-24 05:33:26]

@liberato could you take a look at our approach to creating an overflow menu?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
media-overflow-menu-button
On 2016/08/19 23:56:55, whywhat wrote:
> why are there -internal and public keywords? is it related to whether the page
> can access the element using css? if so, we probably want the overflow button
to
> be internal?
It's not obvious to me what the naming convention is for this file. For the
media controls, I noticed that the all of the buttons in the media player were
public (mute button, play button, closed captions button, etc) so that was my
reasoning for making the overflow button public.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
On 2016/08/19 23:56:55, whywhat wrote:
> ditto wrt internal
> we should not add webkit prefixes to anything new AFAIK

Would you recommend changing the name to Blink? Part of me feels like it should
be webkit for consistency, but if there's new guidelines to avoid using the word
"webkit", I'm happy to change it!

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:90:
virtual String getText() { return ""; }
On 2016/08/19 23:56:55, whywhat wrote:
> nit: return String();
> document the method. what the text is for and what it represents

Done.

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

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:885: return
mediaElement().isPlayingRemotely() ? "Stop casting" : "Cast";
On 2016/08/19 23:56:55, whywhat wrote:
> hm, it's not only "Stop casting", one could control the volume and potentially
> do other stuff. Should we ask Rachel to update the mocks with a better
wording?
> ok to land like this (modulo i18n) but we probably want to correct the wording
> later on.

Acknowledged. I'll reach out to Rachel about this.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:34: #include
<set>
On 2016/08/19 23:56:55, whywhat wrote:
> nit: empty line between system and project includes; also I think the standard
> includes precede the project ones.
> 
> #include <set>
> 
> #include "core/html/..."

If I change the ordering, I get a presubmit error. Thoughts?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:98: String
getText() override;
On 2016/08/19 23:56:55, whywhat wrote:
> This returns the user visible text, right? If so, the strings need to be
> internationalized so that users get translated text if they're using Chrome
with
> a different language.

Done, I think... 
I followed what closed captions did, PTAL?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:146: String
getText() override { return "Captions"; }
On 2016/08/19 23:56:55, whywhat wrote:
> I don't think we should inline string literals in headers - they might get
> multiplied in the binaries making chrome bigger.

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:182: class
MediaControlOverflowMenu final : public MediaControlInputElement {
On 2016/08/19 23:56:55, whywhat wrote:
> nit: please add some comments to the new classes and at least public methods
> where it makes sense

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:202: void
setOverflowMenuControls(std::set<MediaControlElementType>);
On 2016/08/19 23:56:55, whywhat wrote:
> can you pass the set by (const) reference?

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:241: String
getText() override { return "Full Screen"; }
On 2016/08/19 23:56:55, whywhat wrote:
> nit: I believe Fullscreen is one word?

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:267:
m_fullscreenOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
On 2016/08/19 23:56:55, whywhat wrote:
> do we have these elements in the media element's dom tree all the time? I
> thought we would add them in run-time when needed.

liberato@, Anton mentioned you might have recommendations on when these media
elements should be created. PTAL?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:719:
m_toggleClosedCaptionsButton.get(),
On 2016/08/19 23:56:55, whywhat wrote:
> I thought this was fixed by another cl? Do you need to sync and rebase this
cl?

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:784: if
(overflowControls.size() >= 2 && !mediaElement().isFullscreen()) {
On 2016/08/19 23:56:55, whywhat wrote:
> 2 is a magic constant (which is not 0 or 1). please create a name for it and
> define it somewhere (like at the top of the file in unnamed namespace)
> 
> is it safe to assume that in fullscreen (portrait mode) all the controls will
> fit?

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:840:
std::vector<MediaControlInputElement*> controlsListOverflow = {
m_muteOverflowButton, m_ClosedCaptionsOverflowButton, m_castOverflowButton,
m_fullscreenOverflowButton };
On 2016/08/19 23:56:55, whywhat wrote:
> creating a new vector and passing it around seems wasteful. can this be
const-ed
> or cached somehow?

Done. I made controlsListOverflow a private member variable. I'm not sure if
that's the best approach. PTAL?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:94: // menu
On 2016/08/19 23:56:55, whywhat wrote:
> nit: end sentences with a dot, please

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:158:
Member<MediaControlToggleClosedCaptionsButtonElement>
m_ClosedCaptionsOverflowButton;
On 2016/08/19 23:56:55, whywhat wrote:
> nit: s/Closed/closed

Done.

[kdsilva, 2016-08-24 05:33:27]

@liberato could you take a look at our approach to creating an overflow menu?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:158:
Member<MediaControlToggleClosedCaptionsButtonElement>
m_ClosedCaptionsOverflowButton;
On 2016/08/19 23:56:55, whywhat wrote:
> nit: s/Closed/closed

Done.

[kdsilva, 2016-08-24 05:33:27]

@liberato could you take a look at our approach to creating an overflow menu?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:267:
m_fullscreenOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
On 2016/08/19 23:56:55, whywhat wrote:
> do we have these elements in the media element's dom tree all the time? I
> thought we would add them in run-time when needed.

liberato@, Anton mentioned you might have recommendations on when these media
elements should be created. PTAL?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:840:
std::vector<MediaControlInputElement*> controlsListOverflow = {
m_muteOverflowButton, m_ClosedCaptionsOverflowButton, m_castOverflowButton,
m_fullscreenOverflowButton };
On 2016/08/19 23:56:55, whywhat wrote:
> creating a new vector and passing it around seems wasteful. can this be
const-ed
> or cached somehow?

Done. I made controlsListOverflow a private member variable. I'm not sure if
that's the best approach. PTAL?

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:94: // menu
On 2016/08/19 23:56:55, whywhat wrote:
> nit: end sentences with a dot, please

Done.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:158:
Member<MediaControlToggleClosedCaptionsButtonElement>
m_ClosedCaptionsOverflowButton;
On 2016/08/19 23:56:55, whywhat wrote:
> nit: s/Closed/closed

Done.

[Timothy Loh, 2016-08-24 06:19:38]

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
media-overflow-menu-button
On 2016/08/24 05:33:25, kdsilva wrote:
> On 2016/08/19 23:56:55, whywhat wrote:
> > why are there -internal and public keywords? is it related to whether the
page
> > can access the element using css? if so, we probably want the overflow
button
> to
> > be internal?
> It's not obvious to me what the naming convention is for this file. For the
> media controls, I noticed that the all of the buttons in the media player were
> public (mute button, play button, closed captions button, etc) so that was my
> reasoning for making the overflow button public. 

We can't just go and add whatever we want as web-exposed features. Making this
-internal means that web devs can't set this value themselves.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
On 2016/08/24 05:33:25, kdsilva wrote:
> On 2016/08/19 23:56:55, whywhat wrote:
> > ditto wrt internal
> > we should not add webkit prefixes to anything new AFAIK
> 
> Would you recommend changing the name to Blink? Part of me feels like it
should
> be webkit for consistency, but if there's new guidelines to avoid using the
word
> "webkit", I'm happy to change it!

We are not adding new -webkit features (or -blink). See
http://www.chromium.org/blink#vendor-prefixes for out position on vendor
prefixes. This sort of thing is *web-exposed* which means that a web dev can
come along and style your menu button however they like. We don't want to be
adding such non-standard web-exposed features because they are an
interoperability issue between browsers and make it harder for us to make
changes to these features.

This should be -internal and not -webkit. I don't think we actually disallow
usage of -internal selectors right now, but at least compared to -webkit there
shouldn't be any sort of expectation of support.

[liberato (no reviews please), 2016-08-24 16:44:23]

really sorry for the delay.

overall looks like a great start.  just a few questions.

an overarching question is the position of the overflow menu with respect to the
video element.  what's the intended behavior when the element is near the top or
right of the page, and some of the overflow menu is off the screen?

also, how does one cause the menu to go away, especially if one doesn't want to
take any action?

last, if you're planning on adding more functionality over time, then you might
want to hide the overflow menu behind a runtime flag.  then, you don't have to
worry about it leaking out into the wild before you're ready.

thanks
-fl

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:267:
m_fullscreenOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
On 2016/08/24 05:33:27, kdsilva wrote:
> On 2016/08/19 23:56:55, whywhat wrote:
> > do we have these elements in the media element's dom tree all the time? I
> > thought we would add them in run-time when needed.
> 
> liberato@, Anton mentioned you might have recommendations on when these media
> elements should be created. PTAL?

i've had bad luck doing anything too complicated in ComputeWhichControlsFit --
it's called while blink is in some unexpected states.  Creating them up front
seems safer.

that said, it looks like the code is synamically creating new labels and a div
when constructing the overflow menu.  as it sits now, it's halfway between
dynamically allocated and preallocated.

i'd suggest either make it entirely one way or the other.

one might also hide a lot of this inside the MediaControlElement subclasses
themselves -- an element could equally well create its own overflow button. 
haven't thought it through, but it seems like most of the MediaControl code
could work entirely on the original panel elements, rather than maintaining both
lists.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
is -webkit intended?

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

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:307: return
mediaElement().locale().queryString(WebLocalizedString::OverflowMenuUnmute);
you might consider folding all of the queryString() into a non-virtual
MediaControElement::getText() fn.  the element subclasses can implement virtual
WebLocalizedString::Name getOverflowStringName(), called by getText.

saves some boiler plate code, and maybe makes it easier to see what's going on.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:611:
removeChildren(OmitSubtreeModifiedEvent);
if you choose to go the route of pre-allocating everything, then one could do
this ahead of time and "display: none" the elements that you don't want to show.
 that becomes simpler if one has a container div per overflow item that houses
both the label and the button.

if you don't go with pre-allocation, then nevermind :)

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:619: if
(controls->count(element->displayType())) {
if the original element handles providing its own overflow element, then this
becomes a little simpler.  MediaControls hands in the original elements that
didn't fit, then this function just asks each them for its overflow button. 
then, there's no need to do an inner traversal of the vector, nor assume
equality on displayType().

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:623:
item->appendChild(Text::create(document(), element->getText()));
does clicking on the label do anything?  is it supposed to?

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

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:183: class
MediaControlOverflowMenu final : public MediaControlInputElement {
might want to include "Button" in the class name.  that's consistent with the
other button classes.  plus, in this case, it avoids confusion with the
MediaControlOverflowMenuListElement.  "OverflowMenu" sounds like it could be the
latter.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:95:
std::vector<MediaControlInputElement*> getOverflowMenuButtons();
perhaps pass by ref to prevent a copy.  const ref would be okay in this case, i
think.

or, alternatively, since this is only used by the the overflow list element when
setting the overflow controls, just pass a const ref as an argument.  it's
already doing that for the list of controls that didn't fit; might as well just
send both lists.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLocalizedString.h (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLocalizedString.h:122:
OverflowMenuCaptions,
you might also want to add an accessibility string for the overflow button
itself.  this lets the screen reader understand what text to use for it.

search for "AXMediaPlayButton" for an example (AX == accessibility).  It starts
out as a web localized string in this file, just like you've added here.  I
don't remember all the steps to connecting it to the button so that the screen
reader finds it.  sorry about that.

[whywhat, 2016-08-24 18:39:17]

On 2016/08/24 at 06:19:38, timloh wrote:
>
https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):
> 
>
https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
media-overflow-menu-button
> On 2016/08/24 05:33:25, kdsilva wrote:
> > On 2016/08/19 23:56:55, whywhat wrote:
> > > why are there -internal and public keywords? is it related to whether the
page
> > > can access the element using css? if so, we probably want the overflow
button
> > to
> > > be internal?
> > It's not obvious to me what the naming convention is for this file. For the
> > media controls, I noticed that the all of the buttons in the media player
were
> > public (mute button, play button, closed captions button, etc) so that was
my
> > reasoning for making the overflow button public. 
> 
> We can't just go and add whatever we want as web-exposed features. Making this
-internal means that web devs can't set this value themselves.

We're all part of the web platform team, we know and follow the rules ;)
We don't know enough about the media controls and how these are exposed to the
web currently though.

> 
>
https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):
> 
>
https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
> On 2016/08/24 05:33:25, kdsilva wrote:
> > On 2016/08/19 23:56:55, whywhat wrote:
> > > ditto wrt internal
> > > we should not add webkit prefixes to anything new AFAIK
> > 
> > Would you recommend changing the name to Blink? Part of me feels like it
should
> > be webkit for consistency, but if there's new guidelines to avoid using the
word
> > "webkit", I'm happy to change it!
> 
> We are not adding new -webkit features (or -blink). See
http://www.chromium.org/blink#vendor-prefixes for out position on vendor
prefixes. This sort of thing is *web-exposed* which means that a web dev can
come along and style your menu button however they like. We don't want to be
adding such non-standard web-exposed features because they are an
interoperability issue between browsers and make it harder for us to make
changes to these features.
> 
> This should be -internal and not -webkit. I don't think we actually disallow
usage of -internal selectors right now, but at least compared to -webkit there
shouldn't be any sort of expectation of support.

Right, I was wondering if -internal is what makes it not web exposed. -webkit is
just historical artifacts.

We might want to expose these buttons later (following the Blink process) since
I don't think the websites customizing the buttons will like these extra buttons
that might look different. We might want the overflow menu buttons to follow the
same style as the corresponding buttons in the control bar.

[Timothy Loh, 2016-08-25 05:24:32]

On 2016/08/24 18:39:17, whywhat wrote:
> Right, I was wondering if -internal is what makes it not web exposed. -webkit
is
> just historical artifacts.

-internal should make things not web exposed, but this is only implemented for
values and not selectors (so -webkit-appearance: -internal-foo will not be
exposed, but for now video::-internal-foo will be exposed). We should prefer
-internal for both cases anyway. We'll actually remove -internal selectors from
platform visibility when we get a few spare cycles (we have counters for the
existing ones and usage is super low).

[mlamouri (slow - plz ping), 2016-08-26 17:07:38]

On 2016/08/25 at 05:24:32, timloh wrote:
> On 2016/08/24 18:39:17, whywhat wrote:
> > Right, I was wondering if -internal is what makes it not web exposed.
-webkit is
> > just historical artifacts.
> 
> -internal should make things not web exposed, but this is only implemented for
values and not selectors (so -webkit-appearance: -internal-foo will not be
exposed, but for now video::-internal-foo will be exposed). We should prefer
-internal for both cases anyway. We'll actually remove -internal selectors from
platform visibility when we get a few spare cycles (we have counters for the
existing ones and usage is super low).

FWIW, in the case of media controls, I would like consistency and we should
remove the -webkit pseudo elements at the same time as the -internals ones
become web-hidden. Otherwise, we might end up with websites looking fairly
weird.

[kdsilva, 2016-08-26 18:11:03]

Thank you all for the feedback! If I missed responding to a comment, let me know
:)

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
media-overflow-menu-button
On 2016/08/24 06:19:38, Timothy Loh wrote:
> On 2016/08/24 05:33:25, kdsilva wrote:
> > On 2016/08/19 23:56:55, whywhat wrote:
> > > why are there -internal and public keywords? is it related to whether the
> page
> > > can access the element using css? if so, we probably want the overflow
> button
> > to
> > > be internal?
> > It's not obvious to me what the naming convention is for this file. For the
> > media controls, I noticed that the all of the buttons in the media player
were
> > public (mute button, play button, closed captions button, etc) so that was
my
> > reasoning for making the overflow button public. 
> 
> We can't just go and add whatever we want as web-exposed features. Making this
> -internal means that web devs can't set this value themselves.

Acknowledged.

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
On 2016/08/24 06:19:38, Timothy Loh wrote:
> On 2016/08/24 05:33:25, kdsilva wrote:
> > On 2016/08/19 23:56:55, whywhat wrote:
> > > ditto wrt internal
> > > we should not add webkit prefixes to anything new AFAIK
> > 
> > Would you recommend changing the name to Blink? Part of me feels like it
> should
> > be webkit for consistency, but if there's new guidelines to avoid using the
> word
> > "webkit", I'm happy to change it!
> 
> We are not adding new -webkit features (or -blink). See
> http://www.chromium.org/blink#vendor-prefixes for out position on vendor
> prefixes. This sort of thing is *web-exposed* which means that a web dev can
> come along and style your menu button however they like. We don't want to be
> adding such non-standard web-exposed features because they are an
> interoperability issue between browsers and make it harder for us to make
> changes to these features.
> 
> This should be -internal and not -webkit. I don't think we actually disallow
> usage of -internal selectors right now, but at least compared to -webkit there
> shouldn't be any sort of expectation of support.

Got it, that makes sense. Thanks for the details!

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-webkit-overflow-menu-button {
On 2016/08/24 16:44:22, liberato wrote:
> is -webkit intended?

Changed to -internal.

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

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:307: return
mediaElement().locale().queryString(WebLocalizedString::OverflowMenuUnmute);
Done. Although I'm not sure what the default return value should be for the
getOverflowStringName() function since not all subclasses don't need to have an
overflow string name. Any ideas?

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:611:
removeChildren(OmitSubtreeModifiedEvent);
On 2016/08/24 16:44:22, liberato wrote:
> if you choose to go the route of pre-allocating everything, then one could do
> this ahead of time and "display: none" the elements that you don't want to
show.
>  that becomes simpler if one has a container div per overflow item that houses
> both the label and the button.
> 
> if you don't go with pre-allocation, then nevermind :)

Done. Although I couldn't find a simple of changing the "display" attribute.
PTAL?

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:619: if
(controls->count(element->displayType())) {
On 2016/08/24 16:44:22, liberato wrote:
> if the original element handles providing its own overflow element, then this
> becomes a little simpler.  MediaControls hands in the original elements that
> didn't fit, then this function just asks each them for its overflow button. 
> then, there's no need to do an inner traversal of the vector, nor assume
> equality on displayType().

Done.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:623:
item->appendChild(Text::create(document(), element->getText()));
On 2016/08/24 16:44:22, liberato wrote:
> does clicking on the label do anything?  is it supposed to?

Clicking on the label should perform the action the button is associated with.
Ideally, here, we wouldn't be appending two children but I'm not sure what a
better way would be to have a text show up here in lieu of the button icon.

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

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:183: class
MediaControlOverflowMenu final : public MediaControlInputElement {
On 2016/08/24 16:44:22, liberato wrote:
> might want to include "Button" in the class name.  that's consistent with the
> other button classes.  plus, in this case, it avoids confusion with the
> MediaControlOverflowMenuListElement.  "OverflowMenu" sounds like it could be
the
> latter.

Done.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:95:
std::vector<MediaControlInputElement*> getOverflowMenuButtons();
On 2016/08/24 16:44:22, liberato wrote:
> perhaps pass by ref to prevent a copy.  const ref would be okay in this case,
i
> think.
> 
> or, alternatively, since this is only used by the the overflow list element
when
> setting the overflow controls, just pass a const ref as an argument.  it's
> already doing that for the list of controls that didn't fit; might as well
just
> send both lists.

Acknowledged.

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLocalizedString.h (right):

https://codereview.chromium.org/2243473002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLocalizedString.h:122:
OverflowMenuCaptions,
On 2016/08/24 16:44:23, liberato wrote:
> you might also want to add an accessibility string for the overflow button
> itself.  this lets the screen reader understand what text to use for it.
> 
> search for "AXMediaPlayButton" for an example (AX == accessibility).  It
starts
> out as a web localized string in this file, just like you've added here.  I
> don't remember all the steps to connecting it to the button so that the screen
> reader finds it.  sorry about that.

Done.

[whywhat, 2016-08-26 20:57:26]

lgtm modulo Frank's approval

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:34: #include
<set>
On 2016/08/24 at 05:33:26, kdsilva wrote:
> On 2016/08/19 23:56:55, whywhat wrote:
> > nit: empty line between system and project includes; also I think the
standard
> > includes precede the project ones.
> > 
> > #include <set>
> > 
> > #include "core/html/..."
> 
> If I change the ordering, I get a presubmit error. Thoughts?

Ok, presubmit check is the ultimate source of truth then :)

https://codereview.chromium.org/2243473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:98: String
getText() override;
On 2016/08/24 at 05:33:26, kdsilva wrote:
> On 2016/08/19 23:56:55, whywhat wrote:
> > This returns the user visible text, right? If so, the strings need to be
> > internationalized so that users get translated text if they're using Chrome
with
> > a different language.
> 
> Done, I think... 
> I followed what closed captions did, PTAL?

looks good

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:292: 
ditto

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (left):

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:187: 
nit: unintended delete?

[kdsilva, 2016-08-27 14:20:50]

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:292: 
On 2016/08/26 20:57:26, whywhat wrote:
> ditto

Done.

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (left):

https://codereview.chromium.org/2243473002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:187: 
On 2016/08/26 20:57:26, whywhat wrote:
> nit: unintended delete?

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:127: if
(shouldShow)
It's not clear to me what the best way is to change the visibility is of an
Element. Changing the CSS like this seems to work, but I would think there'd be
a better way to only change the visibility attribute.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:109:
virtual WebLocalizedString::Name getOverflowStringName() { return
WebLocalizedString::AXAMPMFieldText; }
I'm not sure what the default return value should be for the
getOverflowStringName() function since not all subclasses don't need to have an
overflow string name. Any ideas?

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

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:258:
std::vector<MediaControlInputElement*> buttons = { m_toggleClosedCaptionsButton,
Ideally here, we’d iterate through all of the children of `panel` and check
which ones have hasOverflowButton() return true rather than create an array. I
don’t know what the “best” way is to do that. It looks like ContainerNode.h has
a .children() method, but that returns a HTMLCollection* which isn’t exactly
what we want. Any suggestions?

[kdsilva, 2016-08-29 14:33:15]

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:127: if
(shouldShow)
On 2016/08/27 14:20:49, kdsilva wrote:
> It's not clear to me what the best way is to change the visibility is of an
> Element. Changing the CSS like this seems to work, but I would think there'd
be
> a better way to only change the visibility attribute.
Hmm, so I think a cleaner approach might be to do
element->removeInlineStyleProperty() and element->setInlineStyleProperty(). If
that sounds reasonable to you, I'll update the code to use those methods
instead!

[liberato (no reviews please), 2016-08-29 17:38:39]

looks good so far.  i'm still going over this, but wanted to send out what i've
got so far.

thanks
-fl

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:127: if
(shouldShow)
On 2016/08/29 14:33:15, kdsilva wrote:
> On 2016/08/27 14:20:49, kdsilva wrote:
> > It's not clear to me what the best way is to change the visibility is of an
> > Element. Changing the CSS like this seems to work, but I would think there'd
> be
> > a better way to only change the visibility attribute.
> Hmm, so I think a cleaner approach might be to do
> element->removeInlineStyleProperty() and element->setInlineStyleProperty(). If
> that sounds reasonable to you, I'll update the code to use those methods
> instead! 

i think that the inline style property is the way to go.  that's what current
panel does.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:133:
String MediaControlElement::getText()
maybe getOverflowString?

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:102:
virtual Element* initOverflowButton(MediaControls&) { return nullptr; }
naming on these is a little confusing.  initOverflowButton calls
createOverflowElement.

personally, i think that "button" is the wrong name now, since it creates a
button and a label.

aha!  how about init*Button does create the button, but nothing else?

as in, it might be possible make createOverflowElement be the externally visible
function.  internally, it calls getText() and createOverflowButton(), which are
provided by the subclass.

that should remove some more duplicated code in the init*Button() methods.

it's still a little weird that we have a member function that acts like (and
uses) a static factory method to do the work.  would be nice to fix that, but i
don't see how to do it without template member functions.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:109:
virtual WebLocalizedString::Name getOverflowStringName() { return
WebLocalizedString::AXAMPMFieldText; }
On 2016/08/27 14:20:49, kdsilva wrote:
> I'm not sure what the default return value should be for the
> getOverflowStringName() function since not all subclasses don't need to have
an
> overflow string name. Any ideas?

nope.  :)

pick one, and NOTREACHED()

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:125: //
Represents the overflow menu element for this media control.
might want to mention that it's optional, and might be null.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:164:
HTMLLabelElement* createOverflowMenuElement(MediaControls&,
MediaControlInputElement*);
can this return HTMLElement?

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

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:315:
button->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
this and setIsWanted seem to be everywhere -- can they be moved into
createOverfloeMenuElement too?  then there's very little left here.

please see my comment elsewhere about flipping this over so that this function
just creates the button, then it's a one-liner with no duplicate code.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:839:
m_overflowMenuElement = createOverflowMenuElement(mediaControls, button);
can createoverflowMenuElement set |m_overflowMenuElement| too?

[kdsilva, 2016-08-29 20:44:14]

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-internal-overflow-menu-button {
I followed the way the closed captions button was styled but looking at this
again, I also need to think about audio only. Is there a smarter way of having
this styling used for audio too other than creating all of these again for
audio::?

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:127: if
(shouldShow)
On 2016/08/29 17:38:38, liberato wrote:
> On 2016/08/29 14:33:15, kdsilva wrote:
> > On 2016/08/27 14:20:49, kdsilva wrote:
> > > It's not clear to me what the best way is to change the visibility is of
an
> > > Element. Changing the CSS like this seems to work, but I would think
there'd
> > be
> > > a better way to only change the visibility attribute.
> > Hmm, so I think a cleaner approach might be to do
> > element->removeInlineStyleProperty() and element->setInlineStyleProperty().
If
> > that sounds reasonable to you, I'll update the code to use those methods
> > instead! 
> 
> i think that the inline style property is the way to go.  that's what current
> panel does.

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:133:
String MediaControlElement::getText()
On 2016/08/29 17:38:38, liberato wrote:
> maybe getOverflowString?

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:102:
virtual Element* initOverflowButton(MediaControls&) { return nullptr; }
On 2016/08/29 17:38:38, liberato wrote:
> naming on these is a little confusing.  initOverflowButton calls
> createOverflowElement.
> 
> personally, i think that "button" is the wrong name now, since it creates a
> button and a label.
> 
> aha!  how about init*Button does create the button, but nothing else?
> 
> as in, it might be possible make createOverflowElement be the externally
visible
> function.  internally, it calls getText() and createOverflowButton(), which
are
> provided by the subclass.
> 
> that should remove some more duplicated code in the init*Button() methods.
> 
> it's still a little weird that we have a member function that acts like (and
> uses) a static factory method to do the work.  would be nice to fix that, but
i
> don't see how to do it without template member functions.

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:109:
virtual WebLocalizedString::Name getOverflowStringName() { return
WebLocalizedString::AXAMPMFieldText; }
On 2016/08/29 17:38:38, liberato wrote:
> On 2016/08/27 14:20:49, kdsilva wrote:
> > I'm not sure what the default return value should be for the
> > getOverflowStringName() function since not all subclasses don't need to have
> an
> > overflow string name. Any ideas?
> 
> nope.  :)
> 
> pick one, and NOTREACHED()

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:125: //
Represents the overflow menu element for this media control.
On 2016/08/29 17:38:38, liberato wrote:
> might want to mention that it's optional, and might be null.

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:164:
HTMLLabelElement* createOverflowMenuElement(MediaControls&,
MediaControlInputElement*);
On 2016/08/29 17:38:38, liberato wrote:
> can this return HTMLElement?

Done. Is it better to return HTMLElement since it's more generic?

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

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:315:
button->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
On 2016/08/29 17:38:38, liberato wrote:
> this and setIsWanted seem to be everywhere -- can they be moved into
> createOverfloeMenuElement too?  then there's very little left here.
> 
> please see my comment elsewhere about flipping this over so that this function
> just creates the button, then it's a one-liner with no duplicate code.

Done.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:839:
m_overflowMenuElement = createOverflowMenuElement(mediaControls, button);
On 2016/08/29 17:38:38, liberato wrote:
> can createoverflowMenuElement set |m_overflowMenuElement| too?

Done.

[liberato (no reviews please), 2016-08-30 16:42:07]

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:421:
video::-internal-overflow-menu-button {
On 2016/08/29 20:44:13, kdsilva wrote:
> I followed the way the closed captions button was styled but looking at this
> again, I also need to think about audio only. Is there a smarter way of having
> this styling used for audio too other than creating all of these again for
> audio::?
> 

i think that most of the entries are of the form

video::-internal-something, audio::-internal-something {
...
}

so, one doesn't need to duplicate the entire rule, just add a selector.

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:164:
HTMLLabelElement* createOverflowMenuElement(MediaControls&,
MediaControlInputElement*);
On 2016/08/29 20:44:13, kdsilva wrote:
> On 2016/08/29 17:38:38, liberato wrote:
> > can this return HTMLElement?
> 
> Done. Is it better to return HTMLElement since it's more generic?

in this case, i think so.  the fact that it's a label seems like an
implementation detail of create...Element().  it's believable that we'd want to
change the structure of the overflow elements in the future.

"more generic" isn't always better (e.g., "Node").  just seems like a better
match for the guarantees that this function is trying to provide.

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

https://codereview.chromium.org/2243473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:258:
std::vector<MediaControlInputElement*> buttons = { m_toggleClosedCaptionsButton,
On 2016/08/27 14:20:50, kdsilva wrote:
> Ideally here, we’d iterate through all of the children of `panel` and check
> which ones have hasOverflowButton() return true rather than create an array. I
> don’t know what the “best” way is to do that. It looks like ContainerNode.h
has
> a .children() method, but that returns a HTMLCollection* which isn’t exactly
> what we want. Any suggestions?

please see the comment by fs in https://codereview.chromium.org/2235473002/ for
a possible approach.  don't know if it's exactly what you want or not.

the problem is that iterating then adding the items doesn't let you control the
order in which they show up in the overflow menu.  unless it's the same as the
panel order, then i'm not sure that you can get away from having at least one
array with them in the proper (UX) order.

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:175:
MediaControlInputElement* button = createOverflowButton(mediaControls);
should this return null if |button| is null?  either that, or CHECK.  depends on
how you plan to use it.

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:179:
HTMLLabelElement* element = HTMLLabelElement::create(mediaControls.document());
is clicking on the label supposed to trigger the action?

if i commented on this before, sorry for the duplicate.  i remember writing it,
but i don't see it in the comments now.

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

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:314: return
MediaControlMuteButtonElement::create(mediaControls);
now that we've successfully reduced this to one line, i ahve to say that having
a method that trampolines to a static factory is kinda odd.  it would be nice to
remove it somehow.  however, i still don't see how unless we move ::create
inline when creating the overflow menu in MediaControls.

if you end up having to explicitly create them to get the order right, then
maybe it's worth doing it that way.

[whywhat, 2016-08-30 20:22:05]

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:179:
HTMLLabelElement* element = HTMLLabelElement::create(mediaControls.document());
On 2016/08/30 at 16:42:07, liberato wrote:
> is clicking on the label supposed to trigger the action?
> 
> if i commented on this before, sorry for the duplicate.  i remember writing
it, but i don't see it in the comments now.

This seems to be by the spec of HTMLLabelElement (and its implementation)
clicking on the label clicks on its related form element which is defined either
by the "for=" attribute of the label or found in descendants.

This behavior seems to be covered well by <label> layout tests.

[liberato (no reviews please), 2016-08-30 20:59:39]

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:179:
HTMLLabelElement* element = HTMLLabelElement::create(mediaControls.document());
On 2016/08/30 20:22:05, whywhat wrote:
> On 2016/08/30 at 16:42:07, liberato wrote:
> > is clicking on the label supposed to trigger the action?
> > 
> > if i commented on this before, sorry for the duplicate.  i remember writing
> it, but i don't see it in the comments now.
> 
> This seems to be by the spec of HTMLLabelElement (and its implementation)
> clicking on the label clicks on its related form element which is defined
either
> by the "for=" attribute of the label or found in descendants.
> 
> This behavior seems to be covered well by <label> layout tests.

i had no idea that it did that.

thanks!

[kdsilva, 2016-08-31 00:17:05]

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:175:
MediaControlInputElement* button = createOverflowButton(mediaControls);
On 2016/08/30 16:42:07, liberato wrote:
> should this return null if |button| is null?  either that, or CHECK.  depends
on
> how you plan to use it.

Done.

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

https://codereview.chromium.org/2243473002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:314: return
MediaControlMuteButtonElement::create(mediaControls);
On 2016/08/30 16:42:07, liberato wrote:
> now that we've successfully reduced this to one line, i ahve to say that
having
> a method that trampolines to a static factory is kinda odd.  it would be nice
to
> remove it somehow.  however, i still don't see how unless we move ::create
> inline when creating the overflow menu in MediaControls.
> 
> if you end up having to explicitly create them to get the order right, then
> maybe it's worth doing it that way.

Done.

[mlamouri (slow - plz ping), 2016-08-31 23:53:40]

Description was changed from

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

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

to

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

BUG=601247
==========

[mlamouri (slow - plz ping), 2016-08-31 23:54:01]

I've updated the bug to be 601247. It was linked to the flash rewrite launch
bug.

[liberato (no reviews please), 2016-09-01 18:05:19]

a few minor comments only.

lgtm.  nice work.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:175: void
updateOverflowString();
might fit better in MediaControlElement, since that's where
getOverflowStringName is.

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

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:303:
updateOverflowString();
would it make sense to call this in setShouldShowButtonInOverflowMenu()?

not sure if it's functionally equivalent, but i also don't know that the
overflow menu is going to change much while it's shown.  i suppose that the
code, as written, would change 'play' to 'pause' if the video ended while the
overflow was shown.

whichever you think is best.

[mlamouri (slow - plz ping), 2016-09-01 18:29:15]

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
content/app/strings/content_strings.grd:862: <message
name="IDS_MEDIA_OVERFLOW_MENU_CAPTIONS" desc="Media controls overflow menu item
label for a closed captions button.">
Should this be called IDS_MEDIA_OVERFLOW_MENU_CLOSED_CAPTIONS?

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
content/app/strings/content_strings.grd:863: Captions
I guess "Captions" is matching the UI specs? Maybe the <message> should point
that the text should be short?

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/display-overflow-menu.html (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/display-overflow-menu.html:50:
test(function() {
I'm not sure you can do that (creating a sync test inside an async one). You
might want to have multiple async test. I would recommend having multiple test
files.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
-internal-overflow-menu-button
I might be missing something but why isn't
internal-media-controls-overflow-menu-list in there?

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:436:
video::-internal-media-controls-overflow-menu-list {
Can you merge this with the closed captions one instead of creating a new rule?

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:105:
String getOverflowString();
`getOverflowMenuString()` or `getOverflowMenuLabel()`?

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:127:
Member<Text> m_overflowText;
Should this be named `m_overflowMenuText`?

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

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:423: if
(mediaControls().overflowMenuVisible())
Can you add a comment explaining why you are doing this. At first, I thought of
a mistake then realised what you are doing and I have some background about your
work :). Others might be confused.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:103:
MediaControlInputElement* createOverflowButton(MediaControls&);
Do you actually need this? I can't find where it is used.

Applies for the ones below.

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

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:454: 
nit: remove this unless it was intentional?

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:773:
element->shouldShowButtonInOverflowMenu(false);
Shouldn't that be set when `isWanted()` is set to false? sounds like it would be
easy to forget otherwise. Alternatively, we could just move this call early in
the for loop and only set to true if needed.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:780:
m_overflowMenu->setIsWanted(true);
I'm confused. How do we know that m_overflowMenu would fit?

[kdsilva, 2016-09-05 14:25:59]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[kdsilva, 2016-09-05 14:26:00]

Description was changed from

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

BUG=601247
==========

to

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

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

[commit-bot: I haz the power, 2016-09-05 14:26:07]

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

[kdsilva, 2016-09-05 14:39:35]

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
content/app/strings/content_strings.grd:862: <message
name="IDS_MEDIA_OVERFLOW_MENU_CAPTIONS" desc="Media controls overflow menu item
label for a closed captions button.">
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Should this be called IDS_MEDIA_OVERFLOW_MENU_CLOSED_CAPTIONS?

Done.

https://codereview.chromium.org/2243473002/diff/300001/content/app/strings/co...
content/app/strings/content_strings.grd:863: Captions
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> I guess "Captions" is matching the UI specs? Maybe the <message> should point
> that the text should be short?

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/display-overflow-menu.html (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/display-overflow-menu.html:50:
test(function() {
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> I'm not sure you can do that (creating a sync test inside an async one). You
> might want to have multiple async test. I would recommend having multiple test
> files.

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:650:
-internal-overflow-menu-button
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> I might be missing something but why isn't
> internal-media-controls-overflow-menu-list in there?

My reasoning was that the closed captions implementation hadn't added their menu
to this list. I'll reach out to someone and confirm that what we've added here
is okay.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:436:
video::-internal-media-controls-overflow-menu-list {
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Can you merge this with the closed captions one instead of creating a new
rule?

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
(right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:105:
String getOverflowString();
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> `getOverflowMenuString()` or `getOverflowMenuLabel()`?

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:127:
Member<Text> m_overflowText;
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Should this be named `m_overflowMenuText`? 

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:175: void
updateOverflowString();
On 2016/09/01 18:05:19, liberato (ooo) wrote:
> might fit better in MediaControlElement, since that's where
> getOverflowStringName is.

Done.

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

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:303:
updateOverflowString();
On 2016/09/01 18:05:19, liberato (ooo) wrote:
> would it make sense to call this in setShouldShowButtonInOverflowMenu()?
> 
> not sure if it's functionally equivalent, but i also don't know that the
> overflow menu is going to change much while it's shown.  i suppose that the
> code, as written, would change 'play' to 'pause' if the video ended while the
> overflow was shown.
> 
> whichever you think is best.

That works much better!

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:423: if
(mediaControls().overflowMenuVisible())
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Can you add a comment explaining why you are doing this. At first, I thought
of
> a mistake then realised what you are doing and I have some background about
your
> work :). Others might be confused.

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:103:
MediaControlInputElement* createOverflowButton(MediaControls&);
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Do you actually need this? I can't find where it is used.
> 
> Applies for the ones below.

Done.

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

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:454: 
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> nit: remove this unless it was intentional?

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:773:
element->shouldShowButtonInOverflowMenu(false);
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> Shouldn't that be set when `isWanted()` is set to false? sounds like it would
be
> easy to forget otherwise. Alternatively, we could just move this call early in
> the for loop and only set to true if needed.

Done.

https://codereview.chromium.org/2243473002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:780:
m_overflowMenu->setIsWanted(true);
On 2016/09/01 18:29:15, mlamouri (slow) wrote:
> I'm confused. How do we know that m_overflowMenu would fit?

Hmm, I lost that logic when changing the implementation earlier. I've updated
the code to include the logic although if you have suggestions about other ways
to go about it, let me know.

[commit-bot: I haz the power, 2016-09-05 15:57:57]

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

[commit-bot: I haz the power, 2016-09-05 15:57:58]

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

[kdsilva, 2016-09-05 16:28:11]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 16:28:18]

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

[kdsilva, 2016-09-05 17:31:48]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 17:31:56]

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-09-05 18:30:11]

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

[commit-bot: I haz the power, 2016-09-05 18:30:13]

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), 2016-09-06 10:25:46]

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/overflow-menu.js (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/overflow-menu.js:2: var overflowButtonsCSS
= ["-webkit-media-controls-mute-button",
nit: for readability, I would break the line after [

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-closed-captions-button.html
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-closed-captions-button.html:10:
<body style="padding: 300px">
If you use padding-top, it would be clearer (same for the tests below).

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html:30:
video.addEventListener("webkitfullscreenchange", t.step_func_done());
We don't have a unprefixed version?

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:444: background-color:
#e0e0e0;
Maybe this could be merged with
`video::-internal-media-controls-text-track-list-item:hover`?

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:141:
m_overflowMenuText->replaceWholeText(getOverflowMenuString());
I'm a bit worried because we are making DOM manipulations every time
`computeWhichControlsFit` (ie. this is called by
`shouldShowButtonInOverflowMenu()`).

Can you call this in `updateDisplayType()` instead? You could add a DCHECK() in
`shouldShowButtonInOverflowMenu()` that verifies that the string returned by
`getOverflowMenuString()` matches the one currently showing.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:183: //
We don't want the button visible within the overflow menu.
style: empty line before the comment.

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

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:621:
setIsWanted(visible);
nit: I'm not entirely convinced we need to wrap setIsWanted() into
showOverflowMenu(). If this is a leftover from another design, maybe you could
consider removing. If you think it makes it easier to read, feel free to leave.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:819: return
WebLocalizedString::OverflowMenuFullscreen;
Should there be an enter and exit fullscreen version?

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:35: #include
<set>
Is this #include a leftover from previous iterations?

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

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:737: }
Why is this needed?

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:765: }
Do you have a test for this?

I would recommend something like this:
```
m_overflowMenu->setDoesFit(true);
m_overflowMenu->setIsWanted(true);
usedWidth = minimumWidth;

// Go trough the algorithm that updated the used width below.
// Count how many elements end up in the overflow menu.
// If 0:
m_overflowMenu->setIsWanted(false);
usedWidth -= minimumWidth;
// If 1:
m_overflowMenu->setIsWanted(false);
theElementInOverflowMenu->setIsWanted(true);
// Otherwise: do nothing.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:788:
element->shouldShowButtonInOverflowMenu(false);
I don't think you need this.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:95:
std::vector<MediaControlInputElement*> getOverflowMenuButtons();
That's never defined it seems.

[kdsilva, 2016-09-06 17:32:46]

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/overflow-menu.js (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/overflow-menu.js:2: var overflowButtonsCSS
= ["-webkit-media-controls-mute-button",
On 2016/09/06 10:25:44, mlamouri (slow) wrote:
> nit: for readability, I would break the line after [

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-closed-captions-button.html
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-closed-captions-button.html:10:
<body style="padding: 300px">
On 2016/09/06 10:25:44, mlamouri (slow) wrote:
> If you use padding-top, it would be clearer (same for the tests below).

Done. Although it looks like we'd need to have it a tad to the left so that it
doesn't end up off the screen.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html:30:
video.addEventListener("webkitfullscreenchange", t.step_func_done());
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> We don't have a unprefixed version?

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:444: background-color:
#e0e0e0;
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> Maybe this could be merged with
> `video::-internal-media-controls-text-track-list-item:hover`?

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:141:
m_overflowMenuText->replaceWholeText(getOverflowMenuString());
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> I'm a bit worried because we are making DOM manipulations every time
> `computeWhichControlsFit` (ie. this is called by
> `shouldShowButtonInOverflowMenu()`).
> 
> Can you call this in `updateDisplayType()` instead? You could add a DCHECK()
in
> `shouldShowButtonInOverflowMenu()` that verifies that the string returned by
> `getOverflowMenuString()` matches the one currently showing.

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:183: //
We don't want the button visible within the overflow menu.
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> style: empty line before the comment.

Done.

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

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:621:
setIsWanted(visible);
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> nit: I'm not entirely convinced we need to wrap setIsWanted() into
> showOverflowMenu(). If this is a leftover from another design, maybe you could
> consider removing. If you think it makes it easier to read, feel free to
leave.

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:819: return
WebLocalizedString::OverflowMenuFullscreen;
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> Should there be an enter and exit fullscreen version?

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:35: #include
<set>
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> Is this #include a leftover from previous iterations?

Done. Looks like that rebase unintendedly added in a line or two...

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

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:737: }
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> Why is this needed?

Acknowledged.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:765: }
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> Do you have a test for this?
> 
> I would recommend something like this:
> ```
> m_overflowMenu->setDoesFit(true);
> m_overflowMenu->setIsWanted(true);
> usedWidth = minimumWidth;
> 
> // Go trough the algorithm that updated the used width below.
> // Count how many elements end up in the overflow menu.
> // If 0:
> m_overflowMenu->setIsWanted(false);
> usedWidth -= minimumWidth;
> // If 1:
> m_overflowMenu->setIsWanted(false);
> theElementInOverflowMenu->setIsWanted(true);
> // Otherwise: do nothing.

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:788:
element->shouldShowButtonInOverflowMenu(false);
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> I don't think you need this.

Done.

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2243473002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:95:
std::vector<MediaControlInputElement*> getOverflowMenuButtons();
On 2016/09/06 10:25:45, mlamouri (slow) wrote:
> That's never defined it seems.

Done.

[mlamouri (slow - plz ping), 2016-09-07 09:07:55]

lgtm with comments applied

https://codereview.chromium.org/2243473002/diff/440001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/2243473002/diff/440001/content/app/strings/co...
content/app/strings/content_strings.grd:868: <message
name="IDS_MEDIA_OVERFLOW_MENU_FULLSCREEN" desc="Media controls overflow menu
item label for a fullscreen button.">
ENTER_FULLSCREEN -- update the description accordingly.

https://codereview.chromium.org/2243473002/diff/440001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2243473002/diff/440001/content/child/blink_pl...
content/child/blink_platform_impl.cc:255: case
WebLocalizedString::OverflowMenuFullscreen:
::OverflowMenuEnterFullscreen.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html
(right):

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html:30:
video.onwebkitfullscreenchange = t.step_func(function() {
This is still using the prefixed version. Can you do:
```
document.onfullscreenchange = t.step_func_done(() => {
  assert_equals(document.fullscreenElement, video);
  assert_equals(getComputedStyle(overflowMenu).display, "none");
});
```

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:126:
DCHECK(m_overflowMenuText->wholeText() == getOverflowMenuString());
nit: `DCHECK_EQ(getOverflowMenuString(), m_overflowMenuText->wholeText());`

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

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:756:
MediaControlElement* overflowElementDropped = nullptr;
May I suggest using a std::list<> instead of keeping a pointer and a counter?

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:774:
overflowElementDropped = element;
Here, you can do: `overflowElements.append(element);`

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:782: if
(numOverflowElements == 0) {
`if (overflowElements.empty())`

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:785: } else if
(numOverflowElements == 1) {
`if (overflowElements.size() == 1)`

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:788:
overflowElementDropped->setDoesFit(true);
You no longer need the null check and can do something like
`overflowElements.begin()->setDoesFit(true);` or whatever is valid std C++ :)

[kdsilva, 2016-09-07 11:01:08]

kdsilva@google.com changed reviewers:
+ clamy@chromium.org, foolip@chromium.org

[kdsilva, 2016-09-07 11:01:10]

@clamy: Could you rubber stamp the content code?
@foolip: Could you rubber stamp the Webkit code?

https://codereview.chromium.org/2243473002/diff/440001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/2243473002/diff/440001/content/app/strings/co...
content/app/strings/content_strings.grd:868: <message
name="IDS_MEDIA_OVERFLOW_MENU_FULLSCREEN" desc="Media controls overflow menu
item label for a fullscreen button.">
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> ENTER_FULLSCREEN -- update the description accordingly.

Done.

https://codereview.chromium.org/2243473002/diff/440001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2243473002/diff/440001/content/child/blink_pl...
content/child/blink_platform_impl.cc:255: case
WebLocalizedString::OverflowMenuFullscreen:
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> ::OverflowMenuEnterFullscreen.

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html
(right):

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-fullscreen-button.html:30:
video.onwebkitfullscreenchange = t.step_func(function() {
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> This is still using the prefixed version. Can you do:
> ```
> document.onfullscreenchange = t.step_func_done(() => {
>   assert_equals(document.fullscreenElement, video);
>   assert_equals(getComputedStyle(overflowMenu).display, "none");
> });
> ```

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:126:
DCHECK(m_overflowMenuText->wholeText() == getOverflowMenuString());
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> nit: `DCHECK_EQ(getOverflowMenuString(), m_overflowMenuText->wholeText());`

Done.

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

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:756:
MediaControlElement* overflowElementDropped = nullptr;
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> May I suggest using a std::list<> instead of keeping a pointer and a counter?

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:774:
overflowElementDropped = element;
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> Here, you can do: `overflowElements.append(element);`

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:782: if
(numOverflowElements == 0) {
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> `if (overflowElements.empty())`

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:785: } else if
(numOverflowElements == 1) {
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> `if (overflowElements.size() == 1)`

Done.

https://codereview.chromium.org/2243473002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:788:
overflowElementDropped->setDoesFit(true);
On 2016/09/07 09:07:54, mlamouri (slow) wrote:
> You no longer need the null check and can do something like
> `overflowElements.begin()->setDoesFit(true);` or whatever is valid std C++ :)

Done.

[kdsilva, 2016-09-07 11:01:16]

@clamy: Could you rubber stamp the content code?
@foolip: Could you rubber stamp the Webkit code?

[clamy, 2016-09-07 14:05:15]

content/ lgtm

[foolip, 2016-09-07 19:25:08]

rs lgtm, I took a very quick look and it looks like a MediaControls change
usually does :)

[mlamouri (slow - plz ping), 2016-09-08 09:59:40]

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

[commit-bot: I haz the power, 2016-09-08 09:59:53]

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-09-08 10:02:41]

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

[commit-bot: I haz the power, 2016-09-08 10:02:42]

Dry run: 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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kdsilva, 2016-09-08 11:28:24]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 11:28:47]

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-09-08 12:52:15]

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

[commit-bot: I haz the power, 2016-09-08 12:52:16]

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

[kdsilva, 2016-09-08 13:43:43]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[kdsilva, 2016-09-08 13:44:01]

The CQ bit was unchecked by kdsilva@google.com

[commit-bot: I haz the power, 2016-09-08 13:44:03]

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

[kdsilva, 2016-09-08 14:56:32]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 14:56:54]

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

[kdsilva, 2016-09-08 14:57:18]

The CQ bit was unchecked by kdsilva@google.com

[kdsilva, 2016-09-08 14:58:07]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 14:58:22]

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-09-08 16:27:06]

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

[commit-bot: I haz the power, 2016-09-08 16:27:07]

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

[kdsilva, 2016-09-09 10:53:24]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 10:53: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, 2016-09-09 12:26:27]

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

[commit-bot: I haz the power, 2016-09-09 12:26: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_...)

[kdsilva, 2016-09-09 13:13:57]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 13:14:08]

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

[kdsilva, 2016-09-09 14:50:20]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-09-09 14:50:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, liberato@chromium.org, mlamouri@chromium.org,
clamy@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2243473002/#ps580001
(title: "fixed bug")

[commit-bot: I haz the power, 2016-09-09 14:50:40]

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

[kdsilva, 2016-09-09 14:51:47]

The CQ bit was unchecked by kdsilva@google.com

[kdsilva, 2016-09-09 14:52:04]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 14:52:27]

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

[kdsilva, 2016-09-09 15:43:57]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 15:44:25]

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

[kdsilva, 2016-09-09 17:01:20]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:02:30]

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

[kdsilva, 2016-09-09 17:03:50]

The CQ bit was unchecked by kdsilva@google.com

[kdsilva, 2016-09-09 17:05:03]

Patchset #4 (id:60001) has been deleted

[kdsilva, 2016-09-09 17:05:12]

Patchset #2 (id:20001) has been deleted

[kdsilva, 2016-09-09 17:05:47]

Patchset #2 (id:40001) has been deleted

[kdsilva, 2016-09-09 17:15:28]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:15:45]

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

[kdsilva, 2016-09-09 18:42:03]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 18:42:29]

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

[kdsilva, 2016-09-09 19:13:43]

Patchset #31 (id:660001) has been deleted

[kdsilva, 2016-09-09 19:15:34]

Patchset #29 (id:620001) has been deleted

[commit-bot: I haz the power, 2016-09-09 20:20:49]

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

[commit-bot: I haz the power, 2016-09-09 20:20:50]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-09-12 08:52:53]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-09-12 08:52:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mlamouri@chromium.org, clamy@chromium.org,
foolip@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2243473002/#ps680001
(title: "updated tests")

[commit-bot: I haz the power, 2016-09-12 08:53:03]

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-09-12 08:55:03]

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

[commit-bot: I haz the power, 2016-09-12 08:55:05]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kdsilva, 2016-09-12 09:18:49]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 09:19:07]

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-09-12 11:28:03]

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

[commit-bot: I haz the power, 2016-09-12 11:28:04]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-09-12 12:08:32]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-09-12 12:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mlamouri@chromium.org, clamy@chromium.org,
foolip@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2243473002/#ps700001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-12 12:08:44]

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-09-12 12:13:14]

Committed patchset #31 (id:700001)

[commit-bot: I haz the power, 2016-09-12 12:14:45]

Description was changed from

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

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

to

==========
Adding overflow menu to media player.

Adding an overflow menu makes the media player more scalable to support
additional actions and features.

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

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

[commit-bot: I haz the power, 2016-09-12 12:14:47]

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