<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:jer.noble@apple.com" title="Jer Noble <jer.noble@apple.com>"> <span class="fn">Jer Noble</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - iOS 8 / OSX 10.10 WebGL - using a video from an other domain fails (CORS bug)"
href="https://bugs.webkit.org/show_bug.cgi?id=135379">bug 135379</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #266976 Flags</td>
<td>
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - iOS 8 / OSX 10.10 WebGL - using a video from an other domain fails (CORS bug)"
href="https://bugs.webkit.org/show_bug.cgi?id=135379#c20">Comment # 20</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - iOS 8 / OSX 10.10 WebGL - using a video from an other domain fails (CORS bug)"
href="https://bugs.webkit.org/show_bug.cgi?id=135379">bug 135379</a>
from <span class="vcard"><a class="email" href="mailto:jer.noble@apple.com" title="Jer Noble <jer.noble@apple.com>"> <span class="fn">Jer Noble</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=266976&action=diff" name="attach_266976" title="Proof of concept patch">attachment 266976</a> <a href="attachment.cgi?id=266976&action=edit" title="Proof of concept patch">[details]</a></span>
Proof of concept patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=266976&action=review">https://bugs.webkit.org/attachment.cgi?id=266976&action=review</a>
Thanks for the patch. We've previously looked into this mechanism for custom data loading of media resources, and our experiments revealed a number of problems with this approach. Namely:
This will only work for initial requests. Subsequent requests (for additional byte ranges, for sub-resources, or due to HTTP redirects) will not come through this path, and will thus we will not get a chance to do a CORS check on those requests. We have to assume that these requests would not pass CORS, or we would risk opening up a hole in CORS support. Additionally, this path is only triggered for the HLS manifest load, but requests for HLS media segments does not come through this API.
I am going to give this a r- due to the above, but only due to the shortcomings of the approach in the patch, not because of the patch itself. In addition, there are a few things in your patch I'd like to call out that would need to be fixed in the hypothetical case that the approach was valid.
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:412
> + int m_didPassCORSAccessCheck;</span >
This should be defined in terms of an enum, with defined states. Such as:
enum CORSAccessCheckResult {
CORSAccessUnknown,
CORSAccessDenied,
CORSAccessAllowed,
};
CORSAccessCheckResult m_didPassCORSAccessCheck { CORSAccessUnknown };
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:514
> + , m_didPassCORSAccessCheck(0)</span >
With C++11, this style of initialization can be put in the header.
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:839
> + if (parsedURL.protocol().lower() == "http") {
> + parsedURL.setProtocol("mock-http");
> + } else if (parsedURL.protocol().lower() == "https") {
> + parsedURL.setProtocol("mock-https");
> + }</span >
WebKit coding style guidelines <<a href="https://webkit.org/code-style-guidelines/">https://webkit.org/code-style-guidelines/</a>> specify dropping braces on single-line control statements.
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2222
> +bool MediaPlayerPrivateAVFoundationObjC::didPassCORSAccessCheck() const
> +{
> + //ASSERT(m_didPassCORSAccessCheck != 0);
> + return m_didPassCORSAccessCheck == 2;</span >
These should use the enum values i mentioned above. So this would become:
return m_didPassCORSAccessCheck == CORSAccessAllowed;
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2227
> +void MediaPlayerPrivateAVFoundationObjC::didFinishCORSAccessCheck(bool passed) {
> + m_didPassCORSAccessCheck = passed ? 2 : 1;
> +}</span >
And this would become:
m_didPassCORSAccessCheck = passed ? CORSAccessCheckAllowed : CORSAccessCheckDenied;
<span class="quote">> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3445
> + URL url([m_avAsset resolvedURL]);
> + if (url.protocol() == "mock-http") {
> + url.setProtocol("http");
> + } else if (url.protocol() == "mock-https") {
> + url.setProtocol("https");
> + }
> + return url;</span >
Ditto about the WebKit coding style guidelines.
<span class="quote">> avfoundation/objc/WebCoreAVFResourceLoader.mm:75
> + if (requestURL.protocol() == "mock-http") {
> + requestURL.setProtocol("http");
> + } else if (requestURL.protocol() == "mock-https") {
> + requestURL.setProtocol("https");
> + }</span >
Ditto.
<span class="quote">> avfoundation/objc/WebCoreAVFResourceLoader.mm:83
> + m_origin = loader->document() ? loader->document()->securityOrigin() : nullptr;
> + ASSERT(m_origin.get());</span >
No need to add .get() to a RefPtr or RetainPtr when testing nullity. Those classes provide a convenience bool operator.
<span class="quote">> avfoundation/objc/WebCoreAVFResourceLoader.mm:117
> + bool didPassCORSAccessCheck = resource->passesAccessControlCheck(*m_origin.get());</span >
Also, they override operator*(), so no need to add a .get() before dereferencing.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>