Bug 66018 – Plumb params CachedResourceLoader to SubresourceLoader

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

[Nate Chapin]

....specifically: sendLoadCallbacks, shouldSniffContent, shouldBufferData.

These parameters are available to SubresourceLoaders, but are currently only used by DocumentThreadableLoader (or are defaulted based on the CachedResource type).  This plumbing will be needed when DocumentThreadableLoader becomes a CachedResourceClient.

[Nate Chapin]

Created attachment 103546 [details]
patch

[Antti Koivisto]

Comment on attachment 103546 [details]
patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.h:114
> -    CachedResource* requestResource(CachedResource::Type, ResourceRequest&, const String& charset, ResourceLoadPriority = ResourceLoadPriorityUnresolved, bool isPreload = false);
> -    CachedResource* revalidateResource(CachedResource*, ResourceLoadPriority priority);
> -    CachedResource* loadResource(CachedResource::Type, ResourceRequest&, const String& charset, ResourceLoadPriority);
> -    void requestPreload(CachedResource::Type, ResourceRequest& url, const String& charset);
> +    // FIXME: sendLoadCallbacks, shouldSniffContent, and shouldBufferData will always default to true currently.
> +    // They are plumbed for http://bugs.webkit.org/show_bug.cgi?id=61225 .
> +    CachedResource* requestResource(CachedResource::Type, ResourceRequest&, const String& charset, ResourceLoadPriority = ResourceLoadPriorityUnresolved, bool isPreload = false, bool sendLoadCallbacks = true, bool shouldContentSniff = true, bool shouldBufferData = true);
> +    CachedResource* revalidateResource(CachedResource*, ResourceLoadPriority);
> +    CachedResource* loadResource(CachedResource::Type, ResourceRequest&, const String& charset, ResourceLoadPriority, bool sendLoadCallbacks, bool shouldContentSniff, bool shouldBufferData);

The number of arguments is getting unwieldy and rows of boolean parameters are generally nasty. How about bundling them into a struct?

[Nate Chapin]

> The number of arguments is getting unwieldy and rows of boolean parameters are generally nasty. How about bundling them into a struct?

I had tried to do something similar with https://bugs.webkit.org/show_bug.cgi?id=63301, but that patch was perhaps too ambitious.  I'll look into reviving that idea.

[Nate Chapin]

Created attachment 105399 [details]
patch using ResourceLoaderOptions

[Antti Koivisto]

Comment on attachment 105399 [details]
patch using ResourceLoaderOptions

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

r=me

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:175
> +    userSheet->setResourceLoaderOptions(ResourceLoaderOptions(DoNotSendCallbacks, SniffContent, BufferData));
> +    userSheet->load(this, /*incremental*/ false, SkipSecurityCheck);

Wouldn't it be better if ResourceLoaderOptions were made a parameter of load() instead of having a setter on CachedResource? They are never used separately. Maybe something to fix later, I realize it would require more refactoring.

[Nate Chapin]

Created attachment 105504 [details]
Patch for landing

[Nate Chapin]

(In reply to comment #5)
> (From update of attachment 105399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105399&action=review
> 
> r=me
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:175
> > +    userSheet->setResourceLoaderOptions(ResourceLoaderOptions(DoNotSendCallbacks, SniffContent, BufferData));
> > +    userSheet->load(this, /*incremental*/ false, SkipSecurityCheck);
> 
> Wouldn't it be better if ResourceLoaderOptions were made a parameter of load() instead of having a setter on CachedResource? They are never used separately. Maybe something to fix later, I realize it would require more refactoring.

Antti and I discussed this on #webkit and concluded that it made sense to land as-is, but to think about ways to clean it up further.

[WebKit Review Bot]

Comment on attachment 105504 [details]
Patch for landing

Clearing flags on attachment: 105504

Committed r94003: <http://trac.webkit.org/changeset/94003>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.