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.
Created attachment 40483 [details] Round 1
The WebKit video folks should be cc'd.
Oh, I read that as RenderThemeMac initially. Now I see that this is chromium only.
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 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.
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 on attachment 40750 [details] Round 2 gah forgot to prepare an updated change log
Created attachment 40752 [details] Round 2 + ChangeLog
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 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.
Created attachment 40815 [details] Round 3 Renamed parentMediaElement -> toParentMediaElement Removed parameters from function signature in RenderMediaControlsChromium
Comment on attachment 40815 [details] Round 3 r=me Thanks!
Comment on attachment 40815 [details] Round 3 Clearing flags on attachment: 40815 Committed r49259: <http://trac.webkit.org/changeset/49259>
All reviewed patches have been landed. Closing bug.