Support hidding an control bar element from the Media element controller (display:none does not work for media control elements)

• Component: New Bugs
• Status: RESOLVED FIXED

[Pierre d'Herbemont]

Support hidding an control bar element from the Media element controller.

We would then be able to remove the some buttons from <video controls> if the back end doesn't support those actions.

This is blocked by https://bugs.webkit.org/show_bug.cgi?id=26653

[Pierre d'Herbemont]

Created attachment 31735 [details]
patch v1.

Attached a patch that should be applied after the blocker.

[Pierre d'Herbemont]

Created attachment 31745 [details]
patch v2.

With ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 31745 [details]
patch v2.

You ChangeLog is missing the bug url.

Style:
+    if(m_isHidden)
+    if(!m_mediaElement || !m_mediaElement->renderer())
+    if (!renderer() && !m_isHidden && style->display() != NONE)
+    {
+        if (renderer)
+        {

When does this ever return NULL?
+        RenderObject* renderer = createRenderer(m_mediaElement->renderer()->renderArena(), style);

Why don't we just early-return in that case?

Why manually set attached instead of calling attach?
+            setAttached();

[Pierre d'Herbemont]

(In reply to comment #3)
> (From update of attachment 31745 [details] [review])
> You ChangeLog is missing the bug url.

Thanks.

> Style:
> +    if(m_isHidden)
> +    if(!m_mediaElement || !m_mediaElement->renderer())
> +    if (!renderer() && !m_isHidden && style->display() != NONE)
> +    {
> +        if (renderer)
> +        {

Thanks.

> When does this ever return NULL?
> +        RenderObject* renderer =
> createRenderer(m_mediaElement->renderer()->renderArena(), style);

That's what previous code does.

I guess it will be NULL if malloc fails basically.

> Why don't we just early-return in that case?

ok.

> Why manually set attached instead of calling attach?
> +            setAttached();

That's what previous code was doing :)

Using attach() requires some changes. And I think it deserve a separate bug. (Basically we need to create a renderer with a specific style, so we need a to have styleForRenderer() virtual, along with other changes).
I have created https://bugs.webkit.org/show_bug.cgi?id=26697 for that.

[Pierre d'Herbemont]

Created attachment 31817 [details]
patch v3.

ChangeLog with Bug number, coding style fixes.

[Simon Fraser (smfr)]

*** Bug 26228 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Comment on attachment 31817 [details]
patch v3.

Let's get rid of the m_isHidden on MediaControlInputElement, and a virtual method shouldBeVisible() on MediaControlInputElement that you can override to ask about fullscreen etc. updateStyle() will then need to handle the detach() case. Also need to clean up update() vs. updateStyle().

[Pierre d'Herbemont]

Created attachment 31882 [details]
patch v4.

Updated with Simon's comment.

[Simon Fraser (smfr)]

Comment on attachment 31882 [details]
patch v4.

> +    if (renderer() && !rendererIsNeeded(style))
> +        detach();
> +    else if (!renderer() && rendererIsNeeded(style)) {

It's probably worth changing this to call rendererIsNeeded() just once.

r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/45271