Removes fallback from Prefs to LoadExtension on UI thread

- Removes fallback from Prefs to LoadExtension on UI thread.
- Fix extension_service_unittest to actually load extensions from prefs.

BUG=19733
TEST=Remove extension manifest from Preferences file. Extension shouldn't load even if the necessary files are present.

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

[Aaron Boodman, 2009-10-29 19:35:59]

Review of InstalledExtensionsLoader by request...

It looks pretty clean, good job. Just a few FYIs.

http://codereview.chromium.org/339058/diff/2004/3007
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/339058/diff/2004/3007#newcode1
Line 1: // Copyright (c) 2009 The Chromium   Authors. All rights reserved.
Nit: remove this change.

http://codereview.chromium.org/339058/diff/2004/3007#newcode284
Line 284: scoped_refptr<InstalledExtensionsLoader> installed_extensions_loader_;
FYI: In many cases when dealing with these "tasky" objects, you need to handle
the case where the client goes away before the task is complete.

What we have done in those cases is add a ClearClient() method to the task that
the client can call to tell the task to not try and report back.

In this case it doesn't matter as the ExtensionsService is owned by the profile
which doesn't go away until the UI thread shuts down.

http://codereview.chromium.org/339058/diff/2004/3010
File chrome/browser/extensions/installed_extensions_loader.cc (right):

http://codereview.chromium.org/339058/diff/2004/3010#newcode23
Line 23: STLDeleteElements(&extensions_to_load_);
You can use linked_ptr for the values of the vector to avoid having to do the
manual delete.

http://codereview.chromium.org/339058/diff/2004/3009
File chrome/browser/extensions/installed_extensions_loader.h (right):

http://codereview.chromium.org/339058/diff/2004/3009#newcode49
Line 49: DictionaryValue* manifest;
This could be a scoped_ptr to avoid the manual delete.

[Nebojša Ćirić, 2009-10-29 20:40:51]

Thanks for the review.

After the offline discussion we decided to avoid loading files on startup. I
will delete parts of CL that do that, and change CL description to reflect
actual state.

On 2009/10/29 19:35:59, Aaron Boodman wrote:
> Review of InstalledExtensionsLoader by request...
> 
> It looks pretty clean, good job. Just a few FYIs.
> 
> http://codereview.chromium.org/339058/diff/2004/3007
> File chrome/browser/extensions/extensions_service.h (right):
> 
> http://codereview.chromium.org/339058/diff/2004/3007#newcode1
> Line 1: // Copyright (c) 2009 The Chromium   Authors. All rights reserved.
> Nit: remove this change.
> 
> http://codereview.chromium.org/339058/diff/2004/3007#newcode284
> Line 284: scoped_refptr<InstalledExtensionsLoader>
installed_extensions_loader_;
> FYI: In many cases when dealing with these "tasky" objects, you need to handle
> the case where the client goes away before the task is complete.
> 
> What we have done in those cases is add a ClearClient() method to the task
that
> the client can call to tell the task to not try and report back.
> 
> In this case it doesn't matter as the ExtensionsService is owned by the
profile
> which doesn't go away until the UI thread shuts down.
> 
> http://codereview.chromium.org/339058/diff/2004/3010
> File chrome/browser/extensions/installed_extensions_loader.cc (right):
> 
> http://codereview.chromium.org/339058/diff/2004/3010#newcode23
> Line 23: STLDeleteElements(&extensions_to_load_);
> You can use linked_ptr for the values of the vector to avoid having to do the
> manual delete.
> 
> http://codereview.chromium.org/339058/diff/2004/3009
> File chrome/browser/extensions/installed_extensions_loader.h (right):
> 
> http://codereview.chromium.org/339058/diff/2004/3009#newcode49
> Line 49: DictionaryValue* manifest;
> This could be a scoped_ptr to avoid the manual delete.

[Matt Perry, 2009-10-29 21:46:35]

LGTM. Sorry for the slow review. Thanks for doing this cleanup.

[Aaron Boodman, 2009-10-29 22:16:32]

LGTM2

On Thu, Oct 29, 2009 at 2:46 PM,  <mpcomplete@chromium.org> wrote:
> LGTM. Sorry for the slow review. Thanks for doing this cleanup.
>
> http://codereview.chromium.org/339058
>