MediaPlayerPrivate should not know about application cache

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 97146 [details]
proposed patch.

[WebKit Review Bot]

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.

[Alexis Menard (darktears)]

Comment on attachment 97146 [details]
proposed patch.

Nice patch, that should also make the support for other port for *free*

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

> 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.

[Eric Carlson]

(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.

[Eric Carlson]

Created attachment 97170 [details]
updated patch

[WebKit Review Bot]

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.

[Eric Carlson]

Created attachment 97172 [details]
Patch, this time with valid ChangeLog!

[Alexey Proskuryakov]

Appcache parts look good to me.

[Alexey Proskuryakov]

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.

[Eric Carlson]

(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!

[Eric Carlson]

http://trac.webkit.org/changeset/88958