Bug 50848 - Rename Loader
Summary: Rename Loader
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-12-10 14:56 PST by Nate Chapin
Modified: 2010-12-14 13:22 PST (History)
6 users (show)

See Also:


Attachments
patch (41.02 KB, patch)
2010-12-10 15:03 PST, Nate Chapin
abarth: 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-12-10 14:56:37 PST
...I've thrown around the name CachedResourceRequest in a couple of places and haven't heard any objections, so that's what I'll call it in my first attempt :)
Comment 1 Nate Chapin 2010-12-10 15:03:30 PST
Created attachment 76265 [details]
patch
Comment 2 Adam Barth 2010-12-10 15:07:39 PST
Comment on attachment 76265 [details]
patch

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

Interesting how the variable of this type is often called a request.

> WebCore/loader/cache/CachedResourceRequest.cpp:105
> -PassRefPtr<Loader> Loader::load(CachedResourceLoader* cachedResourceLoader, CachedResource* resource, bool incremental, SecurityCheckPolicy securityCheck, bool sendResourceLoadCallbacks)
> +PassRefPtr<CachedResourceRequest> CachedResourceRequest::load(CachedResourceLoader* cachedResourceLoader, CachedResource* resource, bool incremental, SecurityCheckPolicy securityCheck, bool sendResourceLoadCallbacks)

Should this just be called "create" ?  Does this method actually kick off the load?
Comment 3 Nate Chapin 2010-12-10 15:15:10 PST
(In reply to comment #2)
> (From update of attachment 76265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76265&action=review
> 
> Interesting how the variable of this type is often called a request.

In http://trac.webkit.org/changeset/73749, I changed those functions from taking a Request to a Loader but didn't rename everything because this patch would soon follow it.
> 
> > WebCore/loader/cache/CachedResourceRequest.cpp:105
> > -PassRefPtr<Loader> Loader::load(CachedResourceLoader* cachedResourceLoader, CachedResource* resource, bool incremental, SecurityCheckPolicy securityCheck, bool sendResourceLoadCallbacks)
> > +PassRefPtr<CachedResourceRequest> CachedResourceRequest::load(CachedResourceLoader* cachedResourceLoader, CachedResource* resource, bool incremental, SecurityCheckPolicy securityCheck, bool sendResourceLoadCallbacks)
> 
> Should this just be called "create" ?  Does this method actually kick off the load?

CachedResourceRequest::load() schedules the load with ResourceLoadScheduler, which from the perspective of its user (CachedResourceLoader) is kicking off the load.

I'm open the idea of renaming it if that's what people want.
Comment 4 Adam Barth 2010-12-10 15:19:55 PST
Comment on attachment 76265 [details]
patch

Let's move forward with this patch.  In the next patch, it might be good to split up load into a create and a start method (or something) to separate the notion of the a request (which should be mostly data, like noun) from a loader (which shown do something, like a verb).
Comment 5 WebKit Commit Bot 2010-12-10 18:54:01 PST
Comment on attachment 76265 [details]
patch

Rejecting attachment 76265 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--non-interactive', 76265]" exit_code: 2
Last 500 characters of output:
ceRequest.h
|===================================================================
|--- WebCore/loader/cache/CachedResourceRequest.h	(revision 73747)
|+++ WebCore/loader/cache/CachedResourceRequest.h	(working copy)
--------------------------
No file to patch.  Skipping patch.
2 out of 2 hunks ignored
patching file WebCore/loader/loader.cpp
rm 'WebCore/loader/loader.cpp'

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6962034
Comment 6 WebKit Review Bot 2010-12-14 12:37:18 PST
http://trac.webkit.org/changeset/74049 might have broken Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug
Comment 7 Nate Chapin 2010-12-14 13:22:12 PST
Landed manually in http://trac.webkit.org/changeset/74049