[Webkit-unassigned] [Bug 29987] Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media controls using GraphicsContext.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 2 13:01:10 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=29987
Eric Seidel <eric at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #40483|review?, commit-queue? |review-
Flag| |
--- Comment #5 from Eric Seidel <eric at webkit.org> 2009-10-02 13:01:10 PDT ---
(From update of attachment 40483)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list