Summary: | Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media controls using GraphicsContext. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Scherkus <scherkus> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarrin, commit-queue, eric.carlson, eric | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Andrew Scherkus
2009-10-01 15:26:51 PDT
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. |