Bug 66018

Summary: Plumb params CachedResourceLoader to SubresourceLoader
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebCore Misc.Assignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, koivisto, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61225    
Attachments:
Description Flags
patch
none
patch using ResourceLoaderOptions
none
Patch for landing none

Nate Chapin
Reported 2011-08-10 15:44:46 PDT
....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.
Attachments
patch (12.51 KB, patch)
2011-08-10 15:48 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
patch using ResourceLoaderOptions (12.85 KB, patch)
2011-08-26 13:32 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Patch for landing (13.22 KB, patch)
2011-08-29 11:19 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 2011-08-10 15:48:20 PDT
Created attachment 103546 [details] patch
Antti Koivisto
Comment 2 2011-08-18 08:28:21 PDT
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
Comment 3 2011-08-18 08:32:45 PDT
> 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
Comment 4 2011-08-26 13:32:09 PDT
Created attachment 105399 [details] patch using ResourceLoaderOptions
Antti Koivisto
Comment 5 2011-08-28 00:06:12 PDT
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
Comment 6 2011-08-29 11:19:18 PDT
Created attachment 105504 [details] Patch for landing
Nate Chapin
Comment 7 2011-08-29 11:23:23 PDT
(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 8 2011-08-29 12:27:02 PDT
Comment on attachment 105504 [details] Patch for landing Clearing flags on attachment: 105504 Committed r94003: <http://trac.webkit.org/changeset/94003>
WebKit Review Bot
Comment 9 2011-08-29 12:27:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.