[webkit-reviews] review granted: [Bug 29161] Implement default media UI for mac chromium. : [Attachment 39401] Reimplement default media UI for Mac Chromium to match the style

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Albert J. Wong
<ajwong at chromium.org>'s request for review:
Bug 29161: Implement default media UI for mac chromium.
https://bugs.webkit.org/show_bug.cgi?id=29161

Attachment 39401: Reimplement default media UI for Mac Chromium to match the
style
https://bugs.webkit.org/attachment.cgi?id=39401&action=review

------- Additional Comments from David Levin <levin at chromium.org>

Just a few nits to fix up and then commit.

> diff --git a/WebCore/rendering/RenderThemeChromiumMac.mm
b/WebCore/rendering/RenderThemeChromiumMac.mm

> +#if ENABLE(VIDEO)
> +// Attempt to retrieve a HTMLMediaElement from a Node. Returns NULL if one
cannot be found.

Use 0 instead of NULL. check-webkit-style would have flagged this :)


> +    static Image* soundFull =
Image::loadPlatformResource("mediaSoundFull").releaseRef();
> +    static Image* soundNone =
Image::loadPlatformResource("mediaSoundNone").releaseRef();
> +    static Image* soundDisabled =
Image::loadPlatformResource("mediaSoundDisabled").releaseRef();
> +
> +    if (mediaElement->networkState() == HTMLMediaElement::NETWORK_NO_SOURCE
|| !mediaElement->hasAudio())
> +	   return paintMediaButtonInternal(paintInfo.context, rect,
soundDisabled);
> +
> +    return paintMediaButtonInternal(paintInfo.context, rect,
mediaElement->muted() ? soundNone: soundFull);

Add a space before the :


> +    if (bufferedRect.width() > 0 && bufferedRect.height() > 0) {
> +	   FloatPoint p0 = bufferedRect.location();
> +	   FloatPoint p1 = p0;

Can you think of more meaningful names than p0 and p1? (sliderTop,
sliderBottom?)