Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to ResourceLoader

Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromium.org
BUG=669383
Committed: https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff
Cr-Commit-Position: refs/heads/master@{#438088}

[tyoshino (SeeGerritForStatus), 2016-11-28 05:31:36]

Description was changed from

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

BUG=none
==========

to

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

BUG=none
==========

[tyoshino (SeeGerritForStatus), 2016-11-28 05:32:23]

Description was changed from

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

BUG=none
==========

to

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

BUG=none
==========

[tyoshino (SeeGerritForStatus), 2016-11-28 05:38:02]

Description was changed from

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

BUG=none
==========

to

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

[tyoshino (SeeGerritForStatus), 2016-11-28 05:45:36]

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

[commit-bot: I haz the power, 2016-11-28 05:45:47]

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

[tyoshino (SeeGerritForStatus), 2016-11-28 05:46:13]

Description was changed from

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

to

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

[tyoshino (SeeGerritForStatus), 2016-11-28 05:46:14]

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

[tyoshino (SeeGerritForStatus), 2016-11-28 05:48:45]

Description was changed from

==========
Move most code handling callback from WebURLLoaderImpl from ResourceFetcher to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

[commit-bot: I haz the power, 2016-11-28 07:07:11]

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

[commit-bot: I haz the power, 2016-11-28 07:07:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tyoshino (SeeGerritForStatus), 2016-11-29 09:43:41]

Description was changed from

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=none
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=669383
==========

[tyoshino (SeeGerritForStatus), 2016-11-29 09:43:47]

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

[commit-bot: I haz the power, 2016-11-29 09:44:10]

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

[tyoshino (SeeGerritForStatus), 2016-11-29 09:56:55]

I discussed the design with yhirano offline.

I agreed with yhirano's point that if we move logic specific to a certain load
to ResourceLoader, we should move everything that matches the criteria.

But it turned out that most of remaining code in ResourceFetcher::didFailLoading
and didFinishLoading are not specific to a certain load but clean up of a
ResourceLoader and taking care of the loaded Resource.

So, I'd like to proceed with this patch. PTAL ps7.

[kinuko, 2016-12-01 04:09:22]

I'm a bit uncertain about these changes... in the existing code ResourceFetcher
is responsible for providing necessary hooks to the FetchContext, but in the new
code some code path go through ResourceFetcher but some do not and directly call
into FetchContext, which seems to feel a bit contrary to what the name
'FetchContext' implies.

Also this change seems to be going into the reverse direction to what Nate has
been trying a while ago (see [1]), have you discussed the direction with Nate?

[tyoshino (SeeGerritForStatus), 2016-12-01 06:19:09]

On 2016/12/01 04:09:22, kinuko wrote:
> I'm a bit uncertain about these changes... in the existing code
ResourceFetcher
> is responsible for providing necessary hooks to the FetchContext, but in the
new
> code some code path go through ResourceFetcher but some do not and directly
call
> into FetchContext, which seems to feel a bit contrary to what the name
> 'FetchContext' implies.

I had similar discussion with Yutaka and is noted in #15.

I also initially thought that the ResourceLoader was considered to be a light
weight adapter and it was supposed that actual logic is put into
ResourceFetcher. But in fact, it has non trivial amount of logic inside it e.g.
willFollowRedirect(). I think we should put everything in ResourceFetcher or
everything in ResourceLoader. This patch is an attempt to do the later. If we do
the former, we should just make the ResourceFetcher a WebURLLoaderClient, but I
guess it'll be less readable.

Regarding exposing FetchContext to ResourceLoader more, I don't think that's
really nice thing to do. So, I could keep the thin stub as what
ResourceFetcher::didReceiveData and ResourceFetcher::didDownloadData are doing
for example.

> 
> Also this change seems to be going into the reverse direction to what Nate has
> been trying a while ago (see [1]), have you discussed the direction with Nate?

Hit "Send Message" before pitting a reference for [1]?

[tyoshino (SeeGerritForStatus), 2016-12-01 06:25:49]

Split the removal of acceptDataFromThreadedReceiver into
https://codereview.chromium.org/2539353002/

[tyoshino (SeeGerritForStatus), 2016-12-01 07:40:42]

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

[commit-bot: I haz the power, 2016-12-01 07:41:09]

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-01 09:00:02]

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

[commit-bot: I haz the power, 2016-12-01 09:00:03]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-12-01 11:04:53]

On 2016/12/01 06:19:09, tyoshino wrote:
> On 2016/12/01 04:09:22, kinuko wrote:
> > I'm a bit uncertain about these changes... in the existing code
> ResourceFetcher
> > is responsible for providing necessary hooks to the FetchContext, but in the
> new
> > code some code path go through ResourceFetcher but some do not and directly
> call
> > into FetchContext, which seems to feel a bit contrary to what the name
> > 'FetchContext' implies.
> 
> I had similar discussion with Yutaka and is noted in #15.
> 
> I also initially thought that the ResourceLoader was considered to be a light
> weight adapter and it was supposed that actual logic is put into
> ResourceFetcher. But in fact, it has non trivial amount of logic inside it
e.g.
> willFollowRedirect(). I think we should put everything in ResourceFetcher or
> everything in ResourceLoader. This patch is an attempt to do the later. If we
do
> the former, we should just make the ResourceFetcher a WebURLLoaderClient, but
I
> guess it'll be less readable.
> 
> Regarding exposing FetchContext to ResourceLoader more, I don't think that's
> really nice thing to do. So, I could keep the thin stub as what
> ResourceFetcher::didReceiveData and ResourceFetcher::didDownloadData are doing
> for example.
> 
> > 
> > Also this change seems to be going into the reverse direction to what Nate
has
> > been trying a while ago (see [1]), have you discussed the direction with
Nate?
> 
> Hit "Send Message" before pitting a reference for [1]?

Am, sorry... [1] -> https://codereview.chromium.org/1975373002

[tyoshino (SeeGerritForStatus), 2016-12-01 11:12:28]

On 2016/12/01 11:04:53, kinuko wrote:
> Am, sorry... [1] -> https://codereview.chromium.org/1975373002

Thank you. I see... I'm still feeling that operation on a Resource which is
being loaded should be encapsulated to some extent. But given the Nate's attempt
and that it's just half year ago, I'll rethink.

[tyoshino (SeeGerritForStatus), 2016-12-02 10:41:20]

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

[tyoshino (SeeGerritForStatus), 2016-12-02 10:49:40]

On 2016/12/01 11:12:28, tyoshino wrote:
> On 2016/12/01 11:04:53, kinuko wrote:
> > Am, sorry... [1] -> https://codereview.chromium.org/1975373002
> 
> Thank you. I see... I'm still feeling that operation on a Resource which is
> being loaded should be encapsulated to some extent. But given the Nate's
attempt
> and that it's just half year ago, I'll rethink.

japhet@,

kinuko@ has pointed your comment that you planned to move logic in
ResourceLoader to ResourceFetcher as much as possible.
https://codereview.chromium.org/1975373002#msg12

That would make the relationship between ResourceFetcher and ResourceLoader more
like content::ResourceDispatcher which processes all IPCs by itself.

I think moving stuff for handling ongoing load from ResourceFetcher to
ResourceLoader would also make some sense. It'll be similar to relationship
between ResourceDispatcherHostImpl and ResourceLoader where ResourceLoader does
a lot and calls back to ResourceDispatcherHostImpl frequently.

Could you please tell us your thoughts on this?

Thanks!

[Nate Chapin, 2016-12-07 19:13:01]

I think there are 2 logically consistent ways to organize this code.

* ResourceLoader does all the per-load logic, and ResourceFetcher does all the
load start, cache policy and per-context bookkeeping (this CL)
* Fold all of ResourceLoader in to ResourceFetcher
(https://codereview.chromium.org/2051243002/ is my bitrotted WIP for that)

I like the contained nature of merging the two. The tradeoff is that
ResourceFetcher is starting to get big and complicated (merging would make that
worse), and it's simpler to move the per-load logic to ResourceLoader than it is
to kill ResourceLoader altogether.

On the balance, I think this approach is better, though I feel bad for the code
churn I created heading toward the other possibility :) LGTM w/nits.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:346: if
(!resource->loader())
Rather than stashing a Resource* here, could we check whether m_loader is null?

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.h (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:60: void restart(const
ResourceRequest&);
This can be private now I think.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:115: void
cancelWithURL(const KURL&);
This is unused?

[kinuko, 2016-12-09 02:03:19]

Cool that now we're settling :)

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:371: encodedDataLength);
This still bothers me a bit (I would be confused by who should be calling
these).  Could we have some documentation in FetchContext.h to note that both
ResourceFetcher and ResourceLoader are responsible for calling these methods?

Are there other places that directly call FetchContext methods other than from
ResourceFetcher? (Hm, I found one in PingLoader)

[yhirano, 2016-12-12 06:32:04]

lgtm

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:194: static void
populateRedirectResourceTimingOnFinish(ResourceTimingInfo* info,
[optional] I prefer placing this function to the unnamed namespace above to
annotating it with "static".

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:399: // |this| is dead
here.
No, |this| is not dead, in terms of use-after-free.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:425: // |this| is dead
here.
Ditto

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

Description was changed from

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

acceptDataFromThreadedReceiver() is removed since no one is using it.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=669383
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=669383
==========

[tyoshino (SeeGerritForStatus), 2016-12-12 11:10:55]

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:194: static void
populateRedirectResourceTimingOnFinish(ResourceTimingInfo* info,
On 2016/12/12 06:32:04, yhirano wrote:
> [optional] I prefer placing this function to the unnamed namespace above to
> annotating it with "static".

Done.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:346: if
(!resource->loader())
On 2016/12/07 19:13:00, Nate Chapin (slow until Jan 3) wrote:
> Rather than stashing a Resource* here, could we check whether m_loader is
null?

I just noticed that the ResourceLoader is garbage collected, and therefore we
don't need to do this. I've changed this to just touch m_resource.

Regarding use of m_loader, hmm, resource->loader() can become nullptr when
either of resource->error() or resource->finish() is called, but they don't
necessarily result in calling handleError() on the ResourceLoader.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:371: encodedDataLength);
On 2016/12/09 02:03:19, kinuko wrote:
> This still bothers me a bit (I would be confused by who should be calling
> these).  Could we have some documentation in FetchContext.h to note that both
> ResourceFetcher and ResourceLoader are responsible for calling these methods?

Done

> 
> Are there other places that directly call FetchContext methods other than from
> ResourceFetcher? (Hm, I found one in PingLoader)

I plan to remove the use in PingLoader soon. ImageResource is also touching it.
This can also be refactored to remove the use of FetchContext, I think.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:399: // |this| is dead
here.
On 2016/12/12 06:32:04, yhirano wrote:
> No, |this| is not dead, in terms of use-after-free.

Removed!

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:425: // |this| is dead
here.
On 2016/12/12 06:32:04, yhirano wrote:
> Ditto

Removed! Right, it's garbage collected.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.h (right):

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:60: void restart(const
ResourceRequest&);
On 2016/12/07 19:13:00, Nate Chapin (slow until Jan 3) wrote:
> This can be private now I think.

Done.

https://codereview.chromium.org/2533683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.h:115: void
cancelWithURL(const KURL&);
On 2016/12/07 19:13:01, Nate Chapin (slow until Jan 3) wrote:
> This is unused?

Oh, right. Removed.

[tyoshino (SeeGerritForStatus), 2016-12-12 11:14:33]

On 2016/12/07 19:13:01, Nate Chapin (slow until Jan 3) wrote:
> I think there are 2 logically consistent ways to organize this code.
> 
> * ResourceLoader does all the per-load logic, and ResourceFetcher does all the
> load start, cache policy and per-context bookkeeping (this CL)
> * Fold all of ResourceLoader in to ResourceFetcher
> (https://codereview.chromium.org/2051243002/ is my bitrotted WIP for that)
> 
> I like the contained nature of merging the two. The tradeoff is that
> ResourceFetcher is starting to get big and complicated (merging would make
that
> worse), and it's simpler to move the per-load logic to ResourceLoader than it
is
> to kill ResourceLoader altogether.
> 
> On the balance, I think this approach is better, though I feel bad for the
code
> churn I created heading toward the other possibility :) LGTM w/nits.

Thank you for review, Nate!

I share all the concern and trade-off here and understand your point. Grad that
we're settling.

Thank you, Kinuko, too, for suggesting that we should reach consensus before
proceeding.

[tyoshino (SeeGerritForStatus), 2016-12-12 11:14:40]

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

[commit-bot: I haz the power, 2016-12-12 11:14:49]

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

[tyoshino (SeeGerritForStatus), 2016-12-12 11:16:25]

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

[tyoshino (SeeGerritForStatus), 2016-12-12 11:17:21]

+tsepez please review platform/weborigin/

[commit-bot: I haz the power, 2016-12-12 12:39:09]

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

[commit-bot: I haz the power, 2016-12-12 12:39:10]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-12 16:35:07]

tsepez@chromium.org changed reviewers:
+ jww@chromium.org, mkwst@chromium.org

[Tom Sepez, 2016-12-12 16:35:08]

Mike or Joel -- how does this play with the suborigin work you've been doing
and/or sandbox frames?

[tyoshino (SeeGerritForStatus), 2016-12-12 16:44:07]

On 2016/12/12 16:35:08, Tom Sepez wrote:
> Mike or Joel -- how does this play with the suborigin work you've been doing
> and/or sandbox frames?

I think this basically wouldn't affect any work. I just factored out the origin
comparison function as after this patch both ResourceLoader and ResourceFetcher
would have a call to it. Are you worried about distribution of the origin logic
into two classes?

[Tom Sepez, 2016-12-12 16:46:25]

On 2016/12/12 16:44:07, tyoshino wrote:
> On 2016/12/12 16:35:08, Tom Sepez wrote:
> > Mike or Joel -- how does this play with the suborigin work you've been doing
> > and/or sandbox frames?
> 
> I think this basically wouldn't affect any work. I just factored out the
origin
> comparison function as after this patch both ResourceLoader and
ResourceFetcher
> would have a call to it. Are you worried about distribution of the origin
logic
> into two classes?

Not quite.  I worry about down the road other callers using this new method and
missing some additional checks that are required for "sameness".

[tyoshino (SeeGerritForStatus), 2016-12-12 16:47:42]

On 2016/12/12 16:44:07, tyoshino wrote:
> On 2016/12/12 16:35:08, Tom Sepez wrote:
> > Mike or Joel -- how does this play with the suborigin work you've been doing
> > and/or sandbox frames?
> 
> I think this basically wouldn't affect any work. I just factored out the
origin
> comparison function as after this patch both ResourceLoader and
ResourceFetcher
> would have a call to it. Are you worried about distribution of the origin
logic
> into two classes?

Oops. s/any work/any work except for ones highly depending on work separation
between ResourceFetcher and ResourceLoader.

[tyoshino (SeeGerritForStatus), 2016-12-12 16:48:27]

On 2016/12/12 16:46:25, Tom Sepez wrote:
> On 2016/12/12 16:44:07, tyoshino wrote:
> > On 2016/12/12 16:35:08, Tom Sepez wrote:
> > > Mike or Joel -- how does this play with the suborigin work you've been
doing
> > > and/or sandbox frames?
> > 
> > I think this basically wouldn't affect any work. I just factored out the
> origin
> > comparison function as after this patch both ResourceLoader and
> ResourceFetcher
> > would have a call to it. Are you worried about distribution of the origin
> logic
> > into two classes?
> 
> Not quite.  I worry about down the road other callers using this new method
and
> missing some additional checks that are required for "sameness".

I see. Thanks.

[tyoshino (SeeGerritForStatus), 2016-12-12 16:50:42]

On 2016/12/12 16:48:27, tyoshino wrote:
> On 2016/12/12 16:46:25, Tom Sepez wrote:
> > On 2016/12/12 16:44:07, tyoshino wrote:
> > > On 2016/12/12 16:35:08, Tom Sepez wrote:
> > > > Mike or Joel -- how does this play with the suborigin work you've been
> doing
> > > > and/or sandbox frames?
> > > 
> > > I think this basically wouldn't affect any work. I just factored out the
> > origin
> > > comparison function as after this patch both ResourceLoader and
> > ResourceFetcher
> > > would have a call to it. Are you worried about distribution of the origin
> > logic
> > > into two classes?
> > 
> > Not quite.  I worry about down the road other callers using this new method
> and
> > missing some additional checks that are required for "sameness".
> 
> I see. Thanks.

I can rename it to areSameSchemeHostPort() so that it would be very clear about
what it does.

[Tom Sepez, 2016-12-12 16:51:47]

> I see. Thanks.

In other words, I think calling isSameSchemeHostPort from your code is ok, but
adding a method that claims to answer the more complicated question of
"sameness" could come back to hurt us.  Thanks.

[tyoshino (SeeGerritForStatus), 2016-12-12 16:55:08]

On 2016/12/12 16:51:47, Tom Sepez wrote:
> > I see. Thanks.
> 
> In other words, I think calling isSameSchemeHostPort from your code is ok, but
> adding a method that claims to answer the more complicated question of
> "sameness" could come back to hurt us.  Thanks.

Yes. It's really good point. I see isSameSchemeHostPortAndSuborigin() and
isSameSchemeHostPort() there. It's bad to name the helper performing
isSameSchemeHostPort() with areSameOrigin. I'll rename it.

[tyoshino (SeeGerritForStatus), 2016-12-12 16:57:43]

On 2016/12/12 16:55:08, tyoshino wrote:
> On 2016/12/12 16:51:47, Tom Sepez wrote:
> > > I see. Thanks.
> > 
> > In other words, I think calling isSameSchemeHostPort from your code is ok,
but
> > adding a method that claims to answer the more complicated question of
> > "sameness" could come back to hurt us.  Thanks.
> 
> Yes. It's really good point. I see isSameSchemeHostPortAndSuborigin() and
> isSameSchemeHostPort() there. It's bad to name the helper performing
> isSameSchemeHostPort() with areSameOrigin. I'll rename it.

Done in ps12

[tyoshino (SeeGerritForStatus), 2016-12-12 16:57:54]

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

[commit-bot: I haz the power, 2016-12-12 16:58:09]

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

[tyoshino (SeeGerritForStatus), 2016-12-12 16:58:22]

Description was changed from

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

BUG=669383
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromiu...
BUG=669383
==========

[Tom Sepez, 2016-12-12 18:11:56]

lgtm

[commit-bot: I haz the power, 2016-12-13 00:05:29]

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

[commit-bot: I haz the power, 2016-12-13 00:05:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tyoshino (SeeGerritForStatus), 2016-12-13 05:08:11]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-13 05:08:11]

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

[commit-bot: I haz the power, 2016-12-13 05:08:29]

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

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

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-13 05:09:40]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-13 05:09:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
japhet@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2533683002/#ps260001
(title: "Fix build")

[commit-bot: I haz the power, 2016-12-13 05:09:59]

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

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

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-13 05:14:45]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-13 05:14:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
japhet@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2533683002/#ps280001
(title: "Fix test")

[commit-bot: I haz the power, 2016-12-13 05:15:16]

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:07:09]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481606085584160,
"parent_rev": "cc7aa3ac9cfc7b4e8d2c6aef99fb8ce6a681a716", "commit_rev":
"a423abb292a9aaf100c96e9b428f748626521dd5"}

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

Description was changed from

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromiu...
BUG=669383
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromiu...
BUG=669383

Review-Url: https://codereview.chromium.org/2533683002
==========

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

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-12-13 07:10:59]

Description was changed from

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromiu...
BUG=669383

Review-Url: https://codereview.chromium.org/2533683002
==========

to

==========
Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to
ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromiu...
BUG=669383

Committed: https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff
Cr-Commit-Position: refs/heads/master@{#438088}
==========

[commit-bot: I haz the power, 2016-12-13 07:11:01]

Patchset 15 (id:??) landed as
https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff
Cr-Commit-Position: refs/heads/master@{#438088}