Move 'theme' parsing out of Extension class

Move 'theme' parsing out of Extension class

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

[Yoyo Zhou, 2013-01-22 21:48:08]

Just a general comment: do you see a place this could go in
chrome/browser/themes rather than chrome/browser/extensions? Can it be rolled
into ThemeServiceFactory? If not, chrome/browser/extensions/api/themes would
make more sense.

[Joe Thomas, 2013-01-23 22:55:34]

On 2013/01/22 21:48:08, Yoyo Zhou wrote:
> Just a general comment: do you see a place this could go in
> chrome/browser/themes rather than chrome/browser/extensions? Can it be rolled
> into ThemeServiceFactory? If not, chrome/browser/extensions/api/themes would
> make more sense.

Thanks for the review!

I moved the ManifestHandler registration to theme_service. Please review the
updated patch.

[Yoyo Zhou, 2013-01-26 00:54:02]

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/browser/them...
File chrome/browser/themes/theme_service.cc (right):

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/browser/them...
chrome/browser/themes/theme_service.cc:214: ThemeService::ThemeService()
It seems like this would work as is (because of other factory dependencies), but
it's better to make ThemeServiceFactory::ServiceIsCreatedWithProfile true.

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/common/exten...
File chrome/common/extensions/theme_handler.h (right):

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/common/exten...
chrome/common/extensions/theme_handler.h:5: #ifndef
CHROME_COMMON_EXTENSIONS_THEME_HANDLER_H_
I've been thinking about where to put this, since themes are not extensions.
Ideally, there would be a type that encompasses extensions and themes, i.e.
things with manifests, and manifest handling would be associated with that type
rather than Extension. But for now, that type can't be distinguished from
Extension. So can you put this in chrome/common/extensions/api/themes?

[Joe Thomas, 2013-01-26 02:05:55]

Thanks for the review. Patch updated.

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/browser/them...
File chrome/browser/themes/theme_service.cc (right):

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/browser/them...
chrome/browser/themes/theme_service.cc:214: ThemeService::ThemeService()
On 2013/01/26 00:54:02, Yoyo Zhou wrote:
> It seems like this would work as is (because of other factory dependencies),
but
> it's better to make ThemeServiceFactory::ServiceIsCreatedWithProfile true.

Done.

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/common/exten...
File chrome/common/extensions/theme_handler.h (right):

https://chromiumcodereview.appspot.com/11968032/diff/5001/chrome/common/exten...
chrome/common/extensions/theme_handler.h:5: #ifndef
CHROME_COMMON_EXTENSIONS_THEME_HANDLER_H_
On 2013/01/26 00:54:02, Yoyo Zhou wrote:
> I've been thinking about where to put this, since themes are not extensions.
> Ideally, there would be a type that encompasses extensions and themes, i.e.
> things with manifests, and manifest handling would be associated with that
type
> rather than Extension. But for now, that type can't be distinguished from
> Extension. So can you put this in chrome/common/extensions/api/themes?

Done.

[Yoyo Zhou, 2013-01-30 00:24:41]

LGTM

[Joe Thomas, 2013-01-30 02:10:42]

Yoyo@
With new ManifestHandler interface.

+ pkotwicz@ for c/b/themes
+ ben@  for gyp changes & chrome/utility/chrome_content_utility_client.cc
+ nileshagrawal@ for android gypi changes.

[Devlin, 2013-01-30 02:56:40]

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
File chrome/common/extensions/api/themes/theme_handler.cc (right):

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
chrome/common/extensions/api/themes/theme_handler.cc:50: // The color must be a
list
nit: comments should have punctuation. In this case, the comment on 50 can be
continued on to 52, so that it reads

// The color must be a list...
// ... and either 3 items (RGB) or 4 (RGBA).

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
chrome/common/extensions/api/themes/theme_handler.cc:58: // For both RGB and
RGBA, the first three items must be ints (R,G,B)
nit: punctuation (period at end).

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
File chrome/common/extensions/api/themes/theme_handler.h (right):

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
chrome/common/extensions/api/themes/theme_handler.h:50:
DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private section of
a class (it's just a macro that the copy constructor and assignment operator for
a class;
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
File chrome/common/extensions/extension.h (left):

https://chromiumcodereview.appspot.com/11968032/diff/11002/chrome/common/exte...
chrome/common/extensions/extension.h:679: bool is_theme() const;
Do we want to keep this in the Extension class? We could easily store it as a
bool in ThemeInfo, thereby removing all theme references from Extension main; is
there a reason to keep it here?

[Joe Thomas, 2013-01-30 18:15:39]

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
File chrome/common/extensions/extension.h (left):

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/extension.h:679: bool is_theme() const;
On 2013/01/30 02:56:40, D Cronin wrote:
> Do we want to keep this in the Extension class? We could easily store it as a
> bool in ThemeInfo, thereby removing all theme references from Extension main;
is
> there a reason to keep it here?

Since this is a function which returns the basic type of Extension object, I
thought it's better to be part of Extension class.

Yoyo@, what do you think?

[Ben Goodger (Google), 2013-01-30 18:44:16]

lgtm

[pkotwicz, 2013-01-30 20:45:28]

LGTM, with a question

https://codereview.chromium.org/11968032/diff/11002/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/11968032/diff/11002/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:222: new extensions::ThemeHandler);
This is probably a stupid question. Is there any guarantee that the ThemeService
has been created at the time that we are parsing a theme?

[Joe Thomas, 2013-01-30 21:12:45]

https://codereview.chromium.org/11968032/diff/11002/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/11968032/diff/11002/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:222: new extensions::ThemeHandler);
Thanks for the review!

On 2013/01/30 20:45:28, pkotwicz wrote:
> This is probably a stupid question. Is there any guarantee that the
ThemeService
> has been created at the time that we are parsing a theme?

Yes, we are making sure that ThemeService gets started at Profile Start-up
(please see the change in theme_service_factory.cc/h). We try to parse the
Manifest file of extension/theme only after profile start-up.

[pkotwicz, 2013-01-30 21:20:46]

LGTM

[nilesh, 2013-01-30 21:24:47]

On 2013/01/30 02:10:42, Joe Thomas wrote:
> Yoyo@
> With new ManifestHandler interface.
> 
> + pkotwicz@ for c/b/themes
> + ben@  for gyp changes & chrome/utility/chrome_content_utility_client.cc
> + nileshagrawal@ for android gypi changes.

LGTM

[Yoyo Zhou, 2013-01-30 22:11:06]

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
File chrome/common/extensions/extension.h (left):

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/extension.h:679: bool is_theme() const;
On 2013/01/30 18:15:39, Joe Thomas wrote:
> On 2013/01/30 02:56:40, D Cronin wrote:
> > Do we want to keep this in the Extension class? We could easily store it as
a
> > bool in ThemeInfo, thereby removing all theme references from Extension
main;
> is
> > there a reason to keep it here?
> 
> Since this is a function which returns the basic type of Extension object, I
> thought it's better to be part of Extension class.
> 
> Yoyo@, what do you think?

This is a property of the manifest rather than the extension, so I'd recommend
leaving it here.

[Joe Thomas, 2013-01-30 23:15:18]

D Cronin@

Thanks for the review. Comments addressed.

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
File chrome/common/extensions/api/themes/theme_handler.cc (right):

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/api/themes/theme_handler.cc:50: // The color must be a
list
On 2013/01/30 02:56:40, D Cronin wrote:
> nit: comments should have punctuation. In this case, the comment on 50 can be
> continued on to 52, so that it reads
> 
> // The color must be a list...
> // ... and either 3 items (RGB) or 4 (RGBA).

Done.

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/api/themes/theme_handler.cc:58: // For both RGB and
RGBA, the first three items must be ints (R,G,B)
On 2013/01/30 02:56:40, D Cronin wrote:
> nit: punctuation (period at end).

Done. Fixed couple of other places too.

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
File chrome/common/extensions/api/themes/theme_handler.h (right):

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/api/themes/theme_handler.h:50:
DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
On 2013/01/30 02:56:40, D Cronin wrote:
> DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private section
of
> a class (it's just a macro that the copy constructor and assignment operator
for
> a class;
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).

Ok. Removed.

[Devlin, 2013-01-30 23:36:33]

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
File chrome/common/extensions/api/themes/theme_handler.h (right):

https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
chrome/common/extensions/api/themes/theme_handler.h:50:
DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
On 2013/01/30 23:15:19, Joe Thomas wrote:
> On 2013/01/30 02:56:40, D Cronin wrote:
> > DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private
section
> of
> > a class (it's just a macro that the copy constructor and assignment operator
> for
> > a class;
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).
> 
> Ok. Removed.

This shouldn't be removed; it should be declared in the private: section of a
class, i.e.
class ThemeHandler : public Manifest Handler {
 public:
  ThemeHandler();
  virtual ~ThemeHandler();

  virtual bool Parse(Extension* extension, string16* error) OVERRIDE;

 private:
  DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
};

[Joe Thomas, 2013-01-31 18:08:30]

On 2013/01/30 23:36:33, D Cronin wrote:
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> File chrome/common/extensions/api/themes/theme_handler.h (right):
> 
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> chrome/common/extensions/api/themes/theme_handler.h:50:
> DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
> On 2013/01/30 23:15:19, Joe Thomas wrote:
> > On 2013/01/30 02:56:40, D Cronin wrote:
> > > DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private
> section
> > of
> > > a class (it's just a macro that the copy constructor and assignment
operator
> > for
> > > a class;
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).
> > 
> > Ok. Removed.
> 
> This shouldn't be removed; it should be declared in the private: section of a
> class,

Oops. Did that in a hurry. Added in private:. Also corrected
default_locale_handler.h

[Devlin, 2013-02-01 00:03:45]

On 2013/01/31 18:08:30, Joe Thomas wrote:
> On 2013/01/30 23:36:33, D Cronin wrote:
> >
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> > File chrome/common/extensions/api/themes/theme_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> > chrome/common/extensions/api/themes/theme_handler.h:50:
> > DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
> > On 2013/01/30 23:15:19, Joe Thomas wrote:
> > > On 2013/01/30 02:56:40, D Cronin wrote:
> > > > DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private
> > section
> > > of
> > > > a class (it's just a macro that the copy constructor and assignment
> operator
> > > for
> > > > a class;
> > > >
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).
> > > 
> > > Ok. Removed.
> > 
> > This shouldn't be removed; it should be declared in the private: section of
a
> > class,
> 
> Oops. Did that in a hurry. Added in private:. Also corrected
> default_locale_handler.h

LGTM

[Devlin, 2013-02-01 00:03:45]

On 2013/01/31 18:08:30, Joe Thomas wrote:
> On 2013/01/30 23:36:33, D Cronin wrote:
> >
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> > File chrome/common/extensions/api/themes/theme_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/11968032/diff/11002/chrome/common/extensions/...
> > chrome/common/extensions/api/themes/theme_handler.h:50:
> > DISALLOW_COPY_AND_ASSIGN(ThemeHandler);
> > On 2013/01/30 23:15:19, Joe Thomas wrote:
> > > On 2013/01/30 02:56:40, D Cronin wrote:
> > > > DISALLOW_COPY_AND_ASSIGN() only works if it's declared in the private
> > section
> > > of
> > > > a class (it's just a macro that the copy constructor and assignment
> operator
> > > for
> > > > a class;
> > > >
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_p...).
> > > 
> > > Ok. Removed.
> > 
> > This shouldn't be removed; it should be declared in the private: section of
a
> > class,
> 
> Oops. Did that in a hurry. Added in private:. Also corrected
> default_locale_handler.h

LGTM

[commit-bot: I haz the power, 2013-02-01 17:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11968032/31011

[commit-bot: I haz the power, 2013-02-01 17:46:35]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-02-01 18:55:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11968032/31011

[Joe Thomas, 2013-02-01 21:28:32]

yoz@, pkotwicz@

The unit test ProtocolHandlerRegistryTest.ClearDefaultMakesProtocolNotHandled
was crashing during TestingProfile destruction. I fixed it by setting
ServiceIsNULLWhileTesting to true.(changes in theme_service_factory.h/cc). 
The change in this CL to start the theme_service at profile Startup triggered
this failure. So I think it's safe to set ServiceIsNULLWhileTesting boolean.

[pkotwicz, 2013-02-04 18:23:14]

Still LGTM

[commit-bot: I haz the power, 2013-02-04 18:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11968032/14033

[commit-bot: I haz the power, 2013-02-04 19:40:27]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Joe Thomas, 2013-02-05 23:58:00]

yoz@

Setting the ServiceIsNULLWhileTesting boolean in theme_service_factory in
patch-set 8 led to more unittest crashes as there are many tests which tries to
access theme_service while executing (using GetForProfile).

So I went back to patch-set 7 and tried to fix the unittest crashes which is
happening during Profile destruction. There are TWO unittests which are crashing
on linux platform (crash is in GTKThemeService, so other platform are safe) due
to the same root cause.

Root Cause: GTKThemeService creates Pref for the profile in
GtkThemeService::Init. The failing tests creates a TestingProfile object and
overwrites the Profile's pref with a new Pref. GTKThemeService then crashes in
the destruction process while using the old Pref pointer which is not valid
anymore. Below are the tests which are crashing:

ProtocolHandlerRegistryTest.* - Fix is uploaded to
https://codereview.chromium.org/12211026/

PrefProviderTest.Incognito - It looks to me like this test cannot be fixed
without having some setter functions in PrefServiceSyncable class to set the
"user_prefs", which I am not sure is correct.

Do you have some idea to fix this failing test?

[pkotwicz, 2013-02-06 01:57:52]

Adding battre@ as a reviewer for his knowledge about the pref service.

[battre, 2013-02-06 12:56:24]

On 2013/02/05 23:58:00, Joe Thomas wrote:
> yoz@
> 
> Setting the ServiceIsNULLWhileTesting boolean in theme_service_factory in
> patch-set 8 led to more unittest crashes as there are many tests which tries
to
> access theme_service while executing (using GetForProfile).
> 
> So I went back to patch-set 7 and tried to fix the unittest crashes which is
> happening during Profile destruction. There are TWO unittests which are
crashing
> on linux platform (crash is in GTKThemeService, so other platform are safe)
due
> to the same root cause.
> 
> Root Cause: GTKThemeService creates Pref for the profile in
> GtkThemeService::Init. The failing tests creates a TestingProfile object and
> overwrites the Profile's pref with a new Pref. GTKThemeService then crashes in
> the destruction process while using the old Pref pointer which is not valid
> anymore. Below are the tests which are crashing:
> 
> ProtocolHandlerRegistryTest.* - Fix is uploaded to
> https://codereview.chromium.org/12211026/
> 
> PrefProviderTest.Incognito - It looks to me like this test cannot be fixed
> without having some setter functions in PrefServiceSyncable class to set the
> "user_prefs", which I am not sure is correct.

Does it help using a TestingProfile::Builder with
TestingProfile::Builder::SetPrefService() instead of first creating the the
profile and then calling SetPrefService?

[Joe Thomas, 2013-02-06 23:41:58]

On 2013/02/06 12:56:24, battre wrote:
> On 2013/02/05 23:58:00, Joe Thomas wrote:
> > yoz@
> > 
> > Setting the ServiceIsNULLWhileTesting boolean in theme_service_factory in
> > patch-set 8 led to more unittest crashes as there are many tests which tries
> to
> > access theme_service while executing (using GetForProfile).
> > 
> > So I went back to patch-set 7 and tried to fix the unittest crashes which is
> > happening during Profile destruction. There are TWO unittests which are
> crashing
> > on linux platform (crash is in GTKThemeService, so other platform are safe)
> due
> > to the same root cause.
> > 
> > Root Cause: GTKThemeService creates Pref for the profile in
> > GtkThemeService::Init. The failing tests creates a TestingProfile object and
> > overwrites the Profile's pref with a new Pref. GTKThemeService then crashes
in
> > the destruction process while using the old Pref pointer which is not valid
> > anymore. Below are the tests which are crashing:
> > 
> > ProtocolHandlerRegistryTest.* - Fix is uploaded to
> > https://codereview.chromium.org/12211026/
> > 
> > PrefProviderTest.Incognito - It looks to me like this test cannot be fixed
> > without having some setter functions in PrefServiceSyncable class to set the
> > "user_prefs", which I am not sure is correct.
> 
> Does it help using a TestingProfile::Builder with
> TestingProfile::Builder::SetPrefService() instead of first creating the the
> profile and then calling SetPrefService?

Yes. That worked. Thanks for the pointer.

Could you please review
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc?

Should I remove TestingProfile::SetPrefService as it's not used anywhere else?

[battre, 2013-02-07 09:45:08]

LGTM @ content_settings_pref_provider_unittest.cc

Please remove TestingProfile::SetPrefService as using it is likely to cause
trouble. Thanks.

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:149:
profile_builder.SetPrefService(make_scoped_ptr(regular_prefs).Pass());
No need for calling ".Pass()"

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:153:
otr_profile_builder.SetPrefService(make_scoped_ptr(otr_prefs).Pass());
No need for calling ".Pass()"

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:157:
profile->SetOffTheRecordProfile(otr_profile);
I think that this would be slightly cleaner:

TestingProfile::Builder otr_profile_builder;
otr_profile_builder.SetPrefService(make_scoped_ptr(otr_prefs))
otr_profile_builder.SetOffTheRecord();
profile->SetOffTheRecordProfile(otr_profile_builder.Build().release());

WDYT?

[Joe Thomas, 2013-02-07 14:22:54]

battre@

Thanks for the review.

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:157:
profile->SetOffTheRecordProfile(otr_profile);
On 2013/02/07 09:45:08, battre wrote:
> I think that this would be slightly cleaner:
> 
> TestingProfile::Builder otr_profile_builder;
> otr_profile_builder.SetPrefService(make_scoped_ptr(otr_prefs))
> otr_profile_builder.SetOffTheRecord();
> profile->SetOffTheRecordProfile(otr_profile_builder.Build().release());
> 
> WDYT?

I had tried it already and it was causing a DCHECK failure. So I left it like
this.
Reason: While creating incognito profile, we dont call ProfileKeyedBaseFactory::
RegisterUserPrefs 
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...).
So we dont register kUsesSystemTheme preference in 
ThemeServiceFactory::RegisterUserPrefs for incognito profile. This causes a
DCHECK failure in pref_service.cc while trying to read this preference from
GTKThemeService::Init.

[Joe Thomas, 2013-02-07 18:30:30]

Removed TestingProfile::SetPrefService. Addressed other review comments.

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:149:
profile_builder.SetPrefService(make_scoped_ptr(regular_prefs).Pass());
On 2013/02/07 09:45:08, battre wrote:
> No need for calling ".Pass()"

Done.

https://codereview.chromium.org/11968032/diff/55001/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:153:
otr_profile_builder.SetPrefService(make_scoped_ptr(otr_prefs).Pass());
On 2013/02/07 09:45:08, battre wrote:
> No need for calling ".Pass()"

Done.

[battre, 2013-02-07 18:51:23]

Thanks. Still LGTM

[commit-bot: I haz the power, 2013-02-11 18:56:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11968032/61007

[commit-bot: I haz the power, 2013-02-11 19:49:52]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Joe Thomas, 2013-02-11 21:33:53]

isherman@

Could you please review chrome/browser/autofill/autofill_manager_unittest.cc and
testing_profile.cc/h changes to remove
TestingProfile::Builder::SetOffTheRecord() as it's not used anywhere else.

Context(same as comment #34): With this CL, GTKThemeService will be started at
Profile creation. While creating incognito profile, we don't call
ProfileKeyedBaseFactory::RegisterUserPrefs 
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...).
And so we don't register kUsesSystemTheme preference in
ThemeServiceFactory::RegisterUserPrefs for incognito profile. This causes a
DCHECK failure in pref_service.cc while trying to read this preference from
GTKThemeService::Init.

Fix: Create the profile and then set the incognito variable.

[Ilya Sherman, 2013-02-11 22:15:00]

LGTM, thanks.

[commit-bot: I haz the power, 2013-02-11 22:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11968032/56013

[commit-bot: I haz the power, 2013-02-12 02:23:14]

Change committed as 181826