Don't use FetchRequest in FrameFetchContext

Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
R=yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2555713002
Cr-Commit-Position: refs/heads/master@{#442845}
Committed: https://chromium.googlesource.com/chromium/src/+/d4c201b64739f0285add84243b618966f978ef37

[tyoshino (SeeGerritForStatus), 2016-12-06 10:20:23]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 10:20:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 11:41:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 11:41:19]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-12-13 05:31:07]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[tyoshino (SeeGerritForStatus), 2016-12-13 05:31:20]

Description was changed from

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
==========

to

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
==========

[tyoshino (SeeGerritForStatus), 2016-12-13 05:31:21]

tyoshino@chromium.org changed reviewers:
+ yhirano@chromium.org

[commit-bot: I haz the power, 2016-12-13 05:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 07:02:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 07:02:23]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-26 08:56:45]

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchContext.h:162: virtual void
populateResourceRequest(Resource::Type,
Can you add a method comment?

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:154: // Exposed for
testing.
Can you add ForTesting suffix for each function?

[tyoshino (SeeGerritForStatus), 2017-01-10 09:36:28]

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchContext.h:162: virtual void
populateResourceRequest(Resource::Type,
On 2016/12/26 08:56:45, yhirano wrote:
> Can you add a method comment?

Done.

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:154: // Exposed for
testing.
On 2016/12/26 08:56:45, yhirano wrote:
> Can you add ForTesting suffix for each function?

They're exposed only for testing, but also used in the production code (used
internally in this class). So, we need to add ForTesting-suffixed methods to do
that (otherwise, we'll have call to ForTesting suffixed functions in the
production code).

Do you think we should do that or use "friend" to hide them?

[tyoshino (SeeGerritForStatus), 2017-01-10 09:55:56]

https://codereview.chromium.org/2555713002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:197: void
addCSPHeaderIfNecessary(Resource::Type, ResourceRequest&);
I've moved this to private as it seems we can.

https://codereview.chromium.org/2555713002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2555713002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:155: void
setFirstPartyCookieAndRequestorOrigin(ResourceRequest&) override;
aligned order of declaration

populateResourceRequest() then setFirstPartyCookieAndRequestorOrigin()

https://codereview.chromium.org/2555713002/diff/120002/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2555713002/diff/120002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:1: /*
Removed dependency to FetchRequest in this file where possible

[tyoshino (SeeGerritForStatus), 2017-01-10 09:56:01]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 09:56:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 11:51:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 11:51:59]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-11 07:12:30]

lgtm

[tyoshino (SeeGerritForStatus), 2017-01-11 07:28:39]

Description was changed from

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
==========

to

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
R=yhirano@chromium.org
==========

[tyoshino (SeeGerritForStatus), 2017-01-11 07:28:46]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2017-01-11 07:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2555713002/#ps150001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-11 07:28:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 10:17:49]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1484119726168040,
"parent_rev": "5b95a428b6dbbcdc487b23828e5637e0b69478b1", "commit_rev":
"d4c201b64739f0285add84243b618966f978ef37"}

[commit-bot: I haz the power, 2017-01-11 10:18:16]

Description was changed from

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
R=yhirano@chromium.org
==========

to

==========
Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
R=yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2555713002
Cr-Commit-Position: refs/heads/master@{#442845}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4c201b64739f0285add84243b61...
==========

[commit-bot: I haz the power, 2017-01-11 10:18:17]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/d4c201b64739f0285add84243b61...