Remove dispatchWillSendRequest from WebFrame

Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=694904
Review-Url: https://codereview.chromium.org/2701333004
Cr-Commit-Position: refs/heads/master@{#451963}
Committed: https://chromium.googlesource.com/chromium/src/+/047e3c9956a25ff9ed0391f550f337842cb3e7d6

[kinuko, 2017-02-20 15:08:53]

Description was changed from

==========
Remove dispatchWillSendRequest from ResourceFetcherImpl

BUG=
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

BUG=
==========

[kinuko, 2017-02-20 15:08:59]

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

[kinuko, 2017-02-20 15:09:04]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-20 15:09: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-02-20 15:20:18]

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

[commit-bot: I haz the power, 2017-02-20 15:20:19]

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

[kinuko, 2017-02-21 00:18:04]

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

[commit-bot: I haz the power, 2017-02-21 00:18:14]

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-02-21 00:32:21]

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

[commit-bot: I haz the power, 2017-02-21 00:32:22]

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

[kinuko, 2017-02-21 01:29:02]

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

[commit-bot: I haz the power, 2017-02-21 01:29:17]

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-02-21 02:42:49]

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

[commit-bot: I haz the power, 2017-02-21 02:42:49]

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_...)

[kinuko, 2017-02-21 04:04:16]

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

[commit-bot: I haz the power, 2017-02-21 04:04:28]

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-02-21 05:07:53]

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

[commit-bot: I haz the power, 2017-02-21 05:07:54]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-21 05:22:30]

Description was changed from

==========
Remove dispatchWillSendRequest from WebFrame

BUG=
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.

BUG=
==========

[kinuko, 2017-02-21 05:22:40]

Patchset #2 (id:40001) has been deleted

[kinuko, 2017-02-21 05:23:19]

Description was changed from

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.

BUG=
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are NetErrorHelper and MojoContextState)

BUG=
==========

[kinuko, 2017-02-21 05:23:47]

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

[kinuko, 2017-02-21 05:25:32]

I think we could possibly remove this plumbing but not 100% sure. Wdyt?

[kinuko, 2017-02-21 05:32:57]

Description was changed from

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are NetErrorHelper and MojoContextState)

BUG=
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=
==========

[kinuko, 2017-02-21 06:45:02]

On 2017/02/21 05:25:32, kinuko wrote:
> I think we could possibly remove this plumbing but not 100% sure. Wdyt?

One more note: I feel no one can be probably sure, and I wonder if we could just
land this and see if it breaks something. (I can remove the WebFrame interface
changes from this CL for easier revert)

[tyoshino (SeeGerritForStatus), 2017-02-21 06:53:54]

On 2017/02/21 06:45:02, kinuko wrote:
> On 2017/02/21 05:25:32, kinuko wrote:
> > I think we could possibly remove this plumbing but not 100% sure. Wdyt?
> 
> One more note: I feel no one can be probably sure, and I wonder if we could
just
> land this and see if it breaks something. (I can remove the WebFrame interface
> changes from this CL for easier revert)

I basically agree that it's weird that ResourceFetcherImpl is doing something
with the frame. Especially, I don't know much how Gin (Gin's AMD is using the
path) is used recently. Also, given that we've been with the plumbing, I'm not
so sure that this won't break anything. But current situation is also not good.
Things in RenderFrameImpl::willSendRequest() that need to be coordinated with
Blink side's logic are not actually coordinated.

Removal from AssociatedResourceFetcherImpl is lg, sure.

lgtm for trying this.

[Nate Chapin, 2017-02-21 19:19:37]

On 2017/02/21 06:53:54, tyoshino wrote:
> On 2017/02/21 06:45:02, kinuko wrote:
> > On 2017/02/21 05:25:32, kinuko wrote:
> > > I think we could possibly remove this plumbing but not 100% sure. Wdyt?
> > 
> > One more note: I feel no one can be probably sure, and I wonder if we could
> just
> > land this and see if it breaks something. (I can remove the WebFrame
interface
> > changes from this CL for easier revert)
> 
> I basically agree that it's weird that ResourceFetcherImpl is doing something
> with the frame. Especially, I don't know much how Gin (Gin's AMD is using the
> path) is used recently. Also, given that we've been with the plumbing, I'm not
> so sure that this won't break anything. But current situation is also not
good.
> Things in RenderFrameImpl::willSendRequest() that need to be coordinated with
> Blink side's logic are not actually coordinated.
> 
> Removal from AssociatedResourceFetcherImpl is lg, sure.
> 
> lgtm for trying this.

lgtm2 to try. If something in ResourceFetcherImpl breaks, let's understand why
and resolve that issue. For that matter, why do we have separate interfaces for
doing a pseudo-blink load, with and without AssociatedURLLoader? I don't know
the history behind ResourceFetcherImpl, but it really seems like it should
either be using AssociatedURLLoader or not talking to blink at all.

[kinuko, 2017-02-22 05:21:21]

Description was changed from

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=694904
==========

[kinuko, 2017-02-22 05:23:26]

On 2017/02/21 19:19:37, Nate Chapin wrote:
> On 2017/02/21 06:53:54, tyoshino wrote:
> > On 2017/02/21 06:45:02, kinuko wrote:
> > > On 2017/02/21 05:25:32, kinuko wrote:
> > > > I think we could possibly remove this plumbing but not 100% sure. Wdyt?
> > > 
> > > One more note: I feel no one can be probably sure, and I wonder if we
could
> > just
> > > land this and see if it breaks something. (I can remove the WebFrame
> interface
> > > changes from this CL for easier revert)
> > 
> > I basically agree that it's weird that ResourceFetcherImpl is doing
something
> > with the frame. Especially, I don't know much how Gin (Gin's AMD is using
the
> > path) is used recently. Also, given that we've been with the plumbing, I'm
not
> > so sure that this won't break anything. But current situation is also not
> good.
> > Things in RenderFrameImpl::willSendRequest() that need to be coordinated
with
> > Blink side's logic are not actually coordinated.
> > 
> > Removal from AssociatedResourceFetcherImpl is lg, sure.
> > 
> > lgtm for trying this.
> 
> lgtm2 to try. If something in ResourceFetcherImpl breaks, let's understand why
> and resolve that issue. For that matter, why do we have separate interfaces
for
> doing a pseudo-blink load, with and without AssociatedURLLoader? I don't know
> the history behind ResourceFetcherImpl, but it really seems like it should
> either be using AssociatedURLLoader or not talking to blink at all.

Thanks, filed a bug (crbug.com/694904) for tracking, and will be landing.

[tyoshino (SeeGerritForStatus), 2017-02-22 05:27:05]

On 2017/02/21 19:19:37, Nate Chapin wrote:
> On 2017/02/21 06:53:54, tyoshino wrote:
> > On 2017/02/21 06:45:02, kinuko wrote:
> > > On 2017/02/21 05:25:32, kinuko wrote:
> > > > I think we could possibly remove this plumbing but not 100% sure. Wdyt?
> > > 
> > > One more note: I feel no one can be probably sure, and I wonder if we
could
> > just
> > > land this and see if it breaks something. (I can remove the WebFrame
> interface
> > > changes from this CL for easier revert)
> > 
> > I basically agree that it's weird that ResourceFetcherImpl is doing
something
> > with the frame. Especially, I don't know much how Gin (Gin's AMD is using
the
> > path) is used recently. Also, given that we've been with the plumbing, I'm
not
> > so sure that this won't break anything. But current situation is also not
> good.
> > Things in RenderFrameImpl::willSendRequest() that need to be coordinated
with
> > Blink side's logic are not actually coordinated.
> > 
> > Removal from AssociatedResourceFetcherImpl is lg, sure.
> > 
> > lgtm for trying this.
> 
> lgtm2 to try. If something in ResourceFetcherImpl breaks, let's understand why
> and resolve that issue. For that matter, why do we have separate interfaces
for
> doing a pseudo-blink load, with and without AssociatedURLLoader? I don't know
> the history behind ResourceFetcherImpl, but it really seems like it should
> either be using AssociatedURLLoader or not talking to blink at all.

Before this clean up by me https://codereview.chromium.org/2399463007 they were
more conflated. Maybe the strange stuffs are results of that.

[kinuko, 2017-02-22 05:36:30]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:37:53]

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-02-22 06:03:58]

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

[commit-bot: I haz the power, 2017-02-22 06:03:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kinuko, 2017-02-22 06:48:50]

On 2017/02/21 19:19:37, Nate Chapin wrote:
> On 2017/02/21 06:53:54, tyoshino wrote:
> > On 2017/02/21 06:45:02, kinuko wrote:
> > > On 2017/02/21 05:25:32, kinuko wrote:
> > > > I think we could possibly remove this plumbing but not 100% sure. Wdyt?
> > > 
> > > One more note: I feel no one can be probably sure, and I wonder if we
could
> > just
> > > land this and see if it breaks something. (I can remove the WebFrame
> interface
> > > changes from this CL for easier revert)
> > 
> > I basically agree that it's weird that ResourceFetcherImpl is doing
something
> > with the frame. Especially, I don't know much how Gin (Gin's AMD is using
the
> > path) is used recently. Also, given that we've been with the plumbing, I'm
not
> > so sure that this won't break anything. But current situation is also not
> good.
> > Things in RenderFrameImpl::willSendRequest() that need to be coordinated
with
> > Blink side's logic are not actually coordinated.
> > 
> > Removal from AssociatedResourceFetcherImpl is lg, sure.
> > 
> > lgtm for trying this.
> 
> lgtm2 to try. If something in ResourceFetcherImpl breaks, let's understand why
> and resolve that issue. For that matter, why do we have separate interfaces
for
> doing a pseudo-blink load, with and without AssociatedURLLoader? I don't know
> the history behind ResourceFetcherImpl, but it really seems like it should
> either be using AssociatedURLLoader or not talking to blink at all.

Nate, I think I need your stamp without '2' part in the same token :)

[kinuko, 2017-02-22 08:12:34]

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

[kinuko, 2017-02-22 08:12:45]

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

[commit-bot: I haz the power, 2017-02-22 08:13:17]

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

[kinuko, 2017-02-22 08:13:34]

On 2017/02/22 06:48:50, kinuko wrote:
> On 2017/02/21 19:19:37, Nate Chapin wrote:
> > lgtm2 to try. If something in ResourceFetcherImpl breaks, let's understand
why
> > and resolve that issue. For that matter, why do we have separate interfaces
> for
> > doing a pseudo-blink load, with and without AssociatedURLLoader? I don't
know
> > the history behind ResourceFetcherImpl, but it really seems like it should
> > either be using AssociatedURLLoader or not talking to blink at all.
> 
> Nate, I think I need your stamp without '2' part in the same token :)

Daniel- might you be able to stamp this (or have more comments on this)?

[dcheng, 2017-02-22 08:20:37]

rubber stamp lgtm based on japhet's review

[kinuko, 2017-02-22 08:23:21]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2017-02-22 08:23:33]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-02-22 08:23: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-02-22 10:01:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487751813431010,
"parent_rev": "5a5c943ccde6a22137a1b9e23a4d334a26b8633b", "commit_rev":
"047e3c9956a25ff9ed0391f550f337842cb3e7d6"}

[commit-bot: I haz the power, 2017-02-22 10:03:45]

Description was changed from

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=694904
==========

to

==========
Remove dispatchWillSendRequest from WebFrame

Customers of AssociatedURLLoader do not need this because
AssociatedURLLoader internally dispatches it.

I think customers of Platform WebURLLoader should not need this
because if it needs to use it it should be using AssociatedURLLoader.
(Current customers are only two: NetErrorHelper and MojoContextState)

BUG=694904

Review-Url: https://codereview.chromium.org/2701333004
Cr-Commit-Position: refs/heads/master@{#451963}
Committed:
https://chromium.googlesource.com/chromium/src/+/047e3c9956a25ff9ed0391f550f3...
==========

[commit-bot: I haz the power, 2017-02-22 10:03:47]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/047e3c9956a25ff9ed0391f550f3...