RESOLVED FIXED 29987
Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media controls using GraphicsContext.
https://bugs.webkit.org/show_bug.cgi?id=29987
Summary Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media c...
Andrew Scherkus
Reported 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.
Attachments
Round 1 (43.15 KB, patch)
2009-10-01 16:05 PDT, Andrew Scherkus
eric: review-
Round 2 (41.39 KB, patch)
2009-10-06 16:59 PDT, Andrew Scherkus
no flags
Round 2 + ChangeLog (45.21 KB, patch)
2009-10-06 17:27 PDT, Andrew Scherkus
eric.carlson: review+
eric.carlson: commit-queue-
Round 3 (45.85 KB, patch)
2009-10-07 12:51 PDT, Andrew Scherkus
no flags
Andrew Scherkus
Comment 1 2009-10-01 16:05:45 PDT
Created attachment 40483 [details] Round 1
Eric Seidel (no email)
Comment 2 2009-10-02 12:51:17 PDT
The WebKit video folks should be cc'd.
Eric Seidel (no email)
Comment 3 2009-10-02 12:52:06 PDT
Oh, I read that as RenderThemeMac initially. Now I see that this is chromium only.
Andrew Scherkus
Comment 4 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)
Eric Seidel (no email)
Comment 5 2009-10-02 13:01:10 PDT
Comment on attachment 40483 [details] Round 1 Looks like you're just moving code. Style: 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(); platformResource("name") 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.
Andrew Scherkus
Comment 6 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
Andrew Scherkus
Comment 7 2009-10-06 17:14:24 PDT
Comment on attachment 40750 [details] Round 2 gah forgot to prepare an updated change log
Andrew Scherkus
Comment 8 2009-10-06 17:27:54 PDT
Created attachment 40752 [details] Round 2 + ChangeLog
Andrew Scherkus
Comment 9 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.
Eric Carlson
Comment 10 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.
Andrew Scherkus
Comment 11 2009-10-07 12:51:07 PDT
Created attachment 40815 [details] Round 3 Renamed parentMediaElement -> toParentMediaElement Removed parameters from function signature in RenderMediaControlsChromium
Eric Carlson
Comment 12 2009-10-07 13:15:49 PDT
Comment on attachment 40815 [details] Round 3 r=me Thanks!
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2009-10-07 13:25:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.