AssociatedURLLoader shouldn't derive from WebURLLoader

AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium.org,dalecurtis@chromium.org,mmenke@chromium.org
BUG=none

Committed: https://crrev.com/6d7671174edc71594c28a3766d641a2c1dbf47e9
Cr-Commit-Position: refs/heads/master@{#427022}

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

Patchset #1 (id:1) has been deleted

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

Patchset #1 (id:20001) has been deleted

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

Patchset #1 (id:40001) has been deleted

[tyoshino (SeeGerritForStatus), 2016-10-06 11:24:03]

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

[commit-bot: I haz the power, 2016-10-06 11:24:22]

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-10-06 11:36:02]

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

[commit-bot: I haz the power, 2016-10-06 11:36:03]

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

[tyoshino (SeeGerritForStatus), 2016-10-07 10:01:11]

Patchset #1 (id:60001) has been deleted

[tyoshino (SeeGerritForStatus), 2016-10-07 10:01:25]

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

[commit-bot: I haz the power, 2016-10-07 10:01:48]

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-10-07 10:21:43]

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

[commit-bot: I haz the power, 2016-10-07 10:21:44]

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

[tyoshino (SeeGerritForStatus), 2016-10-07 11:03:34]

Patchset #1 (id:80001) has been deleted

[tyoshino (SeeGerritForStatus), 2016-10-11 11:28:03]

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

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

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-10-11 11:40:07]

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

[commit-bot: I haz the power, 2016-10-11 11:40:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tyoshino (SeeGerritForStatus), 2016-10-12 06:48:26]

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

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

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-10-12 06:48:41]

Patchset #1 (id:100001) has been deleted

[tyoshino (SeeGerritForStatus), 2016-10-12 06:48:50]

Patchset #1 (id:120001) has been deleted

[tyoshino (SeeGerritForStatus), 2016-10-12 06:48:59]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-12 08:25:34]

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

[tyoshino (SeeGerritForStatus), 2016-10-12 08:25:36]

Description was changed from

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

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

to

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

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

[tyoshino (SeeGerritForStatus), 2016-10-12 08:25:36]

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

[commit-bot: I haz the power, 2016-10-12 08:25:36]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-10-12 08:27:14]

Hi Jochen,

This is a follow up for https://codereview.chromium.org/2230173002/#msg73

Sorry, it's huge, but could you please take a look?

[jochen (gone - plz use gerrit), 2016-10-12 09:40:42]

lgtm

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

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

[tyoshino (SeeGerritForStatus), 2016-10-12 10:04:21]

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

[tyoshino (SeeGerritForStatus), 2016-10-12 10:04:47]

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

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

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

[tyoshino (SeeGerritForStatus), 2016-10-12 10:07:03]

+bbudge for components/nacl/renderer/
+mmenke for chrome/renderer/net/
+hubbe for media/blink/
+raymes for extensions/renderer/guest_view/mime_handler_view/

[mmenke, 2016-10-12 14:48:28]

On 2016/10/12 10:07:03, tyoshino wrote:
> +bbudge for components/nacl/renderer/
> +mmenke for chrome/renderer/net/
> +hubbe for media/blink/
> +raymes for extensions/renderer/guest_view/mime_handler_view/

chrome/renderer/net LGTM

[bbudge, 2016-10-12 17:45:03]

components/nacl/renderer
content/renderer/pepper

lgtm w/nit

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:103:
std::unique_ptr<blink::WebAssociatedURLLoader> for_destruction_only(
nit: prefix blink isn't needed because of using.

[raymes, 2016-10-14 02:36:46]

https://codereview.chromium.org/2399463007/diff/180001/extensions/renderer/gu...
File
extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h
(right):

https://codereview.chromium.org/2399463007/diff/180001/extensions/renderer/gu...
extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h:44:
public blink::WebAssociatedURLLoaderClient {
This looks ok as long as WebAssociatedURLLoader is the right one to use here.
I'm not sure how I know that. But assuming you feel confident about the answer
to that, the code changes lg.

[kinuko, 2016-10-14 03:15:19]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-10-14 03:15:21]

This looks like a nice cleanup!

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/fetch...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:90: // The
AssociatedURLLoader will continue after a load failure.
Do we still need these behaviors in this class?

[tyoshino (SeeGerritForStatus), 2016-10-14 06:12:07]

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/fetch...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:90: // The
AssociatedURLLoader will continue after a load failure.
On 2016/10/14 03:15:20, kinuko (slowish) wrote:
> Do we still need these behaviors in this class?

Good catch! Replaced this and one in didFinishLoading with DCHECK().

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2399463007/diff/180001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:103:
std::unique_ptr<blink::WebAssociatedURLLoader> for_destruction_only(
On 2016/10/12 17:45:03, bbudge wrote:
> nit: prefix blink isn't needed because of using.

Fixed. I also removed some unnecessary blink:: prefixes from this file.

https://codereview.chromium.org/2399463007/diff/180001/extensions/renderer/gu...
File
extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h
(right):

https://codereview.chromium.org/2399463007/diff/180001/extensions/renderer/gu...
extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h:44:
public blink::WebAssociatedURLLoaderClient {
On 2016/10/14 02:36:45, raymes wrote:
> This looks ok as long as WebAssociatedURLLoader is the right one to use here.
> I'm not sure how I know that. But assuming you feel confident about the answer
> to that, the code changes lg.

If the resource loading should be subject to the security model of the web, e.g.
CORS, then WebAssociatedURLLoader is the right one to use. Is this basically for
the PDF viewer?

We could discuss it later. This change doesn't cause any behavior change.

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

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

[commit-bot: I haz the power, 2016-10-14 06:12:27]

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-10-14 07:49:41]

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

[commit-bot: I haz the power, 2016-10-14 07:49:42]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-10-14 13:23:14]

lgtm

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:56: DCHECK(status_ ==
LOADING);
DCHECK_EQ

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:84: DCHECK(data_length > 0);
nit: DCHECK_GT

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:92: DCHECK(data_length > 0);
ditto

[tyoshino (SeeGerritForStatus), 2016-10-14 14:08:02]

Thanks. I've applied your suggestions also to
associated_resource_fetcher_impl.cc and updated the comments in it by replacing
AssociatedURLLoader with WebAssociatedURLLoader.

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
File content/renderer/fetchers/resource_fetcher_impl.cc (right):

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:56: DCHECK(status_ ==
LOADING);
On 2016/10/14 13:23:14, kinuko (slowish) wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:84: DCHECK(data_length > 0);
On 2016/10/14 13:23:14, kinuko (slowish) wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/2399463007/diff/220001/content/renderer/fetch...
content/renderer/fetchers/resource_fetcher_impl.cc:92: DCHECK(data_length > 0);
On 2016/10/14 13:23:14, kinuko (slowish) wrote:
> ditto

Done.

[raymes, 2016-10-14 23:18:03]

lgtm

[tyoshino (SeeGerritForStatus), 2016-10-17 05:04:17]

hubbe,

Please review media/ changes. They're basically just renaming of classes and
files.

Thanks

[tyoshino (SeeGerritForStatus), 2016-10-20 00:33:23]

On 2016/10/17 05:04:17, tyoshino wrote:
> hubbe,
> 
> Please review media/ changes. They're basically just renaming of classes and
> files.
> 
> Thanks

ping

[kinuko, 2016-10-21 08:21:25]

kinuko@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[kinuko, 2016-10-21 08:22:52]

+dalecurtis@ too

Can someone in media/ stamp for this change?  I'd like this change to land!

[DaleCurtis, 2016-10-21 17:15:23]

Sorry, have been OOO, media/ lgtm

[tyoshino (SeeGerritForStatus), 2016-10-24 05:11:49]

Description was changed from

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

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

to

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium...
BUG=none
==========

[tyoshino (SeeGerritForStatus), 2016-10-24 05:11:58]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-10-24 05:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, mmenke@chromium.org, kinuko@chromium.org,
raymes@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2399463007/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-24 05:12:14]

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-10-24 06:45:20]

Description was changed from

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium...
BUG=none
==========

to

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium...
BUG=none
==========

[commit-bot: I haz the power, 2016-10-24 06:45:21]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2016-10-24 06:47:27]

Description was changed from

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium...
BUG=none
==========

to

==========
AssociatedURLLoader shouldn't derive from WebURLLoader

The AssociatedURLLoader is derived from the WebURLLoader interface while
WebURLLoaderImpl is also derived from it. WebURLLoaderImpl is an API to
use the resource dispatcher and net stack beyond it. AssociatedURLLoader
is a wrapper for the DocumentThreadableLoader logic which implements
CORS, etc. defined in the Fetch Standard. They're not as-is. Even
looking at the API, there're clear differences e.g. the
WebURLLoaderOptions is specific to the AssociatedURLLoader.

This unnecessary inheritance has been causing confusion when reading
the code. We should remove this inheritance.

R=bbudge@chromium.org,jochen@chromium.org,kinuko@chromium.org,raymes@chromium...
BUG=none

Committed: https://crrev.com/6d7671174edc71594c28a3766d641a2c1dbf47e9
Cr-Commit-Position: refs/heads/master@{#427022}
==========

[commit-bot: I haz the power, 2016-10-24 06:47:28]

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