Summary: | Loader cleanup: Make Loader per-request, attach to CachedResourceLoader | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||
Component: | WebCore Misc. | Assignee: | Nate Chapin <japhet> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, ap, commit-queue, eric, koivisto | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Nate Chapin
2010-11-19 15:15:47 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.
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. (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. Created attachment 75181 [details]
More descriptive ChangeLog and merged to tip of tree
Attachment 75181 [details] did not build on mac: Build output: http://queues.webkit.org/results/6447048 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 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 Looks like I forgot to close this: http://trac.webkit.org/changeset/73749. |