WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug