[webkit-reviews] review denied: [Bug 29987] Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to render media controls using GraphicsContext. : [Attachment 40483] Round 1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Andrew Scherkus
<scherkus at chromium.org>'s request for review:
Bug 29987: Refactor RenderThemeChromiumMac and RenderThemeChromiumSkia to
render media controls using GraphicsContext.
https://bugs.webkit.org/show_bug.cgi?id=29987

Attachment 40483: Round 1
https://bugs.webkit.org/attachment.cgi?id=40483&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.