MediaPlayerPrivate implementations are in WebCore/Source/platform/graphics/, so they should have no knowledge of the application cache. HTMLMediaElement should be responsible for dealing with the application cache.
Created attachment 97146 [details] proposed patch.
Attachment 97146 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/html/HTMLMediaElement.h:303: The parameter name "state" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 97146 [details] proposed patch. Nice patch, that should also make the support for other port for *free*
Comment on attachment 97146 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=97146&action=review > Source/WebCore/html/HTMLMediaElement.cpp:718 > + mediaLoadingFailed(MediaPlayer::NetworkError); This is a problem with non-media loading too, but we should eventually provide more reasonable errors for these failures. > Source/WebCore/html/HTMLMediaElement.h:343 > + KURL createFileURLForApplicationCacheResource(const String& path); Can this function just be file static? > LayoutTests/http/tests/appcache/video.html:67 > + applicationCache.addEventListener("cached", test1, false); I don't understand how the test can be passing in DRT. DRT is always downloading appcache for the first time (because it's deleted on launch), so without swapCache(), cache isn't going to be used. But why does the second part of the test pass then?
> I don't understand how the test can be passing in DRT. I was just confused about this. For the initial load, the cache is used immediately once it's ready.
(In reply to comment #4) > (From update of attachment 97146 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97146&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:718 > > + mediaLoadingFailed(MediaPlayer::NetworkError); > > This is a problem with non-media loading too, but we should eventually provide more reasonable errors for these failures. > OK. > > Source/WebCore/html/HTMLMediaElement.h:343 > > + KURL createFileURLForApplicationCacheResource(const String& path); > > Can this function just be file static? > Good idea.
Created attachment 97170 [details] updated patch
Attachment 97170 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 97172 [details] Patch, this time with valid ChangeLog!
Appcache parts look good to me.
Comment on attachment 97172 [details] Patch, this time with valid ChangeLog! View in context: https://bugs.webkit.org/attachment.cgi?id=97172&action=review I would love to have a test case that tries to load a non-media resource into a <video> element. And maybe some more explicit error handling for that - we now rely on a failure to load from an empty path when the resource hasn't been stored in a file due to a wrong MIME type. > Source/WebCore/html/HTMLMediaElement.cpp:687 > + url.setProtocol("file"); > + url.setPath(path); Did you verify that this works on Windows? > Source/WebCore/html/HTMLMediaElement.cpp:718 > + mediaLoadingFailed(MediaPlayer::NetworkError); Please file a bug about the other cases that you found where we lack this call.
(In reply to comment #11) > (From update of attachment 97172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97172&action=review > > I would love to have a test case that tries to load a non-media resource into a <video> element. And maybe some more explicit error handling for that - we now rely on a failure to load from an empty path when the resource hasn't been stored in a file due to a wrong MIME type. > I added an additional test that tries to set video.src to one of the JavaScript files in the manifest to ensure that it fails and generates an error event. > > Source/WebCore/html/HTMLMediaElement.cpp:687 > > + url.setProtocol("file"); > > + url.setPath(path); > > Did you verify that this works on Windows? > I will watch the bots ;-) > > Source/WebCore/html/HTMLMediaElement.cpp:718 > > + mediaLoadingFailed(MediaPlayer::NetworkError); > > Please file a bug about the other cases that you found where we lack this call. > I will. Thanks!
http://trac.webkit.org/changeset/88958