Bug 28196 – Chromium: Show a "Playback Disabled" button on media error.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kyle Prete]

Load and paint a mediaPlayDisabled image in RenderThemeChromiumSkia::paintMediaPlayButton() to display when the mediaElement cannot load.

[Kyle Prete]

Created attachment 34608 [details]
Fix

Loads and paints a disabled playback image.

[David Levin]

Comment on attachment 34608 [details]
Fix

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
> [phone]  Kyle Prete  <[e-mail]>
> +
> +        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.

[Kyle Prete]

Created attachment 34615 [details]
Fix

Thanks for the review. Your comments should be covered by this revision.

[Eric Seidel (no email)]

Comment on attachment 34615 [details]
Fix

Rejecting patch 34615 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34615 from bug 28196 failed to download and apply.

[Eric Seidel (no email)]

Comment on attachment 34615 [details]
Fix

Not sure why this was rejected from the queue.

[Eric Seidel (no email)]

Comment on attachment 34615 [details]
Fix

Rejecting patch 34615 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34615 from bug 28196 failed to download and apply.

[Eric Seidel (no email)]

Oh, this patch was made from within WebCore instead of the top level directory.  that's why it's failing:

patching file WebCore/ChangeLog
patch: **** malformed patch at line 20:  

patch -p0 "WebCore/ChangeLog" returned 2.  Pass --force to ignore patch failures.

[Eric Seidel (no email)]

It seems that we could teach svn-apply to handle such patches, especially since the full path is actually in the patch.  But maybe this patch is truly malformed?

[Kyle Prete]

Is it? This is only my second time submitting a patch to Webkit. If I've made a mistake, please let me know so I can fix it.

[David Levin]

Kyle, I think you fixed this issue :"It should have WebCore/ prefixed before each file." by hand, which is ok but you need to fix every file name,

Note that there is 
  --- ChangeLog	(revision 47069)
  +++ ChangeLog	(working copy)
in the patch, for example.

[Kyle Prete]

Created attachment 34756 [details]
Fix

Whoops. Ok, added more prefixes - this should work now.

[Eric Seidel (no email)]

Comment on attachment 34756 [details]
Fix

This change is wrong now too.

The patch should only have the directories under WebKit in it.

svn-create-patch (which you should be using) will do this for you.

bugzilla-tool post-diff 28196
will also do this for you.

[David Levin]

He has an odd enlistment. (enlisted in WebCore and JSC etc separately), so the svn-create-patch thing gives the wrong result.  (It leaves out WebCore/)

Kyle, the top level dir WebKit/shouldn't be in the path but you did fix all the file names this time :) Sorry if I miscommunicated this.

[Albert J. Wong]

I think this particular structure is going to end up being quite common for people who hack the webkit checkout that's embedded in the chromium source tree. :(

Interesitng note, if you specify the paths explicitly like "svn-create-patch WebCore", you will get diff output but the paths will end up rooted off the someplace weird (like the root of the chromium checkout).

I'm willing to manually land this if you guys are okay with that.

[Eric Seidel (no email)]

Seems like this hybrid checkout is not well supported.  Someone needs to file bugs and fix the tools if such a hybrid is desired.

[David Levin]

Comment on attachment 34756 [details]
Fix

The change is fine.  The patch format needs massaging his ajwong has offered to do.

[Albert J. Wong]

Committed as r47247.