Summary: | Replace disabled media mute button with port-specific implementation. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Scherkus <scherkus> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, eric | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Andrew Scherkus
2009-08-24 15:27:53 PDT
Created attachment 38502 [details]
First attempt
I'm still in the process of running the layout tests with a hypothesis that they should all still pass :) 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. 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 on attachment 38502 [details]
First attempt
This seems fine to me, but I'm not an expert here.
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 on attachment 38502 [details]
First attempt
Need to explicitly remove the element from the render tree instead of painting nothing.
*** Bug 29621 has been marked as a duplicate of this bug. *** 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 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. 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 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. Created attachment 40093 [details]
Added asserts
Added ASSERT(document()->page())
Comment on attachment 40093 [details]
Added asserts
r=me
Thanks!
Added commit-queue? (should I do this every time I upload a patch?) Comment on attachment 40027 [details]
Third attempt
Obsoleting this old patch.
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 on attachment 40093 [details] Added asserts Clearing flags on attachment: 40093 Committed r48822: <http://trac.webkit.org/changeset/48822> All reviewed patches have been landed. Closing bug. |