Make HttpNetworkSession::host_mapping_rules no longer a pointer.

Make HttpNetworkSession::host_mapping_rules no longer a pointer.

TBR=dimich@chromium.org
BUG=725653
Review-Url: https://codereview.chromium.org/2906463002
Cr-Commit-Position: refs/heads/master@{#475111}
Committed: https://chromium.googlesource.com/chromium/src/+/0d700dd8b90fd715b2355ad69e5fadfd4fa9eb27

[mmenke, 2017-05-24 17:07:48]

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

[commit-bot: I haz the power, 2017-05-24 17:08:21]

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-24 17:43:43]

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

[commit-bot: I haz the power, 2017-05-24 17:43:44]

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

[mmenke, 2017-05-24 17:59:49]

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

[commit-bot: I haz the power, 2017-05-24 18:00:36]

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-24 18:13:08]

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

[commit-bot: I haz the power, 2017-05-24 18:13:09]

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-05-24 18:15:28]

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

[commit-bot: I haz the power, 2017-05-24 18:16:07]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-05-24 19:12:03]

Description was changed from

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

BUG=715695
==========

to

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

BUG=725653
==========

[mmenke, 2017-05-24 19:13:58]

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

[mmenke, 2017-05-24 19:13:59]

Next CL will be one that splits HttpNetworkSession in two, and will be pretty
big because of all the fallout that will cause in test fixtures.

The last CL I sent you I still haven't landed, and won't land until after branch
(Decided to rebase it on top of the CL that response the
ProxyScriptFetchingURLRequestContext, since it needed some extra shutdown logic
that CL already has, and don't want to land that one in the week of a branch)

[commit-bot: I haz the power, 2017-05-24 19:58:26]

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

[commit-bot: I haz the power, 2017-05-24 19:58:27]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-05-25 17:50:49]

LGTM unless you think there's a problem with the possible behavior change.

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:649: new
net::StaticHttpUserAgentSettings(std::string(), GetUserAgent()));
Why move this line?  (I don't care, I'm just worried there's a data dependency
I'm not seeing.)

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
File net/base/host_mapping_rules.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
net/base/host_mapping_rules.cc:41: 
The movement of the exclusion check looks like a behavior change to me, though I
presume the mapping rules are just used for testing (?) so we're free to make
that change if we want.  Is it not a behavior change/what was the motivation for
it if it was?

https://codereview.chromium.org/2906463002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2906463002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:899: if
(mapping_rules->RewriteHost(endpoint)) {
nit, suggestion: This might all fit on one line without the extra variable. 
Less usefully, the variable could be a reference (I'm not sure that's even a
suggestion, just an alternative.)

[mmenke, 2017-05-25 18:07:10]

Randy:  Am I missing something on the behavior change?  Logic looks the same to
me.  I can revert that change if needed, without remorse, but I'd like to
understand if I made some mistake.

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:649: new
net::StaticHttpUserAgentSettings(std::string(), GetUserAgent()));
On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> Why move this line?  (I don't care, I'm just worried there's a data dependency
> I'm not seeing.)

Purely for code hygiene resions.  It's weird to have:

I moved UserAgentSettings because it's weird to have:

<Do some HostMapping setup>
<Set up UserAgentSettings>
<Finish HostMapping setup>

I moved params_.host_mapping_rules initialization because I had to - before, it
was copying a pointer, so it was fine to configure
globals_->host_mapping_rules.get() after setting params_.host_mapping_rules. 
However, now that we're copying the contents of the rules instead of just using
a pointer to them, the global one ha to be configured first.

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
File net/base/host_mapping_rules.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
net/base/host_mapping_rules.cc:41: 
On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> The movement of the exclusion check looks like a behavior change to me, though
I
> presume the mapping rules are just used for testing (?) so we're free to make
> that change if we want.  Is it not a behavior change/what was the motivation
for
> it if it was?

I don't think this is a behavior change.  This was just done for very modest
perf reasons - I was concerned we'd go from never running this code, since the
rules was nullptr, to always using it.  Which shouldn't matter, but I'm paranoid
(Turns out it's generally not nullptr, anyways, except in CroNet, or other
implementations).

Code was basically:

if (<exclude>)
  return false;

if (<include>) {
  DoStuff();
  return true;
}

return false;

It's now:

if (<include>) {
  if (<exclude>)
    return false;
  DoStuff();
  return true;
}

return false;

So we do less when there are no lists, and when there are lists, we only check
the exclude list if there's an include list hit.  Exclude list I assume would
generally be more specific, so in both cases, it's generally a modest per
improvement.  Or am I missing something?  I can certainly go back to the old
behavior without much concern, just wanted to minimize any impact on perf
(Which, when there are no patterns, would no doubt be trivial anyways).

https://codereview.chromium.org/2906463002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2906463002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:899: if
(mapping_rules->RewriteHost(endpoint)) {
On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> nit, suggestion: This might all fit on one line without the extra variable. 
> Less usefully, the variable could be a reference (I'm not sure that's even a
> suggestion, just an alternative.)

Done.

[Randy Smith (Not in Mondays), 2017-05-25 18:17:06]

Still LGTM; sorry about the confusion around whether or not there was a behavior
change.

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:649: new
net::StaticHttpUserAgentSettings(std::string(), GetUserAgent()));
On 2017/05/25 18:07:10, mmenke wrote:
> On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> > Why move this line?  (I don't care, I'm just worried there's a data
dependency
> > I'm not seeing.)
> 
> Purely for code hygiene resions.  It's weird to have:
> 
> I moved UserAgentSettings because it's weird to have:
> 
> <Do some HostMapping setup>
> <Set up UserAgentSettings>
> <Finish HostMapping setup>
> 
> I moved params_.host_mapping_rules initialization because I had to - before,
it
> was copying a pointer, so it was fine to configure
> globals_->host_mapping_rules.get() after setting params_.host_mapping_rules. 
> However, now that we're copying the contents of the rules instead of just
using
> a pointer to them, the global one ha to be configured first.

Understood; that's fine. 

(I understood moving the host mapping rules; it was the user agent stuff I
wanted to make sure I wasn't missing anything on.)

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
File net/base/host_mapping_rules.cc (left):

https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
net/base/host_mapping_rules.cc:41: 
On 2017/05/25 18:07:10, mmenke wrote:
> On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> > The movement of the exclusion check looks like a behavior change to me,
though
> I
> > presume the mapping rules are just used for testing (?) so we're free to
make
> > that change if we want.  Is it not a behavior change/what was the motivation
> for
> > it if it was?
> 
> I don't think this is a behavior change.  This was just done for very modest
> perf reasons - I was concerned we'd go from never running this code, since the
> rules was nullptr, to always using it.  Which shouldn't matter, but I'm
paranoid
> (Turns out it's generally not nullptr, anyways, except in CroNet, or other
> implementations).
> 
> Code was basically:
> 
> if (<exclude>)
>   return false;
> 
> if (<include>) {
>   DoStuff();
>   return true;
> }
> 
> return false;
> 
> It's now:
> 
> if (<include>) {
>   if (<exclude>)
>     return false;
>   DoStuff();
>   return true;
> }
> 
> return false;
> 
> So we do less when there are no lists, and when there are lists, we only check
> the exclude list if there's an include list hit.  Exclude list I assume would
> generally be more specific, so in both cases, it's generally a modest per
> improvement.  Or am I missing something?  I can certainly go back to the old
> behavior without much concern, just wanted to minimize any impact on perf
> (Which, when there are no patterns, would no doubt be trivial anyways).

I'm sorry, I was reading too fast, and read this as an exchange of position, not
a moving of one check inside another.  Comment retracted.

[mmenke, 2017-05-25 18:17:15]

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

[commit-bot: I haz the power, 2017-05-25 18:18:17]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-05-25 18:20:26]

On 2017/05/25 18:17:06, Randy Smith (Not in Mondays) wrote:
> Still LGTM; sorry about the confusion around whether or not there was a
behavior
> change.
> 
>
https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
> File chrome/browser/io_thread.cc (left):
> 
>
https://codereview.chromium.org/2906463002/diff/40001/chrome/browser/io_threa...
> chrome/browser/io_thread.cc:649: new
> net::StaticHttpUserAgentSettings(std::string(), GetUserAgent()));
> On 2017/05/25 18:07:10, mmenke wrote:
> > On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> > > Why move this line?  (I don't care, I'm just worried there's a data
> dependency
> > > I'm not seeing.)
> > 
> > Purely for code hygiene resions.  It's weird to have:
> > 
> > I moved UserAgentSettings because it's weird to have:
> > 
> > <Do some HostMapping setup>
> > <Set up UserAgentSettings>
> > <Finish HostMapping setup>
> > 
> > I moved params_.host_mapping_rules initialization because I had to - before,
> it
> > was copying a pointer, so it was fine to configure
> > globals_->host_mapping_rules.get() after setting params_.host_mapping_rules.

> > However, now that we're copying the contents of the rules instead of just
> using
> > a pointer to them, the global one ha to be configured first.
> 
> Understood; that's fine. 
> 
> (I understood moving the host mapping rules; it was the user agent stuff I
> wanted to make sure I wasn't missing anything on.)
> 
>
https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
> File net/base/host_mapping_rules.cc (left):
> 
>
https://codereview.chromium.org/2906463002/diff/40001/net/base/host_mapping_r...
> net/base/host_mapping_rules.cc:41: 
> On 2017/05/25 18:07:10, mmenke wrote:
> > On 2017/05/25 17:50:48, Randy Smith (Not in Mondays) wrote:
> > > The movement of the exclusion check looks like a behavior change to me,
> though
> > I
> > > presume the mapping rules are just used for testing (?) so we're free to
> make
> > > that change if we want.  Is it not a behavior change/what was the
motivation
> > for
> > > it if it was?
> > 
> > I don't think this is a behavior change.  This was just done for very modest
> > perf reasons - I was concerned we'd go from never running this code, since
the
> > rules was nullptr, to always using it.  Which shouldn't matter, but I'm
> paranoid
> > (Turns out it's generally not nullptr, anyways, except in CroNet, or other
> > implementations).
> > 
> > Code was basically:
> > 
> > if (<exclude>)
> >   return false;
> > 
> > if (<include>) {
> >   DoStuff();
> >   return true;
> > }
> > 
> > return false;
> > 
> > It's now:
> > 
> > if (<include>) {
> >   if (<exclude>)
> >     return false;
> >   DoStuff();
> >   return true;
> > }
> > 
> > return false;
> > 
> > So we do less when there are no lists, and when there are lists, we only
check
> > the exclude list if there's an include list hit.  Exclude list I assume
would
> > generally be more specific, so in both cases, it's generally a modest per
> > improvement.  Or am I missing something?  I can certainly go back to the old
> > behavior without much concern, just wanted to minimize any impact on perf
> > (Which, when there are no patterns, would no doubt be trivial anyways).
> 
> I'm sorry, I was reading too fast, and read this as an exchange of position,
not
> a moving of one check inside another.  Comment retracted.

Thanks!  Not a problem - much prefer the occasional incorrect comment to having
a real bug or code health issue missed.

[mmenke, 2017-05-25 18:20:32]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-05-25 18:20:36]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-05-25 18:20:46]

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-25 18:33:03]

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

[commit-bot: I haz the power, 2017-05-25 18:33:04]

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

[mmenke, 2017-05-25 18:47:03]

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

[mmenke, 2017-05-25 18:47:03]

[+dimich]  Please review google_apis/gcm/tools/mcs_probe.cc.

I'll probably switch that over to URLRequestContextBuilder at some point, which
would make URLRequestContext much simpler, and means net refactors don't need to
touch that code.

[mmenke, 2017-05-25 19:26:04]

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

[commit-bot: I haz the power, 2017-05-25 19:26:44]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-05-26 18:06:49]

dimich:  May just TBR you on this - I just removed something that was
initialized to its default value in McsProbe.  Have a bunch of CLs on top of
this that I want to start landing Tuesday.

[mmenke, 2017-05-26 18:08:12]

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

[commit-bot: I haz the power, 2017-05-26 18:09:01]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-05-26 20:02:13]

Description was changed from

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

BUG=725653
==========

to

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

TBR=dimich@chromium.org
BUG=725653
==========

[mmenke, 2017-05-26 20:02:22]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-05-26 20:02:31]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-26 20:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2906463002/#ps140001
(title: "More upstream merge conflicts! Fun!")

[commit-bot: I haz the power, 2017-05-26 20:03:12]

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-26 20:36:14]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495828951447870,
"parent_rev": "1635d1c9793a83ecbe0c8333c7f0c4beb115034e", "commit_rev":
"0d700dd8b90fd715b2355ad69e5fadfd4fa9eb27"}

[commit-bot: I haz the power, 2017-05-26 20:36:26]

Description was changed from

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

TBR=dimich@chromium.org
BUG=725653
==========

to

==========
Make HttpNetworkSession::host_mapping_rules no longer a pointer.

TBR=dimich@chromium.org
BUG=725653

Review-Url: https://codereview.chromium.org/2906463002
Cr-Commit-Position: refs/heads/master@{#475111}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d700dd8b90fd715b2355ad69e5f...
==========

[commit-bot: I haz the power, 2017-05-26 20:36:28]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0d700dd8b90fd715b2355ad69e5f...