Bug 28689 - Replace disabled media mute button with port-specific implementation.
Summary: Replace disabled media mute button with port-specific implementation.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
: 29621 (view as bug list)
Depends on:
Reported: 2009-08-24 15:27 PDT by Andrew Scherkus
Modified: 2009-09-28 11:44 PDT (History)
3 users (show)

See Also:

First attempt (5.96 KB, patch)
2009-08-24 15:30 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Second attempt (5.53 KB, patch)
2009-09-21 18:42 PDT, Andrew Scherkus
eric.carlson: review-
Details | Formatted Diff | Diff
Third attempt (9.32 KB, patch)
2009-09-23 16:36 PDT, Andrew Scherkus
eric.carlson: review+
Details | Formatted Diff | Diff
Added asserts (9.39 KB, patch)
2009-09-24 16:36 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2009-08-24 15:27:53 PDT
Instead of skipping rendering the mute button when a source has no audio, allow ports to check hasAudio() and handle the rendering themselves.
Comment 1 Andrew Scherkus 2009-08-24 15:30:48 PDT
Created attachment 38502 [details]
First attempt
Comment 2 Andrew Scherkus 2009-08-24 15:31:46 PDT
I'm still in the process of running the layout tests with a hypothesis that they should all still pass :)
Comment 3 Eric Seidel (no email) 2009-08-25 10:25:25 PDT
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.
Comment 4 Andrew Scherkus 2009-08-25 10:27:04 PDT
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.
Comment 5 Adam Barth 2009-09-01 16:46:30 PDT
Comment on attachment 38502 [details]
First attempt

This seems fine to me, but I'm not an expert here.
Comment 6 Andrew Scherkus 2009-09-01 16:59:33 PDT
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.
Comment 7 Andrew Scherkus 2009-09-01 17:00:49 PDT
Comment on attachment 38502 [details]
First attempt

Need to explicitly remove the element from the render tree instead of painting nothing.
Comment 8 Andrew Scherkus 2009-09-21 18:38:56 PDT
*** Bug 29621 has been marked as a duplicate of this bug. ***
Comment 9 Andrew Scherkus 2009-09-21 18:42:41 PDT
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
Comment 10 Eric Carlson 2009-09-22 08:51:09 PDT
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.
Comment 11 Andrew Scherkus 2009-09-23 16:36:19 PDT
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)
Comment 12 Eric Carlson 2009-09-24 07:42:04 PDT
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);


r=me with this change.
Comment 13 Andrew Scherkus 2009-09-24 16:36:34 PDT
Created attachment 40093 [details]
Added asserts

Added ASSERT(document()->page())
Comment 14 Eric Carlson 2009-09-25 12:33:01 PDT
Comment on attachment 40093 [details]
Added asserts


Comment 15 Andrew Scherkus 2009-09-28 11:24:14 PDT
Added commit-queue?

(should I do this every time I upload a patch?)
Comment 16 Eric Seidel (no email) 2009-09-28 11:33:04 PDT
Comment on attachment 40027 [details]
Third attempt

Obsoleting this old patch.
Comment 17 Eric Seidel (no email) 2009-09-28 11:33:57 PDT
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).
Comment 18 WebKit Commit Bot 2009-09-28 11:44:01 PDT
Comment on attachment 40093 [details]
Added asserts

Clearing flags on attachment: 40093

Committed r48822: <http://trac.webkit.org/changeset/48822>
Comment 19 WebKit Commit Bot 2009-09-28 11:44:06 PDT
All reviewed patches have been landed.  Closing bug.