Remove //chrome dependency from ExtensionPrefs

Remove //chrome dependency from ExtensionPrefs

ExtensionPrefs::Observer has been extended with additional
methods to accomodate the needs of Chrome's contentSettings API
implementation.

The ContentSettingsStore instance which was previously hosted
by ExtensionPrefs is now hosted by an independent (BCKS)
ContentSettingsService.

ExtensionsBrowserClient gets a new method to allow a client to
inject ExtensionPrefs observers between construction and
initialization. Chrome's implementation ensures that
a ContentSettingsStore exists and adds it as an ExtensionPrefs
observer.

Somewhat unfortunately this means that ExtensionPrefs construction
and initialization has been partially split to allow observers to
be added prior to initialization.

BUG=357749, 162530
R=jamescook@chromium.org, yoz@chromium.org
TBR=bauerb@chromium.org for c/b/content_settings, finnur@chromium.org for c/b/ui/webui/extensions

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

[James Cook, 2014-04-01 18:45:02]

Some initial thoughts.

https://codereview.chromium.org/220353002/diff/1/apps/shell/browser/shell_ext...
File apps/shell/browser/shell_extensions_browser_client.h (right):

https://codereview.chromium.org/220353002/diff/1/apps/shell/browser/shell_ext...
apps/shell/browser/shell_extensions_browser_client.h:62: virtual void
AddExtensionPrefsObservers(content::BrowserContext* context,
nit: In the interface could you put this next to GetPrefServiceForContext() so
all the prefs stuff is together?

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_service.cc
(right):

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:15:
namespace {}  // namespace
delete this

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:31:
static base::LazyInstance<
Hrm. I'm not sure about this.  Usually these are file-level static variables.  I
doubt it matters here but I wonder if LazyInstance's thread safety guarantees
are still met with this usage pattern.  Not sure.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:78:
DependsOn(ExtensionPrefsFactory::GetInstance());
Does this depend on ExtensionPrefsFactory or the other way around?

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_service.h
(right):

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.h:32:
static BrowserContextKeyedAPIFactory<ContentSettingsService>*
Can this go in the private section below?

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.h:51:
static const bool kServiceHasOwnInstanceInIncognito = true;
Are you sure this is right? It looks like ExtensionPrefs is redirected in
incognito, so wouldn't that mean there is a shared ContentSettingsStore for
master and incognito?

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs.h:153: // general use. This is to allow
observers to be registered between
Could you instead call
ExtensionsBrowserClient::Get()->AddExtensionPrefObservers() from inside the
Initialize() method and avoid this complication?  Or make it
GetExtensionPrefObserver() and call it inside the constructor?  (I'm a little
nervous about calling an AddExtensionPrefObservers(this) on a partially
constructed this.)

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs.h:188: // callbed.
callbed -> called

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
File extensions/browser/extension_prefs_factory.cc (right):

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs_factory.cc:52:
client->AreExtensionsDisabled(*CommandLine::ForCurrentProcess(),
Another way to do this might be to pass an optional observer into the Create()
method.

[Ken Rockot(use gerrit already), 2014-04-01 21:34:59]

Thanks. I think this might be better.

https://codereview.chromium.org/220353002/diff/1/apps/shell/browser/shell_ext...
File apps/shell/browser/shell_extensions_browser_client.h (right):

https://codereview.chromium.org/220353002/diff/1/apps/shell/browser/shell_ext...
apps/shell/browser/shell_extensions_browser_client.h:62: virtual void
AddExtensionPrefsObservers(content::BrowserContext* context,
On 2014/04/01 18:45:03, James Cook wrote:
> nit: In the interface could you put this next to GetPrefServiceForContext() so
> all the prefs stuff is together?

Done.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_service.cc
(right):

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:15:
namespace {}  // namespace
On 2014/04/01 18:45:03, James Cook wrote:
> delete this

Nice.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:31:
static base::LazyInstance<
On 2014/04/01 18:45:03, James Cook wrote:
> Hrm. I'm not sure about this.  Usually these are file-level static variables. 
I
> doubt it matters here but I wonder if LazyInstance's thread safety guarantees
> are still met with this usage pattern.  Not sure.

I believe it's actually preferred, since it limits the scope and has no
undesirable side effects. Initialization time is a non-issue since LazyInstance
a POD type.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.cc:78:
DependsOn(ExtensionPrefsFactory::GetInstance());
On 2014/04/01 18:45:03, James Cook wrote:
> Does this depend on ExtensionPrefsFactory or the other way around?

This depends on ExtensionPrefsFactory.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_service.h
(right):

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.h:32:
static BrowserContextKeyedAPIFactory<ContentSettingsService>*
On 2014/04/01 18:45:03, James Cook wrote:
> Can this go in the private section below?

Is this comment on the right line? GetFactoryInstance can't be private.

https://codereview.chromium.org/220353002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_service.h:51:
static const bool kServiceHasOwnInstanceInIncognito = true;
On 2014/04/01 18:45:03, James Cook wrote:
> Are you sure this is right? It looks like ExtensionPrefs is redirected in
> incognito, so wouldn't that mean there is a shared ContentSettingsStore for
> master and incognito?

You're right. Fixed.

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs.h:153: // general use. This is to allow
observers to be registered between
On 2014/04/01 18:45:03, James Cook wrote:
> Could you instead call
> ExtensionsBrowserClient::Get()->AddExtensionPrefObservers() from inside the
> Initialize() method and avoid this complication?  Or make it
> GetExtensionPrefObserver() and call it inside the constructor?  (I'm a little
> nervous about calling an AddExtensionPrefObservers(this) on a partially
> constructed this.)

I was going to do that, but it looked like someone went to great lengths
(probably for testing :|) to be able to construct an ExtensionPrefs over a bunch
of concrete dependencies rather than allowing it to ever be aware of a
BrowserContext or related objects.

I'll revisit this and see what I can do.

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs.h:188: // callbed.
On 2014/04/01 18:45:03, James Cook wrote:
> callbed -> called

Done.

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
File extensions/browser/extension_prefs_factory.cc (right):

https://codereview.chromium.org/220353002/diff/1/extensions/browser/extension...
extensions/browser/extension_prefs_factory.cc:52:
client->AreExtensionsDisabled(*CommandLine::ForCurrentProcess(),
On 2014/04/01 18:45:03, James Cook wrote:
> Another way to do this might be to pass an optional observer into the Create()
> method.

True. I like this. Actually I'm going to have it take a vector of early
observers (I can't stomach adding a single special observer argument!) The EBC
method will populate a vector of observers.

ExtensionPrefsFactory will use this to populate its argument to Create.

[James Cook, 2014-04-01 22:09:37]

LGTM with nits

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_service.h
(right):

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_service.h:35: //
ExtensionPrefs::Observer implementation
nit: either end this one with "." or remove it from the one above

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_service.h:50:
static const char* service_name() { return "ContentSettingsService"; }
optional style nit: I might just stick this next to the other
BrowserContextKeyedApi stuff above.

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/DEPS
File extensions/browser/DEPS (left):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/DEPS#...
extensions/browser/DEPS:16:
"+chrome/browser/extensions/api/content_settings/content_settings_store.h",
FYI - I'm planning to pull all the extensions/*/DEPS exceptions into
extensions/DEPS.  Would you like me to wait until you land this, or do you want
to leave these here temporarily and let me clean them up later?

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:12: #include
"extensions/browser/extension_prefs.h"
I would extract ExtensionPrefsObserver into its own class so it can be forward
declared (and this include can go away).

It could either live in extension_prefs.h or its own file, whichever you prefer.

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:97: // are not owned by
ExtenionPrefs.
"ExtensionPrefs"

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
extensions/browser/test_extensions_browser_client.cc:137: void
TestExtensionsBrowserClient::AddExtensionPrefsObservers(
delete this

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
extensions/browser/test_extensions_browser_client.h:65: virtual void
AddExtensionPrefsObservers(content::BrowserContext* context,
delete this

[Ken Rockot(use gerrit already), 2014-04-01 22:41:37]

yoz@ can you please review as owner of */extensions/*?

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_service.h
(right):

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_service.h:35: //
ExtensionPrefs::Observer implementation
On 2014/04/01 22:09:37, James Cook wrote:
> nit: either end this one with "." or remove it from the one above

Done.

https://codereview.chromium.org/220353002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_service.h:50:
static const char* service_name() { return "ContentSettingsService"; }
On 2014/04/01 22:09:37, James Cook wrote:
> optional style nit: I might just stick this next to the other
> BrowserContextKeyedApi stuff above.

I don't see any fundamental reason for service_name() to stay private, but I'm
going to leave it this way since it's what everyone else does.

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/DEPS
File extensions/browser/DEPS (left):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/DEPS#...
extensions/browser/DEPS:16:
"+chrome/browser/extensions/api/content_settings/content_settings_store.h",
On 2014/04/01 22:09:37, James Cook wrote:
> FYI - I'm planning to pull all the extensions/*/DEPS exceptions into
> extensions/DEPS.  Would you like me to wait until you land this, or do you
want
> to leave these here temporarily and let me clean them up later?

I'll leave them (i.e. leave this file unchanged in this CL)

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:12: #include
"extensions/browser/extension_prefs.h"
On 2014/04/01 22:09:37, James Cook wrote:
> I would extract ExtensionPrefsObserver into its own class so it can be forward
> declared (and this include can go away).
> 
> It could either live in extension_prefs.h or its own file, whichever you
prefer.

Done.

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:97: // are not owned by
ExtenionPrefs.
On 2014/04/01 22:09:37, James Cook wrote:
> "ExtensionPrefs"

Done.

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/220353002/diff/40001/extensions/browser/test_...
extensions/browser/test_extensions_browser_client.cc:137: void
TestExtensionsBrowserClient::AddExtensionPrefsObservers(
On 2014/04/01 22:09:37, James Cook wrote:
> delete this

Done.

[Yoyo Zhou, 2014-04-02 01:40:33]

LGTM

https://chromiumcodereview.appspot.com/220353002/diff/80001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/220353002/diff/80001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:125:
observers->push_back(content_settings);
nit: can collapse 2 lines into 1

https://chromiumcodereview.appspot.com/220353002/diff/80001/extensions/browse...
File extensions/browser/extension_prefs_factory.cc (right):

https://chromiumcodereview.appspot.com/220353002/diff/80001/extensions/browse...
extensions/browser/extension_prefs_factory.cc:48:
client->GetAdditionalExtensionPrefsObservers(context, &prefs_observers);
Why not call this GetEarlyExtensionPrefsObservers (to match the argument name to
ExtensionPrefs::Create)?

[Ken Rockot(use gerrit already), 2014-04-02 03:53:01]

https://codereview.chromium.org/220353002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/220353002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_client.cc:125:
observers->push_back(content_settings);
On 2014/04/02 01:40:33, Yoyo Zhou wrote:
> nit: can collapse 2 lines into 1

Done.

https://codereview.chromium.org/220353002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_prefs_factory.cc (right):

https://codereview.chromium.org/220353002/diff/80001/extensions/browser/exten...
extensions/browser/extension_prefs_factory.cc:48:
client->GetAdditionalExtensionPrefsObservers(context, &prefs_observers);
On 2014/04/02 01:40:33, Yoyo Zhou wrote:
> Why not call this GetEarlyExtensionPrefsObservers (to match the argument name
to
> ExtensionPrefs::Create)?

SGTM - Done.

[Ken Rockot(use gerrit already), 2014-04-02 03:53:10]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-02 03:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/220353002/100001

[commit-bot: I haz the power, 2014-04-02 04:18:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 04:18:55]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Ken Rockot(use gerrit already), 2014-04-02 04:50:46]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-02 04:51:16]

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

[commit-bot: I haz the power, 2014-04-02 06:39:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 06:40:00]

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

[Ken Rockot(use gerrit already), 2014-04-02 16:36:48]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:37:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/220353002/140001

[commit-bot: I haz the power, 2014-04-02 16:43:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:43:59]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Ken Rockot(use gerrit already), 2014-04-02 16:50:03]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:50:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/220353002/140001

[Ken Rockot(use gerrit already), 2014-04-02 22:52:25]

Committed patchset #8 manually as r261207 (presubmit successful).