WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
62648
MediaPlayerPrivate should not know about application cache
https://bugs.webkit.org/show_bug.cgi?id=62648
Summary
MediaPlayerPrivate should not know about application cache
Eric Carlson
Reported
2011-06-14 10:31:26 PDT
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.
Attachments
proposed patch.
(24.68 KB, patch)
2011-06-14 11:34 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
updated patch
(141.02 KB, patch)
2011-06-14 15:19 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch, this time with valid ChangeLog!
(24.32 KB, patch)
2011-06-14 15:24 PDT
,
Eric Carlson
ap
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
2011-06-14 11:34:15 PDT
Created
attachment 97146
[details]
proposed patch.
WebKit Review Bot
Comment 2
2011-06-14 11:36:36 PDT
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 3
2011-06-14 11:44:16 PDT
Comment on
attachment 97146
[details]
proposed patch. Nice patch, that should also make the support for other port for *free*
Alexey Proskuryakov
Comment 4
2011-06-14 11:46:49 PDT
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
Comment 5
2011-06-14 12:59:04 PDT
> 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
Comment 6
2011-06-14 15:18:50 PDT
(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
Comment 7
2011-06-14 15:19:23 PDT
Created
attachment 97170
[details]
updated patch
WebKit Review Bot
Comment 8
2011-06-14 15:20:57 PDT
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
Comment 9
2011-06-14 15:24:37 PDT
Created
attachment 97172
[details]
Patch, this time with valid ChangeLog!
Alexey Proskuryakov
Comment 10
2011-06-14 16:03:55 PDT
Appcache parts look good to me.
Alexey Proskuryakov
Comment 11
2011-06-15 10:08:34 PDT
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
Comment 12
2011-06-15 10:42:14 PDT
(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
Comment 13
2011-06-15 10:42:40 PDT
http://trac.webkit.org/changeset/88958
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug