[webkit-reviews] review denied: [Bug 28196] Chromium: Show a "Playback Disabled" button on media error. : [Attachment 34608] Fix

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kyle Prete <kylep at chromium.org>'s
request for review:
Bug 28196: Chromium: Show a "Playback Disabled" button on media error.
https://bugs.webkit.org/show_bug.cgi?id=28196

Attachment 34608: Fix
https://bugs.webkit.org/attachment.cgi?id=34608&action=review

------- Additional Comments from David Levin <levin at chromium.org>
The diff here is odd.  It should have WebCore/ prefixed before each file.  I
suspect that you are working from a chromium directory and that's why you have
this problem.  (I don't do that so I don't know the fix.)  At worst fix it by
hand to aid in landing your patch.



> Index: ChangeLog
> +2009-08-11  Kyle Prete  <kylep at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

You need the bug title and a link to the bug here. (see other changelog
entries.)

prepare-ChangeLog -bug # will give the you the right format.


> +	   Use a disabled play button when the media file cannot be played.
> +
> +	   No new tests. (OOPS!)

This shouldn't be here.  Either add a test or indicate if existing tests cover
this.


> Index: rendering/RenderThemeChromiumSkia.cpp
>      static Image* mediaPlay =
Image::loadPlatformResource("mediaPlay").releaseRef();
>      static Image* mediaPause =
Image::loadPlatformResource("mediaPause").releaseRef();
> +    static Image* mediaPlayDisabled =
Image::loadPlatformResource("mediaPlayDisabled").releaseRef();
>  
> +    if (mediaElement->networkState() == HTMLMediaElement::NETWORK_NO_SOURCE)

> +	 return paintMediaButtonInternal(paintInfo.context, rect,
mediaPlayDisabled);

This needs a 4 space indent.