Sets correct ActivityLog enabled status to the first renderer process

Sets correct ActivityLog enabled status to the first renderer process

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

[felt, 2013-07-14 01:37:26]

I'm seeing some segfaults on the tryserver runs of our end to end test
(ActivityLogApiTest.TriggerEvent) for this patch. I don't *think* we're seeing
this problem in other runs, so I suspect it's from your patch. (But I might be
wrong.) Could you look at the logs to see if you think it's from your patch, vs.
another problem we aren't aware of yet?

[karenlees, 2013-07-14 03:20:12]

On 2013/07/14 01:37:26, felt wrote:
> I'm seeing some segfaults on the tryserver runs of our end to end test
> (ActivityLogApiTest.TriggerEvent) for this patch. I don't *think* we're seeing
> this problem in other runs, so I suspect it's from your patch. (But I might be
> wrong.) Could you look at the logs to see if you think it's from your patch,
vs.
> another problem we aren't aware of yet?

It looks like it's timing out waiting for "XMLHttpRequest.open"  to be logged as
a DOM activity but the logging never happens. This failure isn't happening in
the current test runs. Could anything on this change affect the XMLHttpRequest
logging?

This is the code that should trigger the activity logging:
   var request = new XMLHttpRequest();
   request.open('POST', defaultUrl, false);
   request.setRequestHeader('Content-type', 'text/plain;charset=UTF-8');

[pmarch, 2013-07-16 17:27:57]

Hi Adrienne and Karen

Could you please review this CL.

Thanks

[felt, 2013-07-18 16:49:43]

Just starting the review now, could you explain how the prefs work? How do
people set prefs?

[felt, 2013-07-18 16:54:32]

On 2013/07/18 16:49:43, felt wrote:
> Just starting the review now, could you explain how the prefs work? How do
> people set prefs?

Another question: can you set prefs from the command line? (We need to be able
to set the value from the command line.)

[pmarch, 2013-07-18 17:25:43]

On 2013/07/18 16:49:43, felt wrote:
> Just starting the review now, could you explain how the prefs work? How do
> people set prefs?

Syncable prefs are stored somewhere on a disk. I do not know exactly where. When
there is no entry for prefs::kActivityLogExtensionEnabled, pref manager calls
extensions::ActivityLog::RegisterProfilePrefs to set the default value. If there
is a value set for prefs::kActivityLogExtensionEnabled in the permanent storage,
perf manager does not reset it.

I spotted a bug, currently, prefs::kActivityLogExtensionEnabled is a single pref
for *all* profiles, where we need it per profile as ActivityLogExtension may be
installed per profile. I will look in this now,

[pmarch, 2013-07-18 17:29:10]

On 2013/07/18 16:54:32, felt wrote:
> On 2013/07/18 16:49:43, felt wrote:
> > Just starting the review now, could you explain how the prefs work? How do
> > people set prefs?
> 
> Another question: can you set prefs from the command line? (We need to be able
> to set the value from the command line.)

It is definitely possible, but there is a problem. If you set it it will be
still set in the next invocation of the browser despite the fact that in this
invocation there may be no flag supplied. Why do you want to set it via flag? we
have enable-extension-activity-logging flag? If this is just for tests, we can
do it as the test's profile (and prefs) is destroyed after each test.

[pmarch, 2013-07-18 18:38:13]

On 2013/07/18 17:29:10, pmarch wrote:
> On 2013/07/18 16:54:32, felt wrote:
> > On 2013/07/18 16:49:43, felt wrote:
> > > Just starting the review now, could you explain how the prefs work? How do
> > > people set prefs?
> > 
> > Another question: can you set prefs from the command line? (We need to be
able
> > to set the value from the command line.)
> 
> It is definitely possible, but there is a problem. If you set it it will be
> still set in the next invocation of the browser despite the fact that in this
> invocation there may be no flag supplied. Why do you want to set it via flag?
we
> have enable-extension-activity-logging flag? If this is just for tests, we can
> do it as the test's profile (and prefs) is destroyed after each test.

ActivityLogExtensionEnabled pref is actually per-profile, so we are cool here.

[felt, 2013-07-18 20:42:03]

Hi Petr, a few small comments based on our earlier discussion.

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:177:
activity_log_extension_enabled_ =
Can you also add a comment here documenting what this is checking (is the
extension installed & enabled)?

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:186: return;
What if the extension is installed later, causing enabled_ to be set to true? I
think we still need the has_threads_ check to make sure that we don't end up
with enabled_=true when has_threads_=false.

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:189: enabled_ =
IsLogEnabledOnAnyProfile();
And another comment here specifying that this is checking for the flag?

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.h (right):

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.h:220: bool
activity_log_extension_enabled_;
I'm finding this name confusingly similar to enabled_. Could you name it
something like: watchdog_extension_running_ or watchdog_extension_active_? And
mention here that this value is based on prefs, then updated?

[pmarch, 2013-07-18 21:37:14]

updated

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:177:
activity_log_extension_enabled_ =
On 2013/07/18 20:42:03, felt wrote:
> Can you also add a comment here documenting what this is checking (is the
> extension installed & enabled)?

Done.

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:186: return;
On 2013/07/18 20:42:03, felt wrote:
> What if the extension is installed later, causing enabled_ to be set to true?
I
> think we still need the has_threads_ check to make sure that we don't end up
> with enabled_=true when has_threads_=false.
nice catch, thanks

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:189: enabled_ =
IsLogEnabledOnAnyProfile();
On 2013/07/18 20:42:03, felt wrote:
> And another comment here specifying that this is checking for the flag?

Done.

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.h (right):

https://codereview.chromium.org/18430004/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.h:220: bool
activity_log_extension_enabled_;
On 2013/07/18 20:42:03, felt wrote:
> I'm finding this name confusingly similar to enabled_. Could you name it
> something like: watchdog_extension_running_ or watchdog_extension_active_? And
> mention here that this value is based on prefs, then updated?

Done.

[felt, 2013-07-18 22:11:37]

It now occurs to me that this CL obviates the need for a static method to check
the log status, so the LogIsEnabled class & static method are no longer needed
at all. You'd just have to move the flag check from
LogIsEnabled::ComputeIsFlagEnabled() into the activity log constructor.

https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:172:
activity_log_extension_active_(false) {
nit: can you also replace "activity_log" with "watchdog"? since that's what its
name will be

[pmarch, 2013-07-19 02:41:53]

Let's kill LogIsEnabled in a separate patch.

https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:172:
activity_log_extension_active_(false) {
On 2013/07/18 22:11:37, felt wrote:
> nit: can you also replace "activity_log" with "watchdog"? since that's what
its
> name will be

Done.

[felt, 2013-07-22 23:14:30]

On 2013/07/19 02:41:53, pmarch wrote:
> Let's kill LogIsEnabled in a separate patch.
> 
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> File chrome/browser/extensions/activity_log/activity_log.cc (right):
> 
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> chrome/browser/extensions/activity_log/activity_log.cc:172:
> activity_log_extension_active_(false) {
> On 2013/07/18 22:11:37, felt wrote:
> > nit: can you also replace "activity_log" with "watchdog"? since that's what
> its
> > name will be
> 
> Done.

Why do it separately?

[pmarch, 2013-07-22 23:21:59]

On 2013/07/22 23:14:30, felt wrote:
> On 2013/07/19 02:41:53, pmarch wrote:
> > Let's kill LogIsEnabled in a separate patch.
> > 
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > File chrome/browser/extensions/activity_log/activity_log.cc (right):
> > 
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > chrome/browser/extensions/activity_log/activity_log.cc:172:
> > activity_log_extension_active_(false) {
> > On 2013/07/18 22:11:37, felt wrote:
> > > nit: can you also replace "activity_log" with "watchdog"? since that's
what
> > its
> > > name will be
> > 
> > Done.
> 
> Why do it separately?

It may cause some non-trivial failures which is better to fix separately. I can
try to delete the class and see what is gonna happen.

[felt, 2013-07-22 23:32:48]

On 2013/07/22 23:21:59, pmarch wrote:
> On 2013/07/22 23:14:30, felt wrote:
> > On 2013/07/19 02:41:53, pmarch wrote:
> > > Let's kill LogIsEnabled in a separate patch.
> > > 
> > >
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > > File chrome/browser/extensions/activity_log/activity_log.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > > chrome/browser/extensions/activity_log/activity_log.cc:172:
> > > activity_log_extension_active_(false) {
> > > On 2013/07/18 22:11:37, felt wrote:
> > > > nit: can you also replace "activity_log" with "watchdog"? since that's
> what
> > > its
> > > > name will be
> > > 
> > > Done.
> > 
> > Why do it separately?
> 
> It may cause some non-trivial failures which is better to fix separately. I
can
> try to delete the class and see what is gonna happen.

I think it should be addressed here.
1) It's a bad idea to leave a big chunk of dead code around with a TODO to
delete it later.
2) It shouldn't be used anymore as of this patch, if it IS still being used that
is a bug. So if you delete it and find that something breaks, that should be
fixed.

[pmarch, 2013-07-23 00:56:16]

On 2013/07/22 23:32:48, felt wrote:
> On 2013/07/22 23:21:59, pmarch wrote:
> > On 2013/07/22 23:14:30, felt wrote:
> > > On 2013/07/19 02:41:53, pmarch wrote:
> > > > Let's kill LogIsEnabled in a separate patch.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > > > File chrome/browser/extensions/activity_log/activity_log.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18430004/diff/42001/chrome/browser/extensions...
> > > > chrome/browser/extensions/activity_log/activity_log.cc:172:
> > > > activity_log_extension_active_(false) {
> > > > On 2013/07/18 22:11:37, felt wrote:
> > > > > nit: can you also replace "activity_log" with "watchdog"? since that's
> > what
> > > > its
> > > > > name will be
> > > > 
> > > > Done.
> > > 
> > > Why do it separately?
> > 
> > It may cause some non-trivial failures which is better to fix separately. I
> can
> > try to delete the class and see what is gonna happen.
> 
> I think it should be addressed here.
> 1) It's a bad idea to leave a big chunk of dead code around with a TODO to
> delete it later.
> 2) It shouldn't be used anymore as of this patch, if it IS still being used
that
> is a bug. So if you delete it and find that something breaks, that should be
> fixed.

Done

[felt, 2013-07-23 06:37:33]

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:140: return;
If we return here, observers is never set to anything. In practice I think
that's fine, but can you at least set it to NULL in the initialization list?

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:185: enabled_ = true;
Can you switch this to:
if (!has_threads_) return;

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:485:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
Does this mean it will sync across browsers? Since we don't have support for
syncing yet, this should probably not be synced across browsers.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (left):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:51:
saved_cmdline_ = *CommandLine::ForCurrentProcess();
why did you remove this? we're still mucking around with the command line and
should be restoring it at the end of the test -- with the switch appended, the
activity log will remain on

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (left):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:125: if
(ActivityLog::IsLogEnabledOnAnyProfile()) {
Something I forgot about until now, and you should be aware of. This part is a
little tricky -- IsLogEnabledOnAnyProfile is, quite frankly, going to be wrong a
lot, so we 100% do want to get rid of it. However we can't switch to a
non-static method here because this method doesn't have a profile object. OTOH
it would be nice to not do this if the AL is disabled because dispatching the
IPC does have some cost.

The command line switch can be checked in a static method. Is there a way to
check prefs (perhaps all of the prefs) without using a profile object? If so we
could have a new, more-correct static method for here.

[felt, 2013-07-29 17:13:43]

Hi Petr, I'm back from vacation/conference now. What's the current status of
this CL?

[pmarch, 2013-07-29 17:36:39]

On 2013/07/29 17:13:43, felt wrote:
> Hi Petr, I'm back from vacation/conference now. What's the current status of
> this CL?

I switched to work on something else but I plan to get back to this CL in the
end of this week.

[pmarch, 2013-07-29 17:36:46]

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:140: return;
On 2013/07/23 06:37:34, felt wrote:
> If we return here, observers is never set to anything. In practice I think
> that's fine, but can you at least set it to NULL in the initialization list?

I think you are right, we should not return here. We need to set observers even
when there is no threads, so to monitor enabling/disabling watchdog extension.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:185: enabled_ = true;
On 2013/07/23 06:37:34, felt wrote:
> Can you switch this to:
> if (!has_threads_) return;
I do not think it is the right thing to do. If has_threads_ == false, and a user
installs the watchdog extensions, we still need to set the pref variable
correctly.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:485:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
On 2013/07/23 06:37:34, felt wrote:
> Does this mean it will sync across browsers? Since we don't have support for
> syncing yet, this should probably not be synced across browsers.

I will make it Non-syncable.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (left):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:51:
saved_cmdline_ = *CommandLine::ForCurrentProcess();
On 2013/07/23 06:37:34, felt wrote:
> why did you remove this? we're still mucking around with the command line and
> should be restoring it at the end of the test -- with the switch appended, the
> activity log will remain on

Comment below says saved_cmdline_ is used to preserve a copy of the original
command line. The test framework will do this itself as well. However, by
then,	it is too late to call ActivityLog::RecomputeLoggingIsEnabled() in
TearDown().

We do not have RecomputeLoggingIsEnabled() anymore. Moreover, other tests that
muck with the command line do not save it.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/event_router.cc (left):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/event_router.cc:125: if
(ActivityLog::IsLogEnabledOnAnyProfile()) {
On 2013/07/23 06:37:34, felt wrote:
> Something I forgot about until now, and you should be aware of. This part is a
> little tricky -- IsLogEnabledOnAnyProfile is, quite frankly, going to be wrong
a
> lot, so we 100% do want to get rid of it. However we can't switch to a
> non-static method here because this method doesn't have a profile object. OTOH
> it would be nice to not do this if the AL is disabled because dispatching the
> IPC does have some cost.
> 
> The command line switch can be checked in a static method. Is there a way to
> check prefs (perhaps all of the prefs) without using a profile object? If so
we
> could have a new, more-correct static method for here.

Here, we need to check for command line switch and if the watchdog extension is
enabled. The command line switch can be checked statically but the watchdog
extension enable/disable status is per-profile. Thus we need to have a profile
object here.

I see two solutions:
1) implementing something similar to IsLogEnabledOnAnyProfile() 
2) moving message logging to the upper layer of IPC message handling where there
is a profile object available.

[felt, 2013-07-29 19:29:42]

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:185: enabled_ = true;
On 2013/07/29 17:36:47, pmarch wrote:
> On 2013/07/23 06:37:34, felt wrote:
> > Can you switch this to:
> > if (!has_threads_) return;
> I do not think it is the right thing to do. If has_threads_ == false, and a
user
> installs the watchdog extensions, we still need to set the pref variable
> correctly. 

Ah, OK, you're right -- the semantics have changed a bit here now that we are
permanently storing the state.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (left):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:51:
saved_cmdline_ = *CommandLine::ForCurrentProcess();
On 2013/07/29 17:36:47, pmarch wrote:
> On 2013/07/23 06:37:34, felt wrote:
> > why did you remove this? we're still mucking around with the command line
and
> > should be restoring it at the end of the test -- with the switch appended,
the
> > activity log will remain on
> 
> Comment below says saved_cmdline_ is used to preserve a copy of the original
> command line. The test framework will do this itself as well. However, by
> then,	it is too late to call ActivityLog::RecomputeLoggingIsEnabled() in
> TearDown().
> 
> We do not have RecomputeLoggingIsEnabled() anymore. Moreover, other tests that
> muck with the command line do not save it.

Ahh, I see, the problem was being caused by RecomputeLoggingIsEnabled, not that
the testing framework itself didn't reset the command line? OK, sounds fine
then. (But please double check that you aren't seeing the AL running on the
trybots for other tests.)

[felt, 2013-07-29 21:05:22]

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:64: // Restore
the original command line and undo the affects of SetUp().
PS please kill the comment too if removing the code

[pmarch, 2013-08-08 14:42:41]

Adrienne, could you please have another look at this.

Thanks.

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/18430004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:64: // Restore
the original command line and undo the affects of SetUp().
On 2013/07/29 21:05:23, felt wrote:
> PS please kill the comment too if removing the code

Done.

[felt, 2013-08-08 17:49:24]

lgtm except for these comments

Note that if you keep the old name IsLogEnabledOnAnyProfile you'll need fewer
extra reviewers ;)

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.h (right):

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.h:174: static bool
maybe_enabled_;
This is a nice solution. Have you tested this across profiles to make sure it
works?

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1675: if
(extensions::ActivityLog::MaybeLogEnabled()) {
You might as well keep the old name (IsLogEnabledOnAnyProfile) since it does
describe it still. Then you wouldn't have to change this.

[felt, 2013-08-08 18:28:59]

Hi Petr, you're going to need a few more reviewers.

For chrome/browser/prefs, you need *one* of:
battre@chromium.org
bauerb@chromium.org
mnissler@chromium.org
pam@chromium.org

For chrome/browser/extensions/api/web_request/web_request_api.cc and
chrome/browser/extensions/event_router.cc:
you should ask mpcomplete@, although I don't think those files actually need to
be changed if you want to use the old name IsLogEnabledOnAnyProfile for your new
method.

For chrome/browser/chrome_content_browser_client.cc and
chrome/common/pref_names.[cc|h]:
You don't need an additional reviewer for these, my lgtm is enough.

Adrienne

[pmarch, 2013-08-12 20:43:53]

Adrienne, I've added a unit test.

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/18430004/diff/89001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api.cc:1675: if
(extensions::ActivityLog::MaybeLogEnabled()) {
On 2013/08/08 17:49:24, felt wrote:
> You might as well keep the old name (IsLogEnabledOnAnyProfile) since it does
> describe it still. Then you wouldn't have to change this.

Done.

[pmarch, 2013-08-12 20:47:39]

Hi Dominic, could you please approve change to 
chrome/browser/prefs/browser_prefs.cc

Thanks

[battre, 2013-08-13 13:38:01]

chrome/browser/prefs/browser_prefs.cc LGTM

[commit-bot: I haz the power, 2013-08-13 13:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/18430004/120001

[pmarch, 2013-08-13 14:34:57]

Adrienne,

Can you go through a new unit test before I submit the CL.

Thanks

[felt, 2013-08-13 15:05:39]

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc (right):

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:19: class
IsLogEnabledTest : public ChromeRenderViewHostTestHarness {
i think you can replace this with a typedef

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:22:
TEST_F(IsLogEnabledTest, NoSwitch) {
can you put ActivityLog in the name of the test class?

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:56:
EXPECT_TRUE(activity_log2->IsLogEnabledOnAnyProfile());
why not also check activity_log1->IsLogEnabledOnAnyProfile()? same question
elsewhere

[pmarch, 2013-08-13 15:52:53]

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc (right):

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:19: class
IsLogEnabledTest : public ChromeRenderViewHostTestHarness {
On 2013/08/13 15:05:40, felt wrote:
> i think you can replace this with a typedef

Done.

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:22:
TEST_F(IsLogEnabledTest, NoSwitch) {
On 2013/08/13 15:05:40, felt wrote:
> can you put ActivityLog in the name of the test class?

Done.

https://codereview.chromium.org/18430004/diff/115001/chrome/browser/extension...
chrome/browser/extensions/activity_log/is_log_enabled_unittest.cc:56:
EXPECT_TRUE(activity_log2->IsLogEnabledOnAnyProfile());
On 2013/08/13 15:05:40, felt wrote:
> why not also check activity_log1->IsLogEnabledOnAnyProfile()? same question
> elsewhere

IsLogEnabledOnAnyProfile is a static method, but yes why not.

Done.

[commit-bot: I haz the power, 2013-08-13 20:38:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/18430004/156001

[commit-bot: I haz the power, 2013-08-14 01:18:49]

Change committed as 217425