Bug 29987 - Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media controls using GraphicsContext.
Summary: Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media c...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-10-01 15:26 PDT by Andrew Scherkus
Modified: 2009-10-07 13:25 PDT (History)
4 users (show)

See Also:

Round 1 (43.15 KB, patch)
2009-10-01 16:05 PDT, Andrew Scherkus
eric: review-
Details | Formatted Diff | Diff
Round 2 (41.39 KB, patch)
2009-10-06 16:59 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Round 2 + ChangeLog (45.21 KB, patch)
2009-10-06 17:27 PDT, Andrew Scherkus
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
Round 3 (45.85 KB, patch)
2009-10-07 12:51 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-10-01 15:26:51 PDT
We were using a mix of GraphicsContext, CG and Skia canvas code depending on the port.  No more!!

This follows the pattern for RenderThemeSafari and RenderThemeWin where a common class RenderMediaControls is used to handle drawing the media controls.  For Chromium I created RenderMediaControlsChromium.

No need for additional layout tests as this is pure refactor/cosmetic change and does not address bugs or add new functionality.
Comment 1 Andrew Scherkus 2009-10-01 16:05:45 PDT
Created attachment 40483 [details]
Round 1
Comment 2 Eric Seidel (no email) 2009-10-02 12:51:17 PDT
The WebKit video folks should be cc'd.
Comment 3 Eric Seidel (no email) 2009-10-02 12:52:06 PDT
Oh, I read that as RenderThemeMac initially.  Now I see that this is chromium only.
Comment 4 Andrew Scherkus 2009-10-02 12:54:57 PDT
It's still ok to get video input :)

I'm hoping that one day we can get rid of the special paintMediaFoo() methods in RenderTheme and switch to paintMedia(ControlPart part) instead (you'll notice in RenderTheme::paint() we do the case handling bit to pass off to methods)
Comment 5 Eric Seidel (no email) 2009-10-02 13:01:10 PDT
Comment on attachment 40483 [details]
Round 1

Looks like you're just moving code.

 39     static bool shouldRenderMediaControlPart(ControlPart part, Element* e);

I'm not sure I understand this change:
 141         && (!style->hasAppearance() || document()->page()->theme()->shouldRenderMediaControlPart(style->appearance(), m_mediaElement));

I guess that allows all shouldRenderMediaControlPart implementations to ASSERT(appearance)?

Seems we could pretty easily consolidate this now, no?
// FIXME: this is copied from RenderMediaControls.cpp, try to consolidate.
 63 static HTMLMediaElement* parentMediaElement(RenderObject* o)

Seems we should just use a HashMap for all these now that we have so many:
79     static Image* soundFull = Image::loadPlatformResource("mediaSoundFull").releaseRef();
 80     static Image* soundNone = Image::loadPlatformResource("mediaSoundNone").releaseRef();
 81     static Image* soundDisabled = Image::loadPlatformResource("mediaSoundDisabled").releaseRef();


which looks it up and sticks it in the hash if it doesn't already exists.  I could ASSERT_NOT_REACHED that the loadPlatformResource ever fails.

Not required part of this change, but something to think about for the future.

Bug number?
 115     // FIXME: this should be a rounded rect but need to fix GraphicsContextSkia first.

We generally avoid default: for enums:
35         return true;
 236     default:
 237         return false;
Otherwise the compiler can't help us make sure we haven't missed a case.  Maybe ControlPart has a bunch more types than we're seeing here, in which case default: would maek sense.

Seems we should unify all the ASSERT_NOT_REACHED cases:
253     case MediaSeekBackButton:
 254         ASSERT_NOT_REACHED();
 255         break;
 256     case MediaSeekForwardButton:
 257         ASSERT_NOT_REACHED();
 258         break;

Seems this needs one more round.
Comment 6 Andrew Scherkus 2009-10-06 16:59:24 PDT
Created attachment 40750 [details]
Round 2

no appearance means it will pass in NoControlPart enum, which doesn't help the callee figure out which part it is and they can't make a good decision whether they should render it or not
Comment 7 Andrew Scherkus 2009-10-06 17:14:24 PDT
Comment on attachment 40750 [details]
Round 2

gah forgot to prepare an updated change log
Comment 8 Andrew Scherkus 2009-10-06 17:27:54 PDT
Created attachment 40752 [details]
Round 2 + ChangeLog
Comment 9 Andrew Scherkus 2009-10-06 17:30:45 PDT
MediaControlElements seemed like the most logical place to put parentMediaElement (both RenderMediaControls and RenderMediaControlsChromium use that file), but I made the function a global -- if that's not cool just let me know.

RenderMedia didn't feel like as good as a fit since we're dealing with RenderOjects.  HTMLMediaElement might work.. thoughts appreciated.
Comment 10 Eric Carlson 2009-10-07 08:18:48 PDT
Comment on attachment 40752 [details]
Round 2 + ChangeLog

> +HTMLMediaElement* parentMediaElement(RenderObject* o)
> +{
> +    Node* node = o->node();
> +    Node* mediaNode = node ? node->shadowAncestorNode() : 0;
> +    if (!mediaNode || (!mediaNode->hasTagName(HTMLNames::videoTag) && !mediaNode->hasTagName(HTMLNames::audioTag)))
> +        return 0;
> +
> +    return static_cast<HTMLMediaElement*>(mediaNode);

I suggest naming this "toParentMediaElement" to match the pattern used elsewhere in WebCore (toInputElement, toRenderXXXX, etc). This file seems like the right place for the function.

> +class RenderMediaControlsChromium {
> +public:
> +    static bool shouldRenderMediaControlPart(ControlPart part, Element* e);

Don't need the parameter names here.

r=me with these minor changes.
Comment 11 Andrew Scherkus 2009-10-07 12:51:07 PDT
Created attachment 40815 [details]
Round 3

Renamed parentMediaElement -> toParentMediaElement

Removed parameters from function signature in RenderMediaControlsChromium
Comment 12 Eric Carlson 2009-10-07 13:15:49 PDT
Comment on attachment 40815 [details]
Round 3


Comment 13 WebKit Commit Bot 2009-10-07 13:25:07 PDT
Comment on attachment 40815 [details]
Round 3

Clearing flags on attachment: 40815

Committed r49259: <http://trac.webkit.org/changeset/49259>
Comment 14 WebKit Commit Bot 2009-10-07 13:25:11 PDT
All reviewed patches have been landed.  Closing bug.