Upstreaming SpdyProxy-related switches, OTR logic, and histograms

Upstreaming SpdyProxy-related switches, OTR logic, and histograms

BUG=149055, 149038
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160200

[bengr (incorrect), 2012-09-28 17:52:49]

This replaces https://codereview.chromium.org/10910260/ and
https://codereview.chromium.org/10936002/.

I'm hoping that either sky@, thakis@, or thestig@ can review:
-- chrome_content_browser_client.cc
-- switches.{h,cc}
-- prefnames.{h,cc},

that sky@ or pkasting@ can review:
-- prefs_tab_helper.cc, 

and that rlp@ or mirandac@ can review:
-- off_the_record_profile_impl.cc
-- profile.cc

[rpetterson, 2012-09-28 18:10:58]

https://codereview.chromium.org/10985083/diff/1/chrome/browser/profiles/off_t...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/10985083/diff/1/chrome/browser/profiles/off_t...
chrome/browser/profiles/off_the_record_profile_impl.cc:91: #if
defined(OS_ANDROID)
Is it possible to group these into a single function which you call here
(preferred) or at the very least put in a comment as to what/why you're doing to
make it clearer what's going on?

[Lei Zhang, 2012-09-28 21:12:51]

Can |kSpdyProxyEnabled| go in a separate CL?

[bengr (incorrect), 2012-09-28 21:45:21]

On 2012/09/28 21:12:51, Lei Zhang wrote:
> Can |kSpdyProxyEnabled| go in a separate CL?

Would you like me to pull out pref_names.{h/cc} only, or should I also pull out
profile.cc (the one reference to it)?

[Lei Zhang, 2012-09-28 21:54:56]

On 2012/09/28 21:45:21, bengr wrote:
> On 2012/09/28 21:12:51, Lei Zhang wrote:
> > Can |kSpdyProxyEnabled| go in a separate CL?
> 
> Would you like me to pull out pref_names.{h/cc} only, or should I also pull
out
> profile.cc (the one reference to it)?

Well, profile.cc can't compile without pref_names.*, so all 3 files. Assuming
the code that sets / reads |kSpdyProxyEnabled| can stand on its own, you should
put it in its own CL and not mangle it with |kSpdyProxyOrigin|.

[Peter Kasting, 2012-09-28 22:57:03]

Rubber-stamp LGTM for prefs_tab_helper.cc

[Nico, 2012-10-01 00:47:04]

Sounds like thestig is on it. Removing myself.

[Miranda Callahan, 2012-10-01 12:59:21]

On 2012/10/01 00:47:04, Nico wrote:
> Sounds like thestig is on it. Removing myself.

Removing myself as Rachel has started reviewing.

[bengr (incorrect), 2012-10-01 18:52:45]

https://codereview.chromium.org/10985083/diff/1/chrome/browser/profiles/off_t...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/10985083/diff/1/chrome/browser/profiles/off_t...
chrome/browser/profiles/off_the_record_profile_impl.cc:91: #if
defined(OS_ANDROID)
On 2012/09/28 18:10:58, rpetterson wrote:
> Is it possible to group these into a single function which you call here
> (preferred) or at the very least put in a comment as to what/why you're doing
to
> make it clearer what's going on?

Done.

[rpetterson, 2012-10-01 20:40:15]

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:168: void
OffTheRecordProfileImpl::UseSystemProxy() {
I like this better, but I think it should be defined in an separate file as
static function (pass in the prefs since that's all you need) in order to
eliminate more of the #if defined(OS_ANDROID).

[bengr (incorrect), 2012-10-01 22:38:06]

Shall I create a new file in chrome/browser/profiles? If not, do you have a
suggestion as to where I should put it? I know of no file in chromium that
contains such android-specific utilities. There is
chrome/browser/platform_util_android.{h/cc}, but the functions in that file
are UI-focused.


On Mon, Oct 1, 2012 at 1:40 PM, <rlp@chromium.org> wrote:

>
> https://codereview.chromium.**org/10985083/diff/11007/**
>
chrome/browser/profiles/off_**the_record_profile_impl.cc<https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/off_the_record_profile_impl.cc>
> File chrome/browser/profiles/off_**the_record_profile_impl.cc (right):
>
> https://codereview.chromium.**org/10985083/diff/11007/**
>
chrome/browser/profiles/off_**the_record_profile_impl.cc#**newcode168<https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/off_the_record_profile_impl.cc#newcode168>
> chrome/browser/profiles/off_**the_record_profile_impl.cc:**168: void
> OffTheRecordProfileImpl::**UseSystemProxy() {
> I like this better, but I think it should be defined in an separate file
> as static function (pass in the prefs since that's all you need) in
> order to eliminate more of the #if defined(OS_ANDROID).
>
>
https://codereview.chromium.**org/10985083/<https://codereview.chromium.org/1...
>

[rpetterson, 2012-10-01 23:33:52]

On 2012/10/01 22:38:06, bengr wrote:
> Shall I create a new file in chrome/browser/profiles? If not, do you have a
> suggestion as to where I should put it? I know of no file in chromium that
> contains such android-specific utilities. There is
> chrome/browser/platform_util_android.{h/cc}, but the functions in that file
> are UI-focused.

I'd say if you expect there to be similar functions in the future (that is,
android specific profile stuff), then yes, please create a new file. If this is
a one off then it's fine and it has my LGTM.

[bengr (incorrect), 2012-10-01 23:45:41]

I believe it's a one off. Thanks.


On Mon, Oct 1, 2012 at 4:33 PM, <rlp@chromium.org> wrote:

> On 2012/10/01 22:38:06, bengr wrote:
>
>> Shall I create a new file in chrome/browser/profiles? If not, do you have
>> a
>> suggestion as to where I should put it? I know of no file in chromium that
>> contains such android-specific utilities. There is
>> chrome/browser/platform_util_**android.{h/cc}, but the functions in that
>> file
>> are UI-focused.
>>
>
> I'd say if you expect there to be similar functions in the future (that is,
> android specific profile stuff), then yes, please create a new file. If
> this is
> a one off then it's fine and it has my LGTM.
>
>
https://codereview.chromium.**org/10985083/<https://codereview.chromium.org/1...
>

[bengr (incorrect), 2012-10-02 00:12:13]

Ok, if my accounting is correct, I believe I still need sky@ or thestig@ to LGTM
content_browser_client.cc and pkasting@ to LGTM page_load_histograms.cc. Thanks!


On 2012/10/01 23:45:41, bengr wrote:
> I believe it's a one off. Thanks.
> 
> 
> On Mon, Oct 1, 2012 at 4:33 PM, <mailto:rlp@chromium.org> wrote:
> 
> > On 2012/10/01 22:38:06, bengr wrote:
> >
> >> Shall I create a new file in chrome/browser/profiles? If not, do you have
> >> a
> >> suggestion as to where I should put it? I know of no file in chromium that
> >> contains such android-specific utilities. There is
> >> chrome/browser/platform_util_**android.{h/cc}, but the functions in that
> >> file
> >> are UI-focused.
> >>
> >
> > I'd say if you expect there to be similar functions in the future (that is,
> > android specific profile stuff), then yes, please create a new file. If
> > this is
> > a one off then it's fine and it has my LGTM.
> >
> >
>
https://codereview.chromium.**org/10985083/%3Chttps://codereview.chromium.org...>
> >

[sky, 2012-10-02 00:20:33]

LGTM

[Lei Zhang, 2012-10-02 00:25:14]

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:935: switches::kSpdyProxyOrigin,
Do utility processes really need this switch?

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:92: UseSystemProxy();
I'm not that familiar with OTR profiles. Does this really need to go here or
should it go in Init() immediately below?

[bengr (incorrect), 2012-10-02 18:19:00]

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:935: switches::kSpdyProxyOrigin,
I don't believe so. Removed.

On 2012/10/02 00:25:14, Lei Zhang wrote:
> Do utility processes really need this switch?

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/10985083/diff/11007/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:92: UseSystemProxy();
On 2012/10/02 00:25:14, Lei Zhang wrote:
> I'm not that familiar with OTR profiles. Does this really need to go here or
> should it go in Init() immediately below?

Done.

[Lei Zhang, 2012-10-02 18:24:45]

lgtm++

[bengr (incorrect), 2012-10-02 18:32:27]

Peter, I'm down to needing an LGTM for chrome/renderer/page_load_histograms.cc.
Would you mind taking a look? Thanks!

[bengr (incorrect), 2012-10-03 00:03:48]

Jim, would you mind taking a look at chrome/renderer/page_load_histograms.cc? I
have LGTMs for the rest of the CL. Thanks!

[jar (doing other things), 2012-10-04 17:49:24]

On 2012/10/03 00:03:48, bengr wrote:
> Jim, would you mind taking a look at chrome/renderer/page_load_histograms.cc?
I
> have L G T Ms for the rest of the CL. Thanks!

The change looks good (in page_load_histograms.cc).... is there a reason why
this is only valuable to Android?

[bengr (incorrect), 2012-10-04 18:11:31]

I've removed the #if defined(OS_ANDROID) from the SpdyProxy histograms.

[commit-bot: I haz the power, 2012-10-04 18:14:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@google.com/10985083/14002

[commit-bot: I haz the power, 2012-10-04 19:56:32]

Change committed as 160200