Network service: avoid requesting a new URLLoaderFactory for each NavigationURLLoaderNetworkService.

Network service: avoid requesting a new URLLoaderFactory for each NavigationURLLoaderNetworkService.

Eventually we will need the ability to customize URL factory per frame, for things like appcache or service worker. But by default we could use a global URL factory.

BUG=717689
Review-Url: https://codereview.chromium.org/2854723002
Cr-Commit-Position: refs/heads/master@{#468776}
Committed: https://chromium.googlesource.com/chromium/src/+/fa5e572013e1c46993b8e79da9e55b58a0f2c86e

[yzshen1, 2017-05-01 22:48:59]

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

[commit-bot: I haz the power, 2017-05-01 22:49:52]

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-05-01 22:54:02]

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

[commit-bot: I haz the power, 2017-05-01 22:54:03]

Dry run: 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...)

[yzshen1, 2017-05-02 00:14:14]

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

[commit-bot: I haz the power, 2017-05-02 00:14:46]

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-05-02 01:30:57]

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

[commit-bot: I haz the power, 2017-05-02 01:30:58]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-05-02 17:54:57]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org

[yzshen1, 2017-05-02 17:54:58]

Hi, John.

Would you please take a look? Thanks!

[scottmg, 2017-05-02 18:00:55]

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

[scottmg, 2017-05-02 18:00:56]

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:208:
mojom::URLLoaderFactory*
nit; maybe make this return a & instead of a pointer, just so it's clear you'll
always get one (since it's a little confusing now that we're creating/nulling
the factory out)

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:211: // e.g.,
for appacache or service worker.
"appcache"

[Randy Smith (Not in Mondays), 2017-05-02 18:15:54]

On 2017/05/02 17:54:58, yzshen1 wrote:
> Hi, John.
> 
> Would you please take a look? Thanks!

FWIW, I believe that in the final analysis, we're going to always have
restrictions (based on frame and origin, around cookies if nothing else) in the
URLLoaderFactory, and hence want a separate URLLoaderFactory per document.  So I
wouldn't bother with code that uses a default global as I don't think it's going
to last.  I don't have strong feelings about it, though.

[yzshen1, 2017-05-02 18:30:05]

On 2017/05/02 18:15:54, Randy Smith (Not in Mondays) wrote:
> On 2017/05/02 17:54:58, yzshen1 wrote:
> > Hi, John.
> > 
> > Would you please take a look? Thanks!
> 
> FWIW, I believe that in the final analysis, we're going to always have
> restrictions (based on frame and origin, around cookies if nothing else) in
the
> URLLoaderFactory, and hence want a separate URLLoaderFactory per document.  So
I
> wouldn't bother with code that uses a default global as I don't think it's
going
> to last.  I don't have strong feelings about it, though.

Thanks for input, Randy!

I don't think this is likely to be a significant perf issue, and don't have a
strong opinion either.
Just thinking that the change is trivial and it definitely reduces IPCs.

I am fine to abandon the CL if eventually we won't use a global URLLoaderFactory
in any case.

[yzshen1, 2017-05-02 18:30:06]

On 2017/05/02 18:15:54, Randy Smith (Not in Mondays) wrote:
> On 2017/05/02 17:54:58, yzshen1 wrote:
> > Hi, John.
> > 
> > Would you please take a look? Thanks!
> 
> FWIW, I believe that in the final analysis, we're going to always have
> restrictions (based on frame and origin, around cookies if nothing else) in
the
> URLLoaderFactory, and hence want a separate URLLoaderFactory per document.  So
I
> wouldn't bother with code that uses a default global as I don't think it's
going
> to last.  I don't have strong feelings about it, though.

Thanks for input, Randy!

I don't think this is likely to be a significant perf issue, and don't have a
strong opinion either.
Just thinking that the change is trivial and it definitely reduces IPCs.

I am fine to abandon the CL if eventually we won't use a global URLLoaderFactory
in any case.

[jam, 2017-05-02 20:03:36]

lgtm

for now this is better than what we have, when we implement per-frame factories
that'll be another improvement :)

[yzshen1, 2017-05-02 20:20:14]

> for now this is better than what we have, when we implement 
> per-frame factories that'll be another improvement :)

Okay, then I will land this CL.

Thanks!

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:208:
mojom::URLLoaderFactory*
On 2017/05/02 18:00:56, scottmg wrote:
> nit; maybe make this return a & instead of a pointer, just so it's clear
you'll
> always get one (since it's a little confusing now that we're creating/nulling
> the factory out)

Done.

https://codereview.chromium.org/2854723002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:211: // e.g.,
for appacache or service worker.
On 2017/05/02 18:00:56, scottmg wrote:
> "appcache"

Done.

[yzshen1, 2017-05-02 20:20:52]

Description was changed from

==========
Network service: avoid requesting a new URLLoaderFactory for each
NavigationURLLoaderNetworkService.

Eventually we will need the ability to customize URL factory per frame, for
things like appcache or service worker. But by default we could use a global URL
factory.

BUG=598073
==========

to

==========
Network service: avoid requesting a new URLLoaderFactory for each
NavigationURLLoaderNetworkService.

Eventually we will need the ability to customize URL factory per frame, for
things like appcache or service worker. But by default we could use a global URL
factory.

BUG=717689
==========

[yzshen1, 2017-05-02 20:21:03]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-05-02 20:21:30]

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-05-02 21:24:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493756463783730,
"parent_rev": "b81e1a532d7bdfab27546d5fa6f6d4e664fc494b", "commit_rev":
"fa5e572013e1c46993b8e79da9e55b58a0f2c86e"}

[commit-bot: I haz the power, 2017-05-02 21:24:30]

Description was changed from

==========
Network service: avoid requesting a new URLLoaderFactory for each
NavigationURLLoaderNetworkService.

Eventually we will need the ability to customize URL factory per frame, for
things like appcache or service worker. But by default we could use a global URL
factory.

BUG=717689
==========

to

==========
Network service: avoid requesting a new URLLoaderFactory for each
NavigationURLLoaderNetworkService.

Eventually we will need the ability to customize URL factory per frame, for
things like appcache or service worker. But by default we could use a global URL
factory.

BUG=717689

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

[commit-bot: I haz the power, 2017-05-02 21:24:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fa5e572013e1c46993b8e79da9e5...