Bug 28689 – Replace disabled media mute button with port-specific implementation.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrew Scherkus]

Instead of skipping rendering the mute button when a source has no audio, allow ports to check hasAudio() and handle the rendering themselves.

[Andrew Scherkus]

Created attachment 38502 [details]
First attempt

[Andrew Scherkus]

I'm still in the process of running the layout tests with a hypothesis that they should all still pass :)

[Eric Seidel (no email)]

That's a long layout test run. :)  Eric should officially be a reviewer by now... or certainly is hours away.  CCing him so he at least sees this go by.

[Andrew Scherkus]

I got completely sidetracked during said layout test run :(

I don't have a mac machine to call my own so I'm borrowing someone else's in the meantime.

[Adam Barth]

Comment on attachment 38502 [details]
First attempt

This seems fine to me, but I'm not an expert here.

[Andrew Scherkus]

it's review- :(

I need to fix the issue via adjusting the style to display:none otherwise we have a strange looking "missing square" on the mac port.

[Andrew Scherkus]

Comment on attachment 38502 [details]
First attempt

Need to explicitly remove the element from the render tree instead of painting nothing.

[Andrew Scherkus]

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

[Andrew Scherkus]

Created attachment 39900 [details]
Second attempt

This is a draft of deferring the decision on rendering a media control element to the RenderTheme.

This is my first real dive into Element/RenderObject/Style/Theme interaction, so feel free to point out my mistakes :)

The overall change is:
 1) Media control elements will always create a renderer
 2) RenderTheme decides whether those renderers will be visible based on inspecting the HTMLMediaElement

[Eric Carlson]

Comment on attachment 39900 [details]
Second attempt


> +        Sample implementation for port-specific rendering of media controls.

"Sample implementation", what do you mean?


> +    // Alternatively we could pass in the theme for fine-tuned adjustment.  If
> +    // we keep it as simply setting display to NONE then I might rename this method
> +    // shouldDisplayMediaControlPart instead.
> +    if (!renderer->theme()->shouldRenderMediaControlPart(renderer))
> +        style->setDisplay(NONE);

This shouldn't be necessary. I think if have both MediaControlElement::
rendererIsNeeded and MediaControlInputElement:: rendererIsNeeded call the theme
method it should just work.


>  bool MediaControlMuteButtonElement::rendererIsNeeded(RenderStyle* style)
>  {
> -    return MediaControlInputElement::rendererIsNeeded(style) && !disabled();
> +    // All subclasses will probably end up NOT implementing this method anymore.
> +    return MediaControlInputElement::rendererIsNeeded(style);

Why not just remove the method completely?


> +bool RenderTheme::shouldRenderMediaControlPart(RenderObject* o)
> +{
> +    HTMLMediaElement* mediaElement = static_cast<MediaControlInputElement*>(o->node())->mediaElement();
> +    switch (o->style()->appearance()) {
> +    case MediaMuteButtonPart:
> +        return mediaElement->hasAudio();
> +    // More media parts can go here, but then we end up moving localized logic
> +    // from the MediaControl elements into a big switch statement here, which
> +    // kind of sucks.

"which kind of sucks" is probably not the most helpful comment to someone coming along later.

[Andrew Scherkus]

Created attachment 40027 [details]
Third attempt

Being new to this part of WebKit I'm not quite sure how to access the RenderTheme from inside an HTMLElement (and its subclasses).  document()->page()->theme() was the best I could come up with (looks like what RenderObject does)

[Eric Carlson]

Comment on attachment 40027 [details]
Third attempt

> -    return HTMLDivElement::rendererIsNeeded(style) && parent() && parent()->renderer();
> +    return HTMLDivElement::rendererIsNeeded(style) && parent() && parent()->renderer()
> +        && document()->page()->theme()->shouldRenderMediaControlPart(style->appearance(), m_mediaElement);

I don't know if the page can ever be NULL in this situation, but it is probably worth adding a check since RenderObject::theme() asserts it.

> -    return HTMLInputElement::rendererIsNeeded(style) && parent() && parent()->renderer();
> +    return HTMLInputElement::rendererIsNeeded(style) && parent() && parent()->renderer()
> +        && document()->page()->theme()->shouldRenderMediaControlPart(style->appearance(), m_mediaElement);

Ditto.

r=me with this change.

[Andrew Scherkus]

Created attachment 40093 [details]
Added asserts

Added ASSERT(document()->page())

[Eric Carlson]

Comment on attachment 40093 [details]
Added asserts

r=me

Thanks!

[Andrew Scherkus]

Added commit-queue?

(should I do this every time I upload a patch?)

[Eric Seidel (no email)]

Comment on attachment 40027 [details]
Third attempt

Obsoleting this old patch.

[Eric Seidel (no email)]

Comment on attachment 40093 [details]
Added asserts

You only need to set cq? if you want the commit-queue to auto-commit the patch for you.  If that's every time, then yes, you should set cq? every time.  I need to add an option to have post-diff do that for you (bug 29202).

[WebKit Commit Bot]

Comment on attachment 40093 [details]
Added asserts

Clearing flags on attachment: 40093

Committed r48822: <http://trac.webkit.org/changeset/48822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.