Bug 49837 - Loader cleanup: Make Loader per-request, attach to CachedResourceLoader
Summary: Loader cleanup: Make Loader per-request, attach to CachedResourceLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-19 15:15 PST by Nate Chapin
Modified: 2010-12-20 14:02 PST (History)
5 users (show)

See Also:


Attachments
patch (35.28 KB, patch)
2010-11-19 15:20 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
More descriptive ChangeLog and merged to tip of tree (40.71 KB, patch)
2010-11-30 12:20 PST, Nate Chapin
koivisto: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-11-19 15:15:47 PST
This changes will give each request its own Loader, effectively merging Loader and Request.

After this change, Loader should be renamed to something more descriptive (maybe CachedResourceRequest?)
Comment 1 Nate Chapin 2010-11-19 15:20:46 PST
Created attachment 74431 [details]
patch

Note that I have not thoroughly tested this patch yet (it passes layout tests, but I haven't done extensive browsing with it yet).  I figured I should get some eyes more on it to check whether the general design seems sane.
Comment 2 Antti Koivisto 2010-11-25 13:13:17 PST
I like the design direction. Eliminating Request is very nice. Loader seriously needs renaming after this.

Could you explain the rationale for this change bit more (in the ChangeLog too)?

Looks good but I'll have try it before r+'ing.
Comment 3 Nate Chapin 2010-11-30 10:15:45 PST
(In reply to comment #2)
> I like the design direction. Eliminating Request is very nice. Loader seriously needs renaming after this.
> 
> Could you explain the rationale for this change bit more (in the ChangeLog too)?
> 
> Looks good but I'll have try it before r+'ing.

The rationale is something like:
(1) Loader shouldn't be a member of MemoryCache (especially since we're trying to make MemoryCache storage-only, and not involved in the actual loading process).  It should be a member of CachedResourceLoader.
(2) Loader no longer needs to be a singleton if it's not tied to MemoryCache.  Request's only purpose is to store per-request data that Loader can't store because it's a singleton.  Ergo, merge the two.

I'll upload a new patch with a more descriptive ChangeLog in a bit once I've had a chance to test the patch a little bit more.
Comment 4 Nate Chapin 2010-11-30 12:20:02 PST
Created attachment 75181 [details]
More descriptive ChangeLog and merged to tip of tree
Comment 5 Eric Seidel (no email) 2010-11-30 13:26:13 PST
Attachment 75181 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6447048
Comment 6 Antti Koivisto 2010-12-09 11:55:34 PST
Comment on attachment 75181 [details]
More descriptive ChangeLog and merged to tip of tree

r=me but please rename Loader to something more sensible in a follow-up patch and fix naming problems like:

void setRequest(Loader*)

at that point.
Comment 7 WebKit Commit Bot 2010-12-09 13:48:42 PST
Comment on attachment 75181 [details]
More descriptive ChangeLog and merged to tip of tree

Rejecting patch 75181 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
ERSION_MAJOR 0300
    setenv XCODE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/loader.o /Projects/CommitQueue/WebCore/loader/loader.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/6965014
Comment 8 Nate Chapin 2010-12-20 14:02:50 PST
Looks like I forgot to close this: http://trac.webkit.org/changeset/73749.