Transmit a X-Chrome-UMA-Enabled bit to Google domains from clients that have UMA enabled.

Transmit a X-Chrome-UMA-Enabled bit to Google domains from clients that have UMA enabled.

This requires a change to the ChromeNetworkDelegate where we feed the incognito state (a bool) into the object at creation time, so we can check that field when doing our header setting.

BUG=123609
TEST=With UMA enabled (not Chromium), visit www.google.com and ensure that the request header includes X-Chrome-UMA-Enabled with value "1". Ensure that disabling UMA also disables the transmission of this header entirely.  Also, ensure that unit_tests GoogleUtilTests all pass.
TBR=ivankr@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134625

[SteveT, 2012-04-17 17:48:33]

Fixes made as discussed in the other thread: We now avoid use of profile_ by
passing in the is_incognito bool at ChromeNetworkDelegate creation time.

PTAL:
Dominic: chrome/browser/net stuff
Ilya: Make sure you're happy with our use of profiles now.

Everything else should be the same.

[MAD, 2012-04-17 18:16:28]

LGTM with small nits...

Thanks!

BYE
MAD

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
chrome/browser/google/google_util.h:57: // explicit port. If |allow_subdomain|
is true, we check against host
Maybe fix this comment to the new param type not being true/false anymore.

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
chrome/browser/google/google_util.h:61: // |allow_subdomain| is true, we check
against host "*.google.<TLD>" instead.
Same here...

[SteveT, 2012-04-17 18:20:46]

Thanks, MAD! Fixed your nits.

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
chrome/browser/google/google_util.h:57: // explicit port. If |allow_subdomain|
is true, we check against host
On 2012/04/17 18:16:28, MAD wrote:
> Maybe fix this comment to the new param type not being true/false anymore.

Done.

https://chromiumcodereview.appspot.com/10108026/diff/2001/chrome/browser/goog...
chrome/browser/google/google_util.h:61: // |allow_subdomain| is true, we check
against host "*.google.<TLD>" instead.
On 2012/04/17 18:16:28, MAD wrote:
> Same here...

Done.

[battre, 2012-04-17 20:33:02]

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/goo...
File chrome/browser/google/google_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/goo...
chrome/browser/google/google_util_unittest.cc:277:
google_util::DISALLOW_SUBDOMAIN));
do you want to add tests for scheme and port filtering?

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:333:
MetricsServiceHelper::IsMetricsReportingEnabled()) {
Here we still have a threading problem. The MetricServiceHelper accesses the
PrefService which lives on the UI thread. You can use a PrefMember.

[Ilya Sherman, 2012-04-17 20:55:39]

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/ext...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/ext...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:129:
CookieSettings::Factory::GetForProfile(&profile_), false,
nit:  Please use an enumerated constant so that meaning of this argument is
clear at the call site.

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:327: // 3. We only transmit for
non Off The Record profiles.
nit: s/Off The Record/Incognito

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.h (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.h:96: // Whether or not the profile
is an Off The Record profile. Note that this
nit: s/Off The Record/Incognito

[SteveT, 2012-04-17 23:26:43]

Addressed one of Dominic's comments. A question for the other comment.

Ilya: I'll address yours next, but please take a look at my comment re:
PrefMembers in response to Dominic and let me know what you think...

Thanks so far!


Steve

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/goo...
File chrome/browser/google/google_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/goo...
chrome/browser/google/google_util_unittest.cc:277:
google_util::DISALLOW_SUBDOMAIN));
On 2012/04/17 20:33:02, battre wrote:
> do you want to add tests for scheme and port filtering?

Done.

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:333:
MetricsServiceHelper::IsMetricsReportingEnabled()) {
Ah, yes. I see that we do something similar with the enable_referrers PrefMember
here. You're suggesting the same approach, but for the Metrics-Enabled pref?

We're sort of circumventing MetricServiceHelper::IsMetricsReportingEnabled...
but it looks like we have no choice since we're on the IO thread here.  Ilya can
let us know if using the PrefMember is okay or not.

[Ilya Sherman, 2012-04-17 23:33:13]

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:333:
MetricsServiceHelper::IsMetricsReportingEnabled()) {
On 2012/04/17 23:26:44, SteveT wrote:
> Ah, yes. I see that we do something similar with the enable_referrers
PrefMember
> here. You're suggesting the same approach, but for the Metrics-Enabled pref?
> 
> We're sort of circumventing MetricServiceHelper::IsMetricsReportingEnabled...
> but it looks like we have no choice since we're on the IO thread here.  Ilya
can
> let us know if using the PrefMember is okay or not.

I think it's probably ok to use a PrefMember, and add a comment to the
MetricsService file documenting that this class implicitly relies on reading
that pref.  There's a slightly higher chance that the code will drift to be out
of sync, but I think it's still an ok compromise.

[battre, 2012-04-18 17:05:04]

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:333:
MetricsServiceHelper::IsMetricsReportingEnabled()) {
On 2012/04/17 23:33:13, Ilya Sherman wrote:
> On 2012/04/17 23:26:44, SteveT wrote:
> > Ah, yes. I see that we do something similar with the enable_referrers
> PrefMember
> > here. You're suggesting the same approach, but for the Metrics-Enabled pref?
> > 
> > We're sort of circumventing
MetricServiceHelper::IsMetricsReportingEnabled...
> > but it looks like we have no choice since we're on the IO thread here.  Ilya
> can
> > let us know if using the PrefMember is okay or not.
> 
> I think it's probably ok to use a PrefMember, and add a comment to the
> MetricsService file documenting that this class implicitly relies on reading
> that pref.  There's a slightly higher chance that the code will drift to be
out
> of sync, but I think it's still an ok compromise.

We could probably also put the PrefMember in MetricsServiceHelper. I am not sure
which approach wold be cleaner.

[SteveT, 2012-04-18 17:11:58]

On 2012/04/18 17:05:04, battre wrote:
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> chrome/browser/net/chrome_network_delegate.cc:333:
> MetricsServiceHelper::IsMetricsReportingEnabled()) {
> On 2012/04/17 23:33:13, Ilya Sherman wrote:
> > On 2012/04/17 23:26:44, SteveT wrote:
> > > Ah, yes. I see that we do something similar with the enable_referrers
> > PrefMember
> > > here. You're suggesting the same approach, but for the Metrics-Enabled
pref?
> > > 
> > > We're sort of circumventing
> MetricServiceHelper::IsMetricsReportingEnabled...
> > > but it looks like we have no choice since we're on the IO thread here. 
Ilya
> > can
> > > let us know if using the PrefMember is okay or not.
> > 
> > I think it's probably ok to use a PrefMember, and add a comment to the
> > MetricsService file documenting that this class implicitly relies on reading
> > that pref.  There's a slightly higher chance that the code will drift to be
> out
> > of sync, but I think it's still an ok compromise.
> 
> We could probably also put the PrefMember in MetricsServiceHelper. I am not
sure
> which approach wold be cleaner.

That's actually a good suggestion, given the value of the Metrics-Enabled pref
is global to an install rather than individual profiles.

In fact, this helps with the documentation restriction that MetricsServiceHelper
was trying to impose. We can keep this PrefMember in the MetricsServiceHelper
and force interested parties to friend MetricsServiceHelper if they want to use
this PrefMember. We can write a similar Initialize method to set it up and move
it to the IO thread:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/net/chrome_n...

The only concern here is whether or not this single PrefMember can be shared
between Profiles. We know it'll only be accessed on the MainBrowser's IO Thread,
so that avoids races. Is that sufficient? Is there a danger to calling
MoveToThread on the PrefMember more than once?

[battre, 2012-04-18 17:17:54]

On 2012/04/18 17:11:58, SteveT wrote:
> On 2012/04/18 17:05:04, battre wrote:
> >
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> > File chrome/browser/net/chrome_network_delegate.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> > chrome/browser/net/chrome_network_delegate.cc:333:
> > MetricsServiceHelper::IsMetricsReportingEnabled()) {
> > On 2012/04/17 23:33:13, Ilya Sherman wrote:
> > > On 2012/04/17 23:26:44, SteveT wrote:
> > > > Ah, yes. I see that we do something similar with the enable_referrers
> > > PrefMember
> > > > here. You're suggesting the same approach, but for the Metrics-Enabled
> pref?
> > > > 
> > > > We're sort of circumventing
> > MetricServiceHelper::IsMetricsReportingEnabled...
> > > > but it looks like we have no choice since we're on the IO thread here. 
> Ilya
> > > can
> > > > let us know if using the PrefMember is okay or not.
> > > 
> > > I think it's probably ok to use a PrefMember, and add a comment to the
> > > MetricsService file documenting that this class implicitly relies on
reading
> > > that pref.  There's a slightly higher chance that the code will drift to
be
> > out
> > > of sync, but I think it's still an ok compromise.
> > 
> > We could probably also put the PrefMember in MetricsServiceHelper. I am not
> sure
> > which approach wold be cleaner.
> 
> That's actually a good suggestion, given the value of the Metrics-Enabled pref
> is global to an install rather than individual profiles.
> 
> In fact, this helps with the documentation restriction that
MetricsServiceHelper
> was trying to impose. We can keep this PrefMember in the MetricsServiceHelper
> and force interested parties to friend MetricsServiceHelper if they want to
use
> this PrefMember. We can write a similar Initialize method to set it up and
move
> it to the IO thread:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/net/chrome_n...
> 
> The only concern here is whether or not this single PrefMember can be shared
> between Profiles. We know it'll only be accessed on the MainBrowser's IO
Thread,
> so that avoids races. Is that sufficient? Is there a danger to calling
> MoveToThread on the PrefMember more than once?

I haven't checked: Could you use MetricsService::RegisterPrefs to set up the
PrefMember of MetricsServiceHelper? I don't know whether the necessary threads
are started at the time ::RegisterPref is called.

[SteveT, 2012-04-18 19:15:08]

On 2012/04/18 17:17:54, battre wrote:
> On 2012/04/18 17:11:58, SteveT wrote:
> > On 2012/04/18 17:05:04, battre wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> > > File chrome/browser/net/chrome_network_delegate.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10108026/diff/11002/chrome/browser/net...
> > > chrome/browser/net/chrome_network_delegate.cc:333:
> > > MetricsServiceHelper::IsMetricsReportingEnabled()) {
> > > On 2012/04/17 23:33:13, Ilya Sherman wrote:
> > > > On 2012/04/17 23:26:44, SteveT wrote:
> > > > > Ah, yes. I see that we do something similar with the enable_referrers
> > > > PrefMember
> > > > > here. You're suggesting the same approach, but for the Metrics-Enabled
> > pref?
> > > > > 
> > > > > We're sort of circumventing
> > > MetricServiceHelper::IsMetricsReportingEnabled...
> > > > > but it looks like we have no choice since we're on the IO thread here.

> > Ilya
> > > > can
> > > > > let us know if using the PrefMember is okay or not.
> > > > 
> > > > I think it's probably ok to use a PrefMember, and add a comment to the
> > > > MetricsService file documenting that this class implicitly relies on
> reading
> > > > that pref.  There's a slightly higher chance that the code will drift to
> be
> > > out
> > > > of sync, but I think it's still an ok compromise.
> > > 
> > > We could probably also put the PrefMember in MetricsServiceHelper. I am
not
> > sure
> > > which approach wold be cleaner.
> > 
> > That's actually a good suggestion, given the value of the Metrics-Enabled
pref
> > is global to an install rather than individual profiles.
> > 
> > In fact, this helps with the documentation restriction that
> MetricsServiceHelper
> > was trying to impose. We can keep this PrefMember in the
MetricsServiceHelper
> > and force interested parties to friend MetricsServiceHelper if they want to
> use
> > this PrefMember. We can write a similar Initialize method to set it up and
> move
> > it to the IO thread:
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/net/chrome_n...
> > 
> > The only concern here is whether or not this single PrefMember can be shared
> > between Profiles. We know it'll only be accessed on the MainBrowser's IO
> Thread,
> > so that avoids races. Is that sufficient? Is there a danger to calling
> > MoveToThread on the PrefMember more than once?
> 
> I haven't checked: Could you use MetricsService::RegisterPrefs to set up the
> PrefMember of MetricsServiceHelper? I don't know whether the necessary threads
> are started at the time ::RegisterPref is called.

I don't think we have a guarantee on what threads are created at the time, since
the RegisterPrefs stuff happens lazily in the browser.

Turns out we want to avoid keeping a PrefMember on MetricsServiceHelper, since
it's a helper class with static members only. This PrefMember would have to be
static, which is a no-no since it introduces a static initializer.

How do you feel about going back to the proposal of adding the PrefMember to
ProfileIOData, just like the enabled_referrers PrefMember? It requires a bit of
extra memory since we keep one per Profile, but then we don't have to worry
about cleanup races, since each CND is owned by a parent ProfileIOData, which
will also own the PrefMember.

(in the system case, we just don't set the metrics_enabled PrefMember and check
to see if it's non-null before getting its value)

[SteveT, 2012-04-18 19:34:21]

> I don't think we have a guarantee on what threads are created at the time,
since
> the RegisterPrefs stuff happens lazily in the browser.
> 
> Turns out we want to avoid keeping a PrefMember on MetricsServiceHelper, since
> it's a helper class with static members only. This PrefMember would have to be
> static, which is a no-no since it introduces a static initializer.
> 
> How do you feel about going back to the proposal of adding the PrefMember to
> ProfileIOData, just like the enabled_referrers PrefMember? It requires a bit
of
> extra memory since we keep one per Profile, but then we don't have to worry
> about cleanup races, since each CND is owned by a parent ProfileIOData, which
> will also own the PrefMember.
> 
> (in the system case, we just don't set the metrics_enabled PrefMember and
check
> to see if it's non-null before getting its value)

Hm, this might be a no-go, since it forces us to access g_browser_process in
ProfileImplIOData, which has not been done before. According to erg@ this might
not be good if we're trying to get to non-lock-protected global state.

[SteveT, 2012-04-19 18:38:01]

Alright. I've made the move to a PrefMember, as suggested. ProfileImplIOData
owns the PrefMember, and we Init it in the init code that runs on the UI thread.


Dominic: General review of the net/ changes.

Will: OWNERS review for changes to Profile*IOData stuff. I feel like
ProfileImplIOData::Handle::Init is a good place to init the PrefMember, as
local_data is available there, but let me know if you think otherwise.

http://codereview.chromium.org/10108026/diff/11002/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

http://codereview.chromium.org/10108026/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:129:
CookieSettings::Factory::GetForProfile(&profile_), false,
On 2012/04/17 20:55:39, Ilya Sherman wrote:
> nit:  Please use an enumerated constant so that meaning of this argument is
> clear at the call site.

Done.

http://codereview.chromium.org/10108026/diff/11002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10108026/diff/11002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:327: // 3. We only transmit for
non Off The Record profiles.
On 2012/04/17 20:55:39, Ilya Sherman wrote:
> nit: s/Off The Record/Incognito

Done.

http://codereview.chromium.org/10108026/diff/11002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:333:
MetricsServiceHelper::IsMetricsReportingEnabled()) {
Turns out letting the PrefMember live in ProfileIOData makes the most sense. It
can be appropriately initialized on the UI thread like the enable_referrers
memeber.

willchan@ will be on the review to make sure our changes to Profile*IOData are
satisfactory.

[Ilya Sherman, 2012-04-19 18:52:58]

LGTM, thanks :)

http://codereview.chromium.org/10108026/diff/20001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10108026/diff/20001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:35: INCOGNITO,
nit: I know it makes for longer lines, but perhaps BROWSER_VISIBILITY_NORMAL and
BROWSER_VISIBILITY_INCOGNITO would be clearer than just NORMAL when seen out of
the context of this header?

[SteveT, 2012-04-20 16:46:14]

Pinging Dominic and Will :)

[SteveT, 2012-04-20 16:50:32]

Oh, and I've addressed Ilya's nit.

http://codereview.chromium.org/10108026/diff/20001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10108026/diff/20001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:35: INCOGNITO,
Okay, okay. Done :)

[willchan no longer on Chromium, 2012-04-20 17:40:46]

I chatted with stevet@. My primary concern is whether or not this should apply
to both web content (via RDH) *AND* _all_ URLFetchers. Steve said he would
check. If it shouldn't apply to all URLFetchers, then we will need to be more
granular and hook into Chrome's RDHDelegate to add the header for all web
content, and then selectively add the header to the URLFetchers that make sense.

Cheers.

[battre, 2012-04-23 10:49:55]

LGTM

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.h:35: class ChromeNetworkDelegate;
nit: not necessary

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:31: // ChromeNetworkDelegate. This
determines if we are permitted to transmit
This comment is not quite accurate: a) you cannot switch the incognito mode on
and off for a window, b) the system_request_context is not owned by a window.

My proposed formulation:
Whether or not the URLRequestContext associated with the ChromeNetworkDelegate
belongs to an incognito profile. This determines ...

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:114: // The Incognito state of the
browser window that uses this delegate. Note
see above regarding "the browser window"

[SteveT, 2012-04-27 00:05:06]

Okay. I've rewritten this code to use Will's suggested approach of RDH over the
ChromeNetworkDelegate. Please take a look.

battre@: Your previous comments don't make much sense after this rewrite. PTAL
at how we do things in the ChromeResourceDispatcherHostDelegate now, if you're
interested.
willchan@: PTAL at the profile and renderer_host changes.

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.h:35: class ChromeNetworkDelegate;
Removed.

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:31: // ChromeNetworkDelegate. This
determines if we are permitted to transmit
All of this has been removed.

http://codereview.chromium.org/10108026/diff/24003/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:114: // The Incognito state of the
browser window that uses this delegate. Note
All of this has been removed.

[battre, 2012-04-27 09:44:35]

Still LGTM - with one comment.

http://codereview.chromium.org/10108026/diff/46001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10108026/diff/46001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:345: mutable bool is_incognito_;
why mutable?

[SteveT, 2012-04-27 14:17:47]

Dominic's comment addressed - thanks!

http://codereview.chromium.org/10108026/diff/46001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10108026/diff/46001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:345: mutable bool is_incognito_;
Oops - that shouldn't be mutable anymore. Originally, this was being initialized
in a const method along-side other mutable fields, but we now initialize it in
the ctor. Fixed.

[SteveT, 2012-04-27 18:46:00]

Pinging willchan@ on this...

Will, do you have a chance to take a look today? This patch is blocking some
M20-critical work.


Steve

[willchan no longer on Chromium, 2012-04-27 21:46:47]

lgtm other than a cleanup request

http://codereview.chromium.org/10108026/diff/54001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10108026/diff/54001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:147: bool is_incognito;
Can you remove this if you're going to make is_incognito part of ProfileIOData?

[SteveT, 2012-04-28 17:44:53]

(Excuse the spam - reitveld's maintenance allowed emails through, but not my R
changes.)

Additional Rubber-Stamp approvals needed. I received these approvals earlier in
https://chromiumcodereview.appspot.com/10054029, which was reverted. The code
you'll be approving has not changed since 10054029.

Peter: OWNERS for chrome/browser/search_engines
Ivan: OWNERS for chrome/browser/protector

Thanks, all.

[SteveT, 2012-04-28 19:35:04]

R+pkasting for OWNERS of chrome/browser/search_engines re-rubber-stamping.

[SteveT, 2012-04-30 18:12:45]

Pinging, since this is blocking further work.

R+Anton, who is in a more convenient time zone than Ivan.

Anton, could you rubber stamp this? Ivan OK'd the changes in an earlier,
reverted CL. The protector stuff has not changed since then
(https://chromiumcodereview.appspot.com/10054029).

Peter - have a minute to rubber stamp this, too? Thanks!

[Peter Kasting, 2012-04-30 18:13:34]

LGTM

http://codereview.chromium.org/10108026/diff/57002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10108026/diff/57002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3288:
google_util::DISALLOW_SUBDOMAIN)) {
Nit: {} not necessary

[Ilya Sherman, 2012-04-30 21:11:52]

FWIW, I think you should just submit this CL with Ivan TBR'ed, since this is a
re-submission of a previously LGTM'ed CL, with the OWNed code unmodified from
the previously LGTM'ed version.

[SteveT, 2012-04-30 21:15:44]

Hm, you're probably right, since Ivan's approval is the last I'm waiting for.
I've added ivankr@chromium.org as TBR.

Committing presently. Thanks!


Steve

https://chromiumcodereview.appspot.com/10108026/diff/57002/chrome/browser/sea...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://chromiumcodereview.appspot.com/10108026/diff/57002/chrome/browser/sea...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3288:
google_util::DISALLOW_SUBDOMAIN)) {
On 2012/04/30 18:13:34, Peter Kasting wrote:
> Nit: {} not necessary

Done.

[commit-bot: I haz the power, 2012-04-30 21:16:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10108026/64002

[whywhat, 2012-04-30 22:29:00]

lgtm

Russia is on holidays until May 3.

[commit-bot: I haz the power, 2012-04-30 22:54:40]

Change committed as 134625