It's a layering violation.
Created attachment 238596 [details] Patch This patch is for GStreamer, because it's the only media backend I can actually test, but I added the new classes with the current mac implementation in mind, so they should be ready to be used by the mac port as well. After this patch fixing the mac port should be easy, I can try to do it myself, but it would be like a shot in the dark, and I'll need help to add the new files to the xcode files.
Attachment 238596 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5657: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 238597 [details] Try to fix the mac build The style issue is a false positive I think
Attachment 238597 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5657: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
I guess the mac build failure is because I have to add a symbol to a file or mark it as public or something like that.
(In reply to comment #5) > I guess the mac build failure is because I have to add a symbol to a file or mark it as public or something like that. Ah, I see now, I'm using MediaResourceLoader unconditionally in HTMLMediaElement::mediaPlayerCreateResourceLoader(). I can fix the windows build by adding the new files to the build, but I can't fix mac build (don't have a mac to deal with xcode files). I could add #if USE(GSTREAMER) with a FIXME that could be removed when mac switches to implement MediaPlayerResourceLoaderClient
There was a tool that the google folks created to add files to xcode project files, I used it in a patch I did which moved files used by Mac, but I can't remember its name, find it.
(In reply to comment #7) > There was a tool that the google folks created to add files to xcode project files, I used it in a patch I did which moved files used by Mac, but I can't remember its name, find it. I know, I tried it a couple of times in the past and never worked for me. Thanks anyway.
Created attachment 238945 [details] Patch Add build files to Xcode and Visual Studio projects and update WebCore change log entry.
Attachment 238945 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5652: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #9) > Created an attachment (id=238945) [details] > Patch > > Add build files to Xcode and Visual Studio projects and update WebCore change log entry. Thank you!
Comment on attachment 238945 [details] Patch The changes to the GStreamer code look good to me.
Nice clean-up Carlos, thanks :)
Created attachment 239602 [details] Rebased patch after r174563
Attachment 239602 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5656: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin, Eric, could you please confirm this approach is fine for mac also before I land this patch?
Comment on attachment 239602 [details] Rebased patch after r174563 View in context: https://bugs.webkit.org/attachment.cgi?id=239602&action=review A few random comments below. I can't determine how this approach affects Mac. > Source/WebCore/loader/MediaResourceLoader.cpp:57 > + if (m_resource) > + return false; Can the loader be started/stopped multiple times? If not, assertions would be more appropriate than runtime checks. > Source/WebCore/loader/MediaResourceLoader.cpp:69 > + // TODO: Decide whether to use preflight mode for cross-origin requests (see http://wkbug.com/131484). Please use FIXME, not TODO. However, the answer seems quite obvious, we do not need preflight. JS code does not control request method or headers, so it cannot attack a naive intranet server that does not expect malicious requests of these types. All servers need to expect malicious requests that are GET or POSTs generated by forms, because web pages could always send those - CORS makes it possible to send other types of cross-origin requests, and these need preflight. I recommend to remove this comment, and resolve the associated bug. > Source/WebCore/loader/MediaResourceLoader.cpp:94 > +// CachedResourceClient Please do not add these comments to .cpp files. Most people don't maintain them, so they just become incorrect with time. > Source/WebCore/loader/MediaResourceLoader.cpp:103 > + static NeverDestroyed<const String> consoleMessage(ASCIILiteral("Cross-origin media resource load denied by Cross-Origin Resource Sharing policy.")); There is no need to complicate code to optimize for this rare error case. A regular transient string would work perfectly well. > Source/WebCore/loader/MediaResourceLoader.cpp:106 > + m_client->accessControlCheckFailed(ResourceError(errorDomainWebKitInternal, 0, response.url().string(), consoleMessage.get())); Do we get two error lines in Web Inspector console as a result? > Source/WebCore/loader/MediaResourceLoader.h:40 > +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient { Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it.
(In reply to comment #17) > (From update of attachment 239602 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239602&action=review Thanks for looking at it. Note that this is mostly a refactoring, so most of the comments are about code that I have just moved to a different place. > A few random comments below. I can't determine how this approach affects Mac. The idea is that mac switch to use the MediaResourceLoader as well, instead of using the CacheResourceLoader directly, which is a layering violation. So, once this patch lands, WebCoreAVFResourceLoader should turn into a MediaPlayerResourceLoaderClient instead of a CachedRawResourceClient. > > Source/WebCore/loader/MediaResourceLoader.cpp:57 > > + if (m_resource) > > + return false; > > Can the loader be started/stopped multiple times? If not, assertions would be more appropriate than runtime checks. start() is always called once right after the object is created, but stop can be called multiple times, because the destructor call stop, and in most of the cases the loader has already been stopped when it's destroyed. > > Source/WebCore/loader/MediaResourceLoader.cpp:69 > > + // TODO: Decide whether to use preflight mode for cross-origin requests (see http://wkbug.com/131484). > > Please use FIXME, not TODO. > > However, the answer seems quite obvious, we do not need preflight. JS code does not control request method or headers, so it cannot attack a naive intranet server that does not expect malicious requests of these types. All servers need to expect malicious requests that are GET or POSTs generated by forms, because web pages could always send those - CORS makes it possible to send other types of cross-origin requests, and these need preflight. > > I recommend to remove this comment, and resolve the associated bug. Honestly, that comes from WebKitWebSourceGStreamer.cpp, and I don't even know what preflight means. I'll remove the comment anyway. > > Source/WebCore/loader/MediaResourceLoader.cpp:94 > > +// CachedResourceClient > > Please do not add these comments to .cpp files. Most people don't maintain them, so they just become incorrect with time. Ok. > > Source/WebCore/loader/MediaResourceLoader.cpp:103 > > + static NeverDestroyed<const String> consoleMessage(ASCIILiteral("Cross-origin media resource load denied by Cross-Origin Resource Sharing policy.")); > > There is no need to complicate code to optimize for this rare error case. A regular transient string would work perfectly well. Ok. > > Source/WebCore/loader/MediaResourceLoader.cpp:106 > > + m_client->accessControlCheckFailed(ResourceError(errorDomainWebKitInternal, 0, response.url().string(), consoleMessage.get())); > > Do we get two error lines in Web Inspector console as a result? hmm, I haven't checked the web inspector, but only one message is written to the js console, so I guess we get only one, like in the layout tests results. > > Source/WebCore/loader/MediaResourceLoader.h:40 > > +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient { > > Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it. What about PlatformMediaResourceLoader?
(In reply to comment #18) > > > Source/WebCore/loader/MediaResourceLoader.h:40 > > > +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient { > > > > Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it. > > What about PlatformMediaResourceLoader? or MediaResourceLoaderBase?
Comment on attachment 239602 [details] Rebased patch after r174563 View in context: https://bugs.webkit.org/attachment.cgi?id=239602&action=review This approach seems sensible for the Mac as well. Thanks for taking on this cleanup task! >>> Source/WebCore/loader/MediaResourceLoader.h:40 >>> +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient { >> >> Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it. > > What about PlatformMediaResourceLoader? If you are suggesting that MediaPlayerResourceLoader be renamed PlatformMediaResourceLoader and this class stays MediaResourceLoader, this seems fine to me.
Created attachment 240270 [details] Patch or landing
Attachment 240270 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5668: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r175050: <http://trac.webkit.org/changeset/175050>