ABANDONED: Remove Chrome dependencies from TestExtensionPrefs.

Remove Chrome dependencies from TestExtensionPrefs.

This makes TestExtensionPrefs stop using PrefServiceSyncable
so that TEP can eventually be moved to the toplevel
extensions/ directory. preference_api_prefs_unittest.cc
depends on PrefServiceSyncable being used (so it can create
an incognito PrefService), so TEP now takes an optional
PrefServiceFactory (which can be e.g. a Chrome
PrefServiceMockFactory object).

BUG=313284

[Daniel Erat, 2013-12-06 18:00:03]

This is pretty gross. Let me know if you have any better ideas.

[miket_OOO, 2013-12-06 20:42:02]

No better ideas. I considered a setter instead of the optional constructor
argument, but I think that just makes it more complicated in the name of
avoiding a few deltas in otherwise unaffected files. LGTM.

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:67: new
base::PrefServiceFactory),
After offline discussion: it looks like you've expanded the
PrefServiceMockFactory here into a real one that does the same work that test
class was doing (principally setting user prefs to a new TestingPrefStore). This
was the easiest path because of an ongoing PrefServiceSyncable dependency. This
is all fine, just wanted something in print here explaining why the mock seems
to have disappeared (and this comment constitutes that "something in print").

[Daniel Erat, 2013-12-06 20:47:54]

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:67: new
base::PrefServiceFactory),
On 2013/12/06 20:42:03, miket wrote:
> After offline discussion: it looks like you've expanded the
> PrefServiceMockFactory here into a real one that does the same work that test
> class was doing (principally setting user prefs to a new TestingPrefStore).
This
> was the easiest path because of an ongoing PrefServiceSyncable dependency.
This
> is all fine, just wanted something in print here explaining why the mock seems
> to have disappeared (and this comment constitutes that "something in print").

Just to elaborate: PrefServiceMockFactory doesn't actually appear to be a mock.
It derives from PrefServiceSyncableFactory with no differences apart from a
"user_prefs_ = new TestingPrefStore;" in its constructor. That's the reason for
the set_user_prefs() call that I added below.

[Daniel Erat, 2013-12-06 21:01:17]

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (left):

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:111: pref_service_ =
factory.CreateSyncable(pref_registry_.get()).Pass();
ugh, i think i see the problem. i changed this CreateSyncable() call to
Create(). PrefServiceSyncableFactory (which PrefServiceMockFactory derives from)
doesn't override Create(), so it's presumably returning a normal PrefService
instead of a PrefServiceSyncable.

i'm not sure why PrefServiceSyncableFactory adds a new create method instead of
overriding the existing one -- maybe it's so it can return a
scoped_ptr<PrefServiceSyncable> instead of a scoped_ptr<PrefService>?

i'm not sure what the solution is. i could change PrefServiceSyncableFactory to
override Create() instead and then make its (few) callers do the downcast if
they need a PrefServiceSyncable.

[Daniel Erat, 2013-12-06 21:07:42]

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (left):

https://codereview.chromium.org/108643002/diff/1/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:111: pref_service_ =
factory.CreateSyncable(pref_registry_.get()).Pass();
On 2013/12/06 21:01:17, Daniel Erat wrote:
> ugh, i think i see the problem. i changed this CreateSyncable() call to
> Create(). PrefServiceSyncableFactory (which PrefServiceMockFactory derives
from)
> doesn't override Create(), so it's presumably returning a normal PrefService
> instead of a PrefServiceSyncable.
> 
> i'm not sure why PrefServiceSyncableFactory adds a new create method instead
of
> overriding the existing one -- maybe it's so it can return a
> scoped_ptr<PrefServiceSyncable> instead of a scoped_ptr<PrefService>?
> 
> i'm not sure what the solution is. i could change PrefServiceSyncableFactory
to
> override Create() instead and then make its (few) callers do the downcast if
> they need a PrefServiceSyncable.

worse than i thought: CreateSyncable also takes a PrefRegistrySyncable rather
than a PrefRegistry (as Create does). the registry gets passed to
PrefServiceSyncable's c'tor, which requires a syncable registry.

maybe i should just keep all of this testing code in chrome/browser/extensions.
the downside is that extension_prefs_unittest.cc uses TestExtensionPrefs, so it
wouldn't be able to live alongside extension_prefs.cc after that gets moved.