Bug 162144 - Refactor CachedResourceLoader::canRequest
Summary: Refactor CachedResourceLoader::canRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-19 02:43 PDT by youenn fablet
Modified: 2016-09-21 01:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.01 KB, patch)
2016-09-19 03:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (24.30 KB, patch)
2016-09-21 01:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-19 02:43:16 PDT
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.
Comment 1 youenn fablet 2016-09-19 03:12:26 PDT
Created attachment 289221 [details]
Patch
Comment 2 Darin Adler 2016-09-20 09:55:59 PDT
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.
Comment 3 youenn fablet 2016-09-21 01:09:48 PDT
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
Comment 4 youenn fablet 2016-09-21 01:12:46 PDT
Created attachment 289436 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2016-09-21 01:45:38 PDT
Comment on attachment 289436 [details]
Patch for landing

Clearing flags on attachment: 289436

Committed r206203: <http://trac.webkit.org/changeset/206203>
Comment 6 WebKit Commit Bot 2016-09-21 01:45:42 PDT
All reviewed patches have been landed.  Closing bug.