Add a InitializeNetworkSessionParams method IOThread to remove duplicate code.

Add a InitializeNetworkSessionParams method IOThread to remove duplicate code.

BUG=162571
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170421

[Ryan Sleevi, 2012-11-30 00:10:05]

Please see BUG below

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.cc#o...
chrome/browser/io_thread.cc:842: system_params.proxy_service =
globals_->system_proxy_service.get();
|proxy_service| is being initialized differently here then on line 506 (old),
and that means you have a BUG when this is used with line 336 (new)

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.h#ne...
chrome/browser/io_thread.h:87: net::HttpNetworkSession::Params* params) const;
Globals is already a public object, so is there a reason this interface needs to
leak into the header, as opposed to being an internal method in an unnamed
namespace?

To put it differently: Is there anyone, beyond this .cc file, who would expect
to use this?

[Ryan Hamilton, 2012-11-30 00:17:59]

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.cc#o...
chrome/browser/io_thread.cc:842: system_params.proxy_service =
globals_->system_proxy_service.get();
On 2012/11/30 00:10:05, Ryan Sleevi wrote:
> |proxy_service| is being initialized differently here then on line 506 (old),
> and that means you have a BUG when this is used with line 336 (new)

DOH!  Thanks for the catch.

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11419243/diff/4/chrome/browser/io_thread.h#ne...
chrome/browser/io_thread.h:87: net::HttpNetworkSession::Params* params) const;
On 2012/11/30 00:10:05, Ryan Sleevi wrote:
> Globals is already a public object, so is there a reason this interface needs
to
> leak into the header, as opposed to being an internal method in an unnamed
> namespace?
> 
> To put it differently: Is there anyone, beyond this .cc file, who would expect
> to use this?

I would only expect the .cc file to call this method.  That being said, if I
don't make this a method of Globals, and instead made it a method in an unnamed
namespace, it would require sticking "globals." onto each one of the
assignments.  *yuck*  I much prefer the shorter lines.  But if you feel
strongly, I'll move it.

[Ryan Sleevi, 2012-11-30 00:30:16]

Because Globals is a public struct, having this public method like this seems to
violate the C++ Guide (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...
- "Methods should not provide behavior but should only be used to set up the
data members"), but also 'leak' the design out to a lot of callers.

I'd say the typing is worth the ability to hide its visibility.

[Ryan Hamilton, 2012-11-30 00:50:47]

On 2012/11/30 00:30:16, Ryan Sleevi wrote:
> Because Globals is a public struct, having this public method like this seems
to
> violate the C++ Guide (
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...
> - "Methods should not provide behavior but should only be used to set up the
> data members"), but also 'leak' the design out to a lot of callers.
> 
> I'd say the typing is worth the ability to hide its visibility.

Done.

[Ryan Sleevi, 2012-11-30 00:57:28]

lgtm

https://codereview.chromium.org/11419243/diff/7/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11419243/diff/7/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:524:
globals_->proxy_script_fetcher_proxy_service.get());
I'm ambivalent about the net_log/proxy_service change - the old way may have
been a little clearer.

session_params is getting modified, which suggests it should be treated as an
in-out param (eg: appear after all the pure read-only params), but that may be
splitting hairs.

[Ryan Hamilton, 2012-11-30 01:16:52]

On 2012/11/30 00:57:28, Ryan Sleevi wrote:
> lgtm
> 
> https://codereview.chromium.org/11419243/diff/7/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/11419243/diff/7/chrome/browser/io_thread.cc#n...
> chrome/browser/io_thread.cc:524:
> globals_->proxy_script_fetcher_proxy_service.get());
> I'm ambivalent about the net_log/proxy_service change - the old way may have
> been a little clearer.

Reverted.

> session_params is getting modified, which suggests it should be treated as an
> in-out param (eg: appear after all the pure read-only params), but that may be
> splitting hairs.

Reverted :>

[Ryan Hamilton, 2012-11-30 01:19:31]

thakis: One more review for you.  (only 1 file :>)

[Nico, 2012-11-30 01:39:03]

On 2012/11/30 01:19:31, Ryan Hamilton wrote:
> thakis: One more review for you.  (only 1 file :>)

Only one lgtm as well then.

[commit-bot: I haz the power, 2012-11-30 05:21:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11419243/1006

[commit-bot: I haz the power, 2012-11-30 08:18:14]

Change committed as 170421