Set the target type when creating the request for main resource

Set the target type when creating the request for main resources

This is a preparatory CL before refactoring the navigation logic. The goal is to have the TargetType set upon initialization of the ResourceRequest instead of inside the ResourceFetcher. Ultimately the navigations should not go through the ResourceFetcher anymore.

BUG=376025

[clamy, 2014-06-19 13:32:08]

https://chromiumcodereview.appspot.com/339593005/diff/40001/Source/core/fetch...
File Source/core/fetch/FetchRequest.cpp (right):

https://chromiumcodereview.appspot.com/339593005/diff/40001/Source/core/fetch...
Source/core/fetch/FetchRequest.cpp:44: m_resourceRequest.setPriority(priority);
In a third patch, the priority will no longer be set there for main resources.
As for now, WebURLrequests originated from content have not been properly
initialized and their priority needs to be set properly in ResourceFetcher.

[clamy, 2014-06-19 13:33:19]

@ppi: PTAL. You probably want to start with ResourceRequest and WebURLRequest as
the CL is a bit spread out :).

[ppi, 2014-06-19 18:25:43]

Thanks!

Should / could the logic that sets the highest priority for the targets being
main resources be moved to content/browser/loader? It seems that we have all the
information we need in ResourceDispatcherHostImpl::BeginRequest().

Please see more comments below.

https://codereview.chromium.org/339593005/diff/40001/Source/core/loader/Frame...
File Source/core/loader/FrameLoadRequest.h (left):

https://codereview.chromium.org/339593005/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoadRequest.h:35: 
Is this intentional?

https://codereview.chromium.org/339593005/diff/40001/Source/platform/network/...
File Source/platform/network/ResourceRequest.cpp (right):

https://codereview.chromium.org/339593005/diff/40001/Source/platform/network/...
Source/platform/network/ResourceRequest.cpp:85: void
ResourceRequest::initializeForMainResource(bool isMainFrame)
This can be called at any point, while ::initialize() has the nice property of
being private and called only from constructors.

There seems to be existing pattern here of setting additional parameters by
adding a specialized constructor that calls ::initialize() to set up defaults
first, and then overrides some of them, see "ResourceRequest(const KURL& url,
const Referrer& referrer ..." .

Would this pattern work for this case?

For overrides from content we could use simple setters (setTargetType /
setPriority).

https://codereview.chromium.org/339593005/diff/40001/Source/platform/network/...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/339593005/diff/40001/Source/platform/network/...
Source/platform/network/ResourceRequest.h:96: ResourceRequest(const KURL& url,
const Referrer& referrer, ResourceRequestCachePolicy cachePolicy =
UseProtocolCachePolicy)
Should we add a specialized constructor like this one, instead of
"initializeForMainResource" ? See also the comment in the .cpp file.

[clamy, 2014-06-23 15:32:50]

Thanks! PTAL

I have removed changes to the priority and will pursue them in a separate patch.

There are now two factory methods for creating ResourceRequests for main
resources. The constructor of WebURLRequest has been modified to make use of
them.

https://chromiumcodereview.appspot.com/339593005/diff/40001/Source/core/loade...
File Source/core/loader/FrameLoadRequest.h (left):

https://chromiumcodereview.appspot.com/339593005/diff/40001/Source/core/loade...
Source/core/loader/FrameLoadRequest.h:35: 
On 2014/06/19 18:25:42, ppi wrote:
> Is this intentional?

No, sorry my mistake.

[ppi, 2014-06-24 12:22:21]

This lgtm. Two remarks below, thanks!

https://codereview.chromium.org/339593005/diff/120001/Source/platform/exporte...
File Source/platform/exported/WebURLRequest.cpp (right):

https://codereview.chromium.org/339593005/diff/120001/Source/platform/exporte...
Source/platform/exported/WebURLRequest.cpp:74: WebURLRequestPrivateImpl(const
WebURL& url, bool isMainResource, bool isMainFrame)
nit: should we assert that isMainResource is true if isMainFrame is true?

https://codereview.chromium.org/339593005/diff/120001/Source/platform/network...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/339593005/diff/120001/Source/platform/network...
Source/platform/network/ResourceRequest.h:213: void
setTargetTypeForMainResource(bool isMainFrame);
Do we still need this in public: ?

[clamy, 2014-06-24 13:57:07]

Thanks for the review!

https://chromiumcodereview.appspot.com/339593005/diff/120001/Source/platform/...
File Source/platform/exported/WebURLRequest.cpp (right):

https://chromiumcodereview.appspot.com/339593005/diff/120001/Source/platform/...
Source/platform/exported/WebURLRequest.cpp:74: WebURLRequestPrivateImpl(const
WebURL& url, bool isMainResource, bool isMainFrame)
On 2014/06/24 12:22:20, ppi wrote:
> nit: should we assert that isMainResource is true if isMainFrame is true?

I am not sure it is necessary, considering we do not care about the value of
isMainFrame in that case.

https://chromiumcodereview.appspot.com/339593005/diff/120001/Source/platform/...
File Source/platform/network/ResourceRequest.h (right):

https://chromiumcodereview.appspot.com/339593005/diff/120001/Source/platform/...
Source/platform/network/ResourceRequest.h:213: void
setTargetTypeForMainResource(bool isMainFrame);
On 2014/06/24 12:22:21, ppi wrote:
> Do we still need this in public: ?

Done.

[clamy, 2014-06-24 14:01:42]

@japhet: PTAL. The goal of the patch is to eventually remove the determination
of ResourceRequest TargetType in ResourceFetcher for main resources (to be done
in a third patch).

[Nate Chapin, 2014-06-24 18:28:55]

LGTM with a couple questions.

https://codereview.chromium.org/339593005/diff/100002/Source/platform/network...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/339593005/diff/100002/Source/platform/network...
Source/platform/network/ResourceRequest.h:96: ResourceRequest(const KURL& url,
const Referrer& referrer, ResourceRequestCachePolicy cachePolicy =
UseProtocolCachePolicy)
Is this constructor actually used for subresource loads? I would have guessed it
was main resource only.

https://codereview.chromium.org/339593005/diff/100002/Source/platform/network...
Source/platform/network/ResourceRequest.h:104: static ResourceRequest
createMainResourceRequest(const KURL&, bool isMainFrame);
Why is it better to have these static factory methods instead than constructors
(like the other ways to create a ResourceRequest)?

[clamy, 2014-06-25 15:47:11]

@eseidel: PTAL

@japhet: Thanks! As per your suggestion I switched to using constructors, which
should fit better with the rest of the code.

https://chromiumcodereview.appspot.com/339593005/diff/100002/Source/platform/...
File Source/platform/network/ResourceRequest.h (right):

https://chromiumcodereview.appspot.com/339593005/diff/100002/Source/platform/...
Source/platform/network/ResourceRequest.h:96: ResourceRequest(const KURL& url,
const Referrer& referrer, ResourceRequestCachePolicy cachePolicy =
UseProtocolCachePolicy)
On 2014/06/24 18:28:54, Nate Chapin wrote:
> Is this constructor actually used for subresource loads? I would have guessed
it
> was main resource only.

You're right it is only used by main resources. I added a parameter to it.

https://chromiumcodereview.appspot.com/339593005/diff/100002/Source/platform/...
Source/platform/network/ResourceRequest.h:104: static ResourceRequest
createMainResourceRequest(const KURL&, bool isMainFrame);
On 2014/06/24 18:28:54, Nate Chapin wrote:
> Why is it better to have these static factory methods instead than
constructors
> (like the other ways to create a ResourceRequest)?

I thought it was more explicit that they give you a ResourceRequest that should
only be used for main resources. But it doesn't fit very well with the rest of
the code which uses constructors. So I made a new patch set with constructors,
so that we only have one way to create a ResourceRequest.