Add DCHECKs to StatefulExternalExtensionProvider

Add DCHECKs to StatefulExternalExtensionProvider

Also try to make the handling of pref_ more robust by making it private and always initializing it in the constructor of subclasses.

BUG=65925
TEST=ExtensionManagementTest.{ExternalUrlUpdate,ExternalPolicyRefresh}

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

[gfeher, 2010-12-14 13:09:05]

Please review.

[jochen (gone - plz use gerrit), 2010-12-14 13:40:43]

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_policy_extension_provider.cc (right):

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.cc:35: :
StatefulExternalExtensionProvider(
only indent 4 spaces

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_policy_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.h:16: //
prefs::kExtensionInstallForceList to look up which external extensions are
actually it uses whatever is passed in the constructor, right?

[jochen (gone - plz use gerrit), 2010-12-14 13:49:47]

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_policy_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.h:26: void
SetPreferences(const ListValue* forcelist);
are you still using this or can this be private?

[gfeher, 2010-12-14 14:23:15]

Would it make sense to use CHECKs instead of DCHECHKs?

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_policy_extension_provider.cc (right):

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.cc:35: :
StatefulExternalExtensionProvider(
On 2010/12/14 13:40:43, jochen wrote:
> only indent 4 spaces

Done.

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_policy_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.h:16: //
prefs::kExtensionInstallForceList to look up which external extensions are
Not exactly. Added comment.

It would be nice to have an observer for that preference here, but this object
unfortunately lives on the FILE thread.

http://codereview.chromium.org/5784004/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_policy_extension_provider.h:26: void
SetPreferences(const ListValue* forcelist);
No, this is used to refresh the internal list from the outside. The observer can
not be easily moved inside, because this object lives on the FILE thread, as all
the other Observers.

[gfeher, 2010-12-14 16:30:17]

According to Jochen's offline comments, I've added thread-checks.

It looks like that the providers are deleted on the UI thread. Probably this is
not a problem, because they are all owned by ExtensionsServiceBackend, which is
a RefCountedThreadSafe, and all the requests to the providers are going through
this backend.

[jochen (gone - plz use gerrit), 2010-12-15 08:57:49]

seems like you also need to fix the unit tests?

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_provider.h:18: // via
|SetPreferences| in case of run-time updates.
please add a comment that the object must be created and destroyed on the UI
thread and that all of its members must be invoked on the FILE thread

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/s...
File chrome/browser/extensions/stateful_external_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/s...
chrome/browser/extensions/stateful_external_extension_provider.h:22: // are
specified at the constructor.
please add a comment that the object must be created and destroyed on the UI
thread and that all of its public members must be invoked on the FILE thread

[gfeher, 2010-12-15 13:44:58]

All done. Please take another look.

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_provider.h:18: // via
|SetPreferences| in case of run-time updates.
On 2010/12/15 08:57:49, jochen wrote:
> please add a comment that the object must be created and destroyed on the UI
> thread and that all of its members must be invoked on the FILE thread

Done.

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/s...
File chrome/browser/extensions/stateful_external_extension_provider.h (right):

http://codereview.chromium.org/5784004/diff/22001/chrome/browser/extensions/s...
chrome/browser/extensions/stateful_external_extension_provider.h:22: // are
specified at the constructor.
On 2010/12/15 08:57:49, jochen wrote:
> please add a comment that the object must be created and destroyed on the UI
> thread and that all of its public members must be invoked on the FILE thread

Done.

[jochen (gone - plz use gerrit), 2010-12-15 16:00:12]

LGTM thx