Adding overflow menu to media player

third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h

Index: third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h b/third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
index cabfb86e924f2d3dcca320a6ce64e7e978cad6fb..14279e15aacbfd7c84ad7de04b5cdcf9023574fb 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h
@@ -34,6 +34,7 @@
 #include "core/html/HTMLDivElement.h"
 #include "core/html/HTMLInputElement.h"
 #include "core/layout/LayoutBlock.h"
+#include "public/platform/WebLocalizedString.h"
 
 namespace blink {
 
@@ -89,6 +90,24 @@ public:
 
     MediaControlElementType displayType() const { return m_displayType; }
 
+    // By default, media controls elements are not added to the overflow menu.
+    // Controls that can be added to the overflow menu should override this
+    // function and return true.
+    virtual bool hasOverflowButton() { return false; }
+
+    // If true, shows the overflow menu item if it exists. Hides it if false.
+    void shouldShowButtonInOverflowMenu(bool);
+
+    // Creates an overflow menu HTML element.
+    virtual Element* initOverflowButton(MediaControls&) { return nullptr; }

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

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.

[kdsilva, 2016/08/29 20:44:13]

Done.

+
+    // Returns a string representation of the media control element. Used for
+    // the overflow menu.
+    String getText();
+
+    // Returns a string representation of the media control element.
+    virtual WebLocalizedString::Name getOverflowStringName() { return WebLocalizedString::AXAMPMFieldText; }

[kdsilva, 2016/08/27 14:20:49]

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?

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

nope.  :)

pick one, and NOTREACHED()

[kdsilva, 2016/08/29 20:44:13]

Done.

+
     DECLARE_VIRTUAL_TRACE();
 
 protected:
@@ -103,6 +122,11 @@ protected:
 
     void setDisplayType(MediaControlElementType);
 
+    // Represents the overflow menu element for this media control.

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

might want to mention that it's optional, and might be null.

[kdsilva, 2016/08/29 20:44:13]

Done.

+    // The Element contains the button that the user can click on, but having
+    // the button within an Element enables us to style the overflow menu.
+    Member<Element> m_overflowMenuElement;
+
 private:
     // Hide or show based on our fits / wanted state.  We want to show
     // if and only if we're wanted and we fit.
@@ -136,6 +160,9 @@ class MediaControlInputElement : public HTMLInputElement, public MediaControlEle
 public:
     DECLARE_VIRTUAL_TRACE();
 
+    // Creates an overflow menu element with the given button as a child.
+    HTMLLabelElement* createOverflowMenuElement(MediaControls&, MediaControlInputElement*);

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

can this return HTMLElement?

[kdsilva, 2016/08/29 20:44:13]

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

[liberato (no reviews please), 2016/08/30 16:42:06]

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.

+
 protected:
     MediaControlInputElement(MediaControls&, MediaControlElementType);