Load and paint a mediaPlayDisabled image in RenderThemeChromiumSkia::paintMediaPlayButton() to display when the mediaElement cannot load.
Created attachment 34608 [details] Fix Loads and paints a disabled playback image.
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 > +2009-08-11 Kyle Prete <kylep@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.
Created attachment 34615 [details] Fix Thanks for the review. Your comments should be covered by this revision.
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.
Comment on attachment 34615 [details] Fix Not sure why this was rejected from the queue.
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.
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?
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.
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.
Created attachment 34756 [details] Fix Whoops. Ok, added more prefixes - this should work now.
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.
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.
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.
Seems like this hybrid checkout is not well supported. Someone needs to file bugs and fix the tools if such a hybrid is desired.
Comment on attachment 34756 [details] Fix The change is fine. The patch format needs massaging his ajwong has offered to do.
Committed as r47247.