Bug 162144 – Refactor CachedResourceLoader::canRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

This method is doing security checks including CORS mode and CSP.
But it is hard to relate to fetch, especially since it is based on the resource type.

[youenn fablet]

Created attachment 289221 [details]
Patch

[Darin Adler]

Comment on attachment 289221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289221&action=review

> Source/WebCore/dom/ProcessingInstruction.cpp:142
> +                ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();

I would use auto here.

> Source/WebCore/dom/ScriptElement.cpp:284
> +    Document& document = m_element.document();

I would use auto& here.

> Source/WebCore/dom/ScriptElement.cpp:286
> +    if (document.frame() && !document.frame()->settings().isScriptEnabled())
> +        return nullptr;

I probably would have gone directly to document.settings() instead of going through document.frame(). Still would have needed a null check though.

    auto* settings = document.settings();
    if (settings && !settings.isScriptEnabled())
        return nullptr;

> Source/WebCore/loader/DocumentThreadableLoader.cpp:366
> +        ResourceLoaderOptions options = m_options;

I would have used auto here.

> Source/WebCore/loader/LinkLoader.cpp:203
> +        ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();

I would have used auto here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:393
> +    ContentSecurityPolicy::RedirectResponseReceived redirectResponseReceived = didReceiveRedirectResponse ? ContentSecurityPolicy::RedirectResponseReceived::Yes : ContentSecurityPolicy::RedirectResponseReceived::No;

I would have used auto here. This is really ugly too. The dark side of using enum class instead of boolean!

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:400
> +        if (!m_document->contentSecurityPolicy()->allowScriptFromSource(url, false, redirectResponseReceived))

Mysterious false here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:404
> +        if (!m_document->contentSecurityPolicy()->allowStyleFromSource(url, false, redirectResponseReceived))

Mysterious false here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:409
> +        if (!m_document->contentSecurityPolicy()->allowImageFromSource(url, false, redirectResponseReceived))

Mysterious false here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:416
> +        if (!m_document->contentSecurityPolicy()->allowFontFromSource(url, false, redirectResponseReceived))

Mysterious false here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:423
> +        if (!m_document->contentSecurityPolicy()->allowMediaFromSource(url, false, redirectResponseReceived))

Mysterious false here.

> Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp:55
> +    ResourceLoaderOptions fetchOptions = options;

I would have used auto here.

> Source/WebCore/xml/XSLImportRule.cpp:104
> +    ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();

I would have used auto here.

[youenn fablet]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 289221 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289221&action=review
> 
> > Source/WebCore/dom/ProcessingInstruction.cpp:142
> > +                ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();
> 

OK

> 
> > Source/WebCore/dom/ScriptElement.cpp:284
> > +    Document& document = m_element.document();
> 
> I would use auto& here.

OK

> > Source/WebCore/dom/ScriptElement.cpp:286
> > +    if (document.frame() && !document.frame()->settings().isScriptEnabled())
> > +        return nullptr;
> 
> I probably would have gone directly to document.settings() instead of going
> through document.frame(). Still would have needed a null check though.
> 
>     auto* settings = document.settings();
>     if (settings && !settings.isScriptEnabled())
>         return nullptr;

OK

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:366
> > +        ResourceLoaderOptions options = m_options;
> 
> I would have used auto here.

m_options is ThreadableLoaderOptions.

> > Source/WebCore/loader/LinkLoader.cpp:203
> > +        ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();
> 
> I would have used auto here.

OK

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:393
> > +    ContentSecurityPolicy::RedirectResponseReceived redirectResponseReceived = didReceiveRedirectResponse ? ContentSecurityPolicy::RedirectResponseReceived::Yes : ContentSecurityPolicy::RedirectResponseReceived::No;
> 
> I would have used auto here. This is really ugly too. The dark side of using
> enum class instead of boolean!

OK for auto.
It is especially ugly since the called function will pass to a submethod a boolean computed as: redirectResponseReceived == ContentSecurityPolicy::RedirectResponseReceived::Yes

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:400
> > +        if (!m_document->contentSecurityPolicy()->allowScriptFromSource(url, false, redirectResponseReceived))
> 
> Mysterious false here.

I plan to remove the second parameter which is not very useful for all allowXXFromSource.

> > Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp:55
> > +    ResourceLoaderOptions fetchOptions = options;
> 
> I would have used auto here.

OK

> > Source/WebCore/xml/XSLImportRule.cpp:104
> > +    ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();
> 
> I would have used auto here.

OK

[youenn fablet]

Created attachment 289436 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289436 [details]
Patch for landing

Clearing flags on attachment: 289436

Committed r206203: <http://trac.webkit.org/changeset/206203>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.