Split PrefService into PrefService, PrefServiceSimple and PrefServiceSyncable.

Split PrefService into PrefService, PrefServiceSimple and PrefServiceSyncable.

The first two (PrefServiceSimple is a subclass of PrefService) know
nothing about sync or any Chrome or content concepts.

The third (PrefServiceSyncable, a separate subclass of PrefService)
knows about sync and requires users to choose whether each individual
preference is syncable or not when it is registered.

BrowserProcess::local_state() is a PrefServiceSimple after this
change, and Profile::prefs() is a PrefServiceSyncable.

COLLABORATOR=kaiwang@chromium.org
TBR=ben@chromium.org
BUG=155525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174531

[Jói, 2012-12-13 16:58:39]

Hi Mattias,

Here's a preview of the way we're splitting PrefService into different parts. 
Obviously each class will be in a separate file when this is done, and most
callers haven't been updated yet (and there are probably lots of bugs I don't
know about).

Just sending it out now to get any early feedback on the split; do you agree
this is on the right track?

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-12-14 13:50:05]

Looked at the prefs files and left a few comments. Direction looks good in
general.

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/chrom...
File chrome/browser/prefs/chrome_pref_service_builder.h (right):

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/chrom...
chrome/browser/prefs/chrome_pref_service_builder.h:16: class
ChromePrefServiceBuilder : public PrefServiceBuilder {
As mentioned before, I think you can just inline the code at the PrefService
creation sites, assuming the PrefServiceBuilder API is nice enough ;)

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.h (right):

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.h:94: PrefService();
Please not. Let's pass all dependencies through the constructor so we have a
properly initialized PrefService at any time. Then, make PrefServiceBuilder
collect the parameters and call the ctor in its Build() method.

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_builder.h:52: virtual PrefService*
Build(PrefService* pref_service);
Ah, now I understand the issue :) You want the builder to be able to build both
PrefServiceSimple and PrefServiceSyncable. I'd just create subclasses here that
override Build and pass on the parameters.

[kaiwang, 2012-12-15 00:10:06]

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_builder.h:52: virtual PrefService*
Build(PrefService* pref_service);
Initializing PrefServiceSimple and PrefServiceSyncable are exactly same code, so
write sub-builders may not worth. I just changed the code to make constructors
private for PrefServices. This can also make sure no uninitialized PrefService
exist

[Jói, 2012-12-17 16:34:52]

Mattias, for clarification:  Kai and I are working on this change
together, e.g. right now I am continuing where Kai left off when his
day ended on Friday in MTV.  So you may see responses from either of
us on the review thread.

Cheers,
Jói


On Sat, Dec 15, 2012 at 12:10 AM,  <kaiwang@chromium.org> wrote:
>
>
https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
> File chrome/browser/prefs/pref_service_builder.h (right):
>
>
https://codereview.chromium.org/11570009/diff/3001/chrome/browser/prefs/pref_...
> chrome/browser/prefs/pref_service_builder.h:52: virtual PrefService*
> Build(PrefService* pref_service);
> Initializing PrefServiceSimple and PrefServiceSyncable are exactly same
> code, so write sub-builders may not worth. I just changed the code to
> make constructors private for PrefServices. This can also make sure no
> uninitialized PrefService exist
>
> https://codereview.chromium.org/11570009/

[Jói, 2012-12-18 14:47:57]

Hi Mattias,

The latest patch set is ready for review. This is collaborative work by Kai and
myself. We are tracking down a bug in a few tests but I doubt the fix will be a
large change, so I think we should get the review started.

The change is quite large, so we're using a tool to help get it reviewed.  The
tool is a Python script that splits the change into files that can be
mechanically verified (and you can verify that the rules used by the tool are
correct), vs. files that need manual inspection.

I will send you the tool itself, as well as the patches generated by the tool
(the "mechanical" patch, that should only require a glance to make sure the tool
is doing the right thing, and the "manual" patch, which requires full review). 
I have also created a Rietveld issue for the bits of this change that the tool
identified as requiring manual review; see
https://codereview.chromium.org/11573065/.

Cheers,
Jói

[Jói, 2012-12-18 15:00:57]

Attached are the bits I mentioned:
- review_helper.py
- original patch generated by a local [ git diff ]
- patch set for manual review
- patch set that the tool verified is "mechanical" as per the rules in
review_helper.py

The script review_helper.py for now is not generalized; you will see
that its rules are encoded near the top of the CategorizePerFileDiffs
function.  I will work to generalize the tool and add unit tests for
it, since I think it will be generally useful (I have used variants of
it before, for reviews that were part of the content refactoring).

To explain the script's parameters, it will add a file to the
"mechanical" set of files if all of its - and + lines are matched by
one of the following types of rules:
- For pairs of -+ lines (one or more - lines directly followed by one
or more + lines), it verifies that either all of the + lines in the
pair are matched by one of the |ignore_plus_matches| rules, or the
before and after versions of the pair become identical when the
|transform_minus_to_comparative_form| and
|transform_plus_to_comparative_form| transformations have been applied
to the minus and plus versions.
- For individual + lines, it verifies that they are matched by one of
the |ignore_plus_matches| rules.
- For individual - lines, it verifies that they are matched by one of
the |ignore_minus_matches| rules.
- If any of the above do not hold true, the file is to be manually reviewed.

If you look at the ignore and transform rules I wrote, you will see
that files where we have just replaced (Testing)PrefService with
either (Testing)PrefServiceSimple or (Testing)PrefServiceSyncable, or
updated some common related #includes or added forward definitions of
one of the new classes, are getting moved to the mechanical list.  We
also attempt to categorize as mechanically-reviewed those files where
we have just removed the
PrefService::SYNCABLE_PREF/PrefService::UNSYNCABLE_PREF parameters
(which are not required when using PrefServiceSimple).

I hope this helps, and makes the review manageable.  The
manually-reviewable change is still quite large at 64 files modified
(out of ~370 in the full change) and about 14 files with non-trivial
changes, but this is a large change, no way around it.

The files with non-trivial changes, as far as I recall, are:
- pref_service.*
- pref_service_simple.*
- pref_service_syncable.*
- testing_pref_service.*
- chrome_pref_service_factory.*
- pref_service_builder.*
- pref_service_base.h
- browser_prefs.cc

Your review links are:
- full review at https://codereview.chromium.org/11570009/
- bits that need manual review at https://codereview.chromium.org/11573065/

Cheers,
Jói


On Tue, Dec 18, 2012 at 2:47 PM,  <joi@chromium.org> wrote:
> Hi Mattias,
>
> The latest patch set is ready for review. This is collaborative work by Kai
> and
> myself. We are tracking down a bug in a few tests but I doubt the fix will
> be a
> large change, so I think we should get the review started.
>
> The change is quite large, so we're using a tool to help get it reviewed.
> The
> tool is a Python script that splits the change into files that can be
> mechanically verified (and you can verify that the rules used by the tool
> are
> correct), vs. files that need manual inspection.
>
> I will send you the tool itself, as well as the patches generated by the
> tool
> (the "mechanical" patch, that should only require a glance to make sure the
> tool
> is doing the right thing, and the "manual" patch, which requires full
> review).
> I have also created a Rietveld issue for the bits of this change that the
> tool
> identified as requiring manual review; see
> https://codereview.chromium.org/11573065/.
>
> Cheers,
> Jói
>
>
> https://codereview.chromium.org/11570009/

[Mattias Nissler (ping if slow), 2012-12-19 19:50:21]

This review is both massive and important, and I've been sheriffing yesterday
and today, so I'm not done yet. Sending what I have though so you can think
about the comments.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/autofill/D...
chrome/browser/autofill/DEPS:15: # Will go away once Prefs completes its move to
base/prefs/.
How so? You need the syncable bits, which will never be in base/prefs AFAIU.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_utils.cc:22: #include
"chrome/browser/profiles/profile.h"
remove duplicate #include

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:403: PrefServiceSimple*
BrowserProcessImpl::local_state() {
Is there any reason why we can't return PrefServiceBase* here? Whoever calls
browser_process->local_state() should never register prefs ad hoc on the
returned PrefService. Returning PrefServiceBase here would allow us to enforce
that.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:754:
local_state_->UpdateCommandLinePrefStore(CommandLine::ForCurrentProcess());
Ugh, UpdateCommandLinePrefStore shouldn't be in the public PrefService
interface. Filed https://code.google.com/p/chromium/issues/detail?id=166877

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/component_...
File chrome/browser/component_updater/recovery_component_installer.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/component_...
chrome/browser/component_updater/recovery_component_installer.cc:66:
prefs->RegisterStringPref(prefs::kRecoveryComponentVersion, "0.0.0.0");
random note: This shouldn't be registered here but handled via
browser_prefs::RegisterLocalState()

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:282:
PrefServiceSyncable* pref_service() const { return profile_->GetPrefs(); }
Is this accessor really necessary? It seems the code in SetUp() could do
without.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:321:
profile_->SetPrefService(new TestingPrefServiceSyncable());
This shouldn't be necessary, TestingProfile already has a PrefService.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:325:
ProtocolHandlerRegistry::RegisterUserPrefs(pref_service());
This should be removed if you remove line 321.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/debugger/d...
File chrome/browser/debugger/devtools_window.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/debugger/d...
chrome/browser/debugger/devtools_window.cc:382:
PrefServiceSyncable::UNSYNCABLE_PREF);
random note: This should go through browser_prefs::RegisterUserPrefs

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:311:
pref_service_ = profile()->GetTestingPrefService();
used?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs.cc:1642: PrefServiceSyncable* service =
profile()->GetPrefs();
Can we use PrefServiceBase here?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:185:
TestingPrefServiceSyncable* prefs = profile_.GetTestingPrefService();
could just use profile_->GetPrefs()->Set();

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:29: PrefServiceSimple*
local_state);
Can we use PrefServiceBase* for these two?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:96: PrefServiceSyncable* prefs,
Can we pass PrefServiceBase*?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:540:
local_state->RegisterBooleanPref(prefs::kShouldShowWelcomePage, false);
random note: this should happen via browser_prefs::RegisterLocalState()

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:552: local_state->RegisterBooleanPref(
same here.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/gpu/gl_str...
File chrome/browser/gpu/gl_string_manager.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/gpu/gl_str...
chrome/browser/gpu/gl_string_manager.cc:30:
local_state->RegisterStringPref(prefs::kGLVersionString, gl_version_);
Random note: Registrations should go through browser_process::RegisterLocalState

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:250:
ProtocolHandlerRegistry::RegisterUserPrefs(user_prefs);
I don't see why you renamed this.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:304:
NetworkProfileBubble::RegisterUserPrefs(user_prefs);
why rename?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:11: class PrefServiceSimple : public
PrefService {
Thinking more about these names: I'm wondering whether this would be easier to
understand for people if we named this LocalStatePrefService and the one used
for profile prefs ProfilePrefService, respectively. In general, I'm in favor of
naming a class based on what it does instead of what it's used for, but in this
case we might reduce confusing amongst developers significantly (what exactly is
Simple? Does Syncable imply everything gets synced?). WDYT?

[Mattias Nissler (ping if slow), 2012-12-20 13:41:51]

Yay, I made it through \o/

High-level comments (in no particular order):

* This change is too big, which slows down reviews and thus iteration speed. I
think this could have been done in steps: Add PrefService derivatives but keep
PrefService interface intact -> convert consumers one by one -> remove unused
interface from PrefService. I'm not saying you should change this CL, but I
guess you'd appreciate faster reviews in the future :)

* Having looked at the code, I'm no longer convinced the path we're on is the
right one. We might be in the process of committing the standard
OO-language-crime: Overusing subclassing because that's the main abstraction
mechanism everybody is used to. Standard remedy: Use delegation instead!

Dogmas aside, let's take some time to consider the delegation approach: We do
remove the RegisterXYZ() calls from PrefService, but don't implement RegisterXYZ
in subclasses, but make that the responsibility of a separate PrefRegistry (for
lack of a better name) interface. We pass the PrefRegistry to PrefService on
construction time, and PrefService uses it to  check what preferences are
registered and obtain their types. Then, we can have different PrefRegistry
implementations for local_state and user prefs, the former without the syncable
flag, the latter including it.

Benefits include:
- Pref registration is no longer possible if you just have a PrefService
pointer, but no access to the associated PrefRegistry. This property is nice
because it enforces the rule that pref registrations are all supposed to be
handled via browser_prefs::Register(Local|User)Prefs().
- The awkwardness in PrefServiceBuilder which has to initialize two different
types of PrefServices goes away. There's now only a single type and we're
passing the PrefRegistry pointer as an additional parameter.
- Same goes for TestingPrefService, no subclassing needed here either.

WDYT?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread....
chrome/browser/io_thread.h:169: IOThread(PrefServiceSimple* local_state,
Can we pass PrefServiceBase* here?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
File chrome/browser/policy/policy_statistics_collector.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
chrome/browser/policy/policy_statistics_collector.h:15: class
PrefServiceSyncable;
needed?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.cc:85: PrefServiceSimple*
pref_service = new PrefServiceSimple();
I think I posted this somewhere already, but to reiterate: The builder should
really call the constructor, I think we want separate PrefServiceBuilder
subclasses for the separate PrefService derivatives. I'd rather not see
half-initialized PrefService instances.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:37: // The PrefService takes
ownership of |extension_pref_store|.
PrefStores are RefCounted, please remove this sentence.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:28: PrefServiceSimple();
needs a virtual dtor

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:96: // TODO(zea): Have PrefService
implement syncer::SyncableService directly.
update comment

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:119: // Constructs objects.
s/objects/instances/

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:125: 
documentation.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:127: const char* path, Value*
default_value, PrefSyncStatus sync_status);
param decls on separate lines.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:135:
ObserverList<PrefServiceObserver> observer_list_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:106: virtual PrefServiceSyncable*
GetOffTheRecordPrefs() OVERRIDE;
Can we return PrefServiceBase* for these two?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:303: void SyncPrefs::RegisterPreferences() {
Random note: This should be called via browser_prefs::RegisterUserPrefs.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:158: window_pref.append(app_name);
random note: This should register a single dictionary preference instead of
construction pref names.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:123:
RegisterUserPrefs(profile_->GetPrefs());
Random note: This should be called via browser_prefs::RegisterUserPrefs();

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:26: static void
RegisterPrefs(PrefServiceSimple* prefs);
dead code?

[Jói, 2012-12-20 14:40:39]

Hi Mattias,

Thanks for the review.  I was in the process of working through your
specific comments, but it looks like we should first address your
PrefRegistry idea before we go further.

I agree having a separate PrefRegistry object might be cleaner.  I wish we
had thought of it in our design discussion thread a week ago, or 6 days ago
in the preview version of this change, but we can't change the past :)

I think we'll still end up with a Chrome-specific subclass of PrefService
that handles integration with sync, knows about incognito prefs, and a few
other things, i.e. what we call PrefServiceSyncable today.

My suggestion for a way forward is this:
a) Land the current change after addressing your specific comments.
b) In a follow-up, switch BrowserProcess::local_state() to returning a
PrefService* (or PrefServiceBase*), and make sure all registration (to
local state or to profile prefs) is done through the
browser_prefs::RegisterXyz functions.
c) At that point, we'll have code using either PrefService or
PrefServiceSyncable, and registrations will be done through only one code
path (not counting tests). Then it should be easier to switch from having
the RegisterXyzPreference methods in the two different subclasses to having
them in separate subclasses of PrefRegistry, at which point we would also
remove PrefServiceSimple and be down to PrefService vs. PrefServiceSyncable.
d) After that, we could see if it makes sense to also remove the
PrefServiceSyncable subclass.  I think this would mean changing the way
integration with sync is done, but it's quite possible that it will make
sense.

WDYT?

Cheers,
Jói



On Thu, Dec 20, 2012 at 1:41 PM, <mnissler@chromium.org> wrote:

> Yay, I made it through \o/
>
> High-level comments (in no particular order):
>
> * This change is too big, which slows down reviews and thus iteration
> speed. I
> think this could have been done in steps: Add PrefService derivatives but
> keep
> PrefService interface intact -> convert consumers one by one -> remove
> unused
> interface from PrefService. I'm not saying you should change this CL, but I
> guess you'd appreciate faster reviews in the future :)
>
> * Having looked at the code, I'm no longer convinced the path we're on is
> the
> right one. We might be in the process of committing the standard
> OO-language-crime: Overusing subclassing because that's the main
> abstraction
> mechanism everybody is used to. Standard remedy: Use delegation instead!
>
> Dogmas aside, let's take some time to consider the delegation approach: We
> do
> remove the RegisterXYZ() calls from PrefService, but don't implement
> RegisterXYZ
> in subclasses, but make that the responsibility of a separate PrefRegistry
> (for
> lack of a better name) interface. We pass the PrefRegistry to PrefService
> on
> construction time, and PrefService uses it to  check what preferences are
> registered and obtain their types. Then, we can have different PrefRegistry
> implementations for local_state and user prefs, the former without the
> syncable
> flag, the latter including it.
>
> Benefits include:
> - Pref registration is no longer possible if you just have a PrefService
> pointer, but no access to the associated PrefRegistry. This property is
> nice
> because it enforces the rule that pref registrations are all supposed to be
> handled via browser_prefs::Register(Local|**User)Prefs().
> - The awkwardness in PrefServiceBuilder which has to initialize two
> different
> types of PrefServices goes away. There's now only a single type and we're
> passing the PrefRegistry pointer as an additional parameter.
> - Same goes for TestingPrefService, no subclassing needed here either.
>
> WDYT?
>
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/io_thread.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h>
> File chrome/browser/io_thread.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/io_thread.h#**newcode169<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h#newcode169>
> chrome/browser/io_thread.h:**169: IOThread(PrefServiceSimple* local_state,
> Can we pass PrefServiceBase* here?
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/policy/policy_**statistics_collector.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h>
> File chrome/browser/policy/policy_**statistics_collector.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/policy/policy_**statistics_collector.h#**newcode15<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h#newcode15>
> chrome/browser/policy/policy_**statistics_collector.h:15: class
> PrefServiceSyncable;
> needed?
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/chrome_**pref_service_factory.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc>
> File chrome/browser/prefs/chrome_**pref_service_factory.cc (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/chrome_**pref_service_factory.cc#**newcode85<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode85>
> chrome/browser/prefs/chrome_**pref_service_factory.cc:85:
> PrefServiceSimple* pref_service = new PrefServiceSimple();
> I think I posted this somewhere already, but to reiterate: The builder
> should really call the constructor, I think we want separate
> PrefServiceBuilder subclasses for the separate PrefService derivatives.
> I'd rather not see half-initialized PrefService instances.
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/chrome_**pref_service_factory.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h>
> File chrome/browser/prefs/chrome_**pref_service_factory.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/chrome_**pref_service_factory.h#**newcode37<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h#newcode37>
> chrome/browser/prefs/chrome_**pref_service_factory.h:37: // The
> PrefService takes ownership of |extension_pref_store|.
> PrefStores are RefCounted, please remove this sentence.
>
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_simple.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h>
> File chrome/browser/prefs/pref_**service_simple.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_simple.h#newcode28<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h#newcode28>
> chrome/browser/prefs/pref_**service_simple.h:28: PrefServiceSimple();
> needs a virtual dtor
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h>
> File chrome/browser/prefs/pref_**service_syncable.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h#newcode96<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode96>
> chrome/browser/prefs/pref_**service_syncable.h:96: // TODO(zea): Have
> PrefService implement syncer::SyncableService directly.
> update comment
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h#newcode119<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode119>
> chrome/browser/prefs/pref_**service_syncable.h:119: // Constructs objects.
> s/objects/instances/
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h#newcode125<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode125>
> chrome/browser/prefs/pref_**service_syncable.h:125:
> documentation.
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h#newcode127<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode127>
> chrome/browser/prefs/pref_**service_syncable.h:127: const char* path,
> Value* default_value, PrefSyncStatus sync_status);
> param decls on separate lines.
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/prefs/pref_**service_syncable.h#newcode135<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode135>
> chrome/browser/prefs/pref_**service_syncable.h:135:
> ObserverList<**PrefServiceObserver> observer_list_;
> DISALLOW_COPY_AND_ASSIGN
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/profiles/**profile_impl.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h>
> File chrome/browser/profiles/**profile_impl.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/profiles/**profile_impl.h#newcode106<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h#newcode106>
> chrome/browser/profiles/**profile_impl.h:106: virtual PrefServiceSyncable*
> GetOffTheRecordPrefs() OVERRIDE;
> Can we return PrefServiceBase* for these two?
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/sync/sync_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc>
> File chrome/browser/sync/sync_**prefs.cc (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/sync/sync_**prefs.cc#newcode303<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc#newcode303>
> chrome/browser/sync/sync_**prefs.cc:303: void
> SyncPrefs::**RegisterPreferences() {
> Random note: This should be called via browser_prefs::**RegisterUserPrefs.
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/browser_ui_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc>
> File chrome/browser/ui/browser_ui_**prefs.cc (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/browser_ui_**prefs.cc#newcode158<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc#newcode158>
> chrome/browser/ui/browser_ui_**prefs.cc:158: window_pref.append(app_name);
> random note: This should register a single dictionary preference instead
> of construction pref names.
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/views/**browser_actions_container.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc>
> File chrome/browser/ui/views/**browser_actions_container.cc (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/views/**browser_actions_container.cc#**newcode123<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc#newcode123>
> chrome/browser/ui/views/**browser_actions_container.cc:**123:
> RegisterUserPrefs(profile_->**GetPrefs());
> Random note: This should be called via
> browser_prefs::**RegisterUserPrefs();
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h>
> File chrome/browser/ui/webui/ntp/**new_tab_page_handler.h (right):
>
> https://codereview.chromium.**org/11570009/diff/21004/**
>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h#**newcode26<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h#newcode26>
> chrome/browser/ui/webui/ntp/**new_tab_page_handler.h:26: static void
> RegisterPrefs(**PrefServiceSimple* prefs);
> dead code?
>
>
https://codereview.chromium.**org/11570009/<https://codereview.chromium.org/1...
>

[Jói, 2012-12-20 14:58:46]

BTW, I'm looking into this some more; perhaps there is a way to mutate the
current change more effectively into the PrefRegistry idea.  Perhaps the
ChromePrefService (née PrefServiceSyncable) can also be something that
aggregates a PrefService and handles the sync stuff, rather than being a
subclass.  It might still be most efficient to land the current change
first, then try to use just PrefService* everywhere we can, then we'll have
fewer places to fix in the end (I think).  The two TestingPrefService
subclasses will remain a bit ugly in the interim, but should in the end
fold back down to one as you suggested we could do.


On Thu, Dec 20, 2012 at 2:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi Mattias,
>
> Thanks for the review.  I was in the process of working through your
> specific comments, but it looks like we should first address your
> PrefRegistry idea before we go further.
>
> I agree having a separate PrefRegistry object might be cleaner.  I wish we
> had thought of it in our design discussion thread a week ago, or 6 days ago
> in the preview version of this change, but we can't change the past :)
>
> I think we'll still end up with a Chrome-specific subclass of PrefService
> that handles integration with sync, knows about incognito prefs, and a few
> other things, i.e. what we call PrefServiceSyncable today.
>
> My suggestion for a way forward is this:
> a) Land the current change after addressing your specific comments.
> b) In a follow-up, switch BrowserProcess::local_state() to returning a
> PrefService* (or PrefServiceBase*), and make sure all registration (to
> local state or to profile prefs) is done through the
> browser_prefs::RegisterXyz functions.
> c) At that point, we'll have code using either PrefService or
> PrefServiceSyncable, and registrations will be done through only one code
> path (not counting tests). Then it should be easier to switch from having
> the RegisterXyzPreference methods in the two different subclasses to having
> them in separate subclasses of PrefRegistry, at which point we would also
> remove PrefServiceSimple and be down to PrefService vs. PrefServiceSyncable.
> d) After that, we could see if it makes sense to also remove the
> PrefServiceSyncable subclass.  I think this would mean changing the way
> integration with sync is done, but it's quite possible that it will make
> sense.
>
> WDYT?
>
> Cheers,
> Jói
>
>
>
> On Thu, Dec 20, 2012 at 1:41 PM, <mnissler@chromium.org> wrote:
>
>> Yay, I made it through \o/
>>
>> High-level comments (in no particular order):
>>
>> * This change is too big, which slows down reviews and thus iteration
>> speed. I
>> think this could have been done in steps: Add PrefService derivatives but
>> keep
>> PrefService interface intact -> convert consumers one by one -> remove
>> unused
>> interface from PrefService. I'm not saying you should change this CL, but
>> I
>> guess you'd appreciate faster reviews in the future :)
>>
>> * Having looked at the code, I'm no longer convinced the path we're on is
>> the
>> right one. We might be in the process of committing the standard
>> OO-language-crime: Overusing subclassing because that's the main
>> abstraction
>> mechanism everybody is used to. Standard remedy: Use delegation instead!
>>
>> Dogmas aside, let's take some time to consider the delegation approach:
>> We do
>> remove the RegisterXYZ() calls from PrefService, but don't implement
>> RegisterXYZ
>> in subclasses, but make that the responsibility of a separate
>> PrefRegistry (for
>> lack of a better name) interface. We pass the PrefRegistry to PrefService
>> on
>> construction time, and PrefService uses it to  check what preferences are
>> registered and obtain their types. Then, we can have different
>> PrefRegistry
>> implementations for local_state and user prefs, the former without the
>> syncable
>> flag, the latter including it.
>>
>> Benefits include:
>> - Pref registration is no longer possible if you just have a PrefService
>> pointer, but no access to the associated PrefRegistry. This property is
>> nice
>> because it enforces the rule that pref registrations are all supposed to
>> be
>> handled via browser_prefs::Register(Local|**User)Prefs().
>> - The awkwardness in PrefServiceBuilder which has to initialize two
>> different
>> types of PrefServices goes away. There's now only a single type and we're
>> passing the PrefRegistry pointer as an additional parameter.
>> - Same goes for TestingPrefService, no subclassing needed here either.
>>
>> WDYT?
>>
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/io_thread.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h>
>> File chrome/browser/io_thread.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/io_thread.h#**newcode169<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h#newcode169>
>> chrome/browser/io_thread.h:**169: IOThread(PrefServiceSimple*
>> local_state,
>> Can we pass PrefServiceBase* here?
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/policy/policy_**statistics_collector.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h>
>> File chrome/browser/policy/policy_**statistics_collector.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/policy/policy_**statistics_collector.h#**newcode15<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h#newcode15>
>> chrome/browser/policy/policy_**statistics_collector.h:15: class
>> PrefServiceSyncable;
>> needed?
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/chrome_**pref_service_factory.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc>
>> File chrome/browser/prefs/chrome_**pref_service_factory.cc (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/chrome_**pref_service_factory.cc#**newcode85<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode85>
>> chrome/browser/prefs/chrome_**pref_service_factory.cc:85:
>> PrefServiceSimple* pref_service = new PrefServiceSimple();
>> I think I posted this somewhere already, but to reiterate: The builder
>> should really call the constructor, I think we want separate
>> PrefServiceBuilder subclasses for the separate PrefService derivatives.
>> I'd rather not see half-initialized PrefService instances.
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/chrome_**pref_service_factory.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h>
>> File chrome/browser/prefs/chrome_**pref_service_factory.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/chrome_**pref_service_factory.h#**newcode37<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h#newcode37>
>> chrome/browser/prefs/chrome_**pref_service_factory.h:37: // The
>> PrefService takes ownership of |extension_pref_store|.
>> PrefStores are RefCounted, please remove this sentence.
>>
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_simple.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h>
>> File chrome/browser/prefs/pref_**service_simple.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_simple.h#newcode28<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h#newcode28>
>> chrome/browser/prefs/pref_**service_simple.h:28: PrefServiceSimple();
>> needs a virtual dtor
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h>
>> File chrome/browser/prefs/pref_**service_syncable.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h#newcode96<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode96>
>> chrome/browser/prefs/pref_**service_syncable.h:96: // TODO(zea): Have
>> PrefService implement syncer::SyncableService directly.
>> update comment
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h#newcode119<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode119>
>> chrome/browser/prefs/pref_**service_syncable.h:119: // Constructs
>> objects.
>> s/objects/instances/
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h#newcode125<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode125>
>> chrome/browser/prefs/pref_**service_syncable.h:125:
>> documentation.
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h#newcode127<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode127>
>> chrome/browser/prefs/pref_**service_syncable.h:127: const char* path,
>> Value* default_value, PrefSyncStatus sync_status);
>> param decls on separate lines.
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/prefs/pref_**service_syncable.h#newcode135<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode135>
>> chrome/browser/prefs/pref_**service_syncable.h:135:
>> ObserverList<**PrefServiceObserver> observer_list_;
>> DISALLOW_COPY_AND_ASSIGN
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/profiles/**profile_impl.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h>
>> File chrome/browser/profiles/**profile_impl.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/profiles/**profile_impl.h#newcode106<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h#newcode106>
>> chrome/browser/profiles/**profile_impl.h:106: virtual
>> PrefServiceSyncable*
>> GetOffTheRecordPrefs() OVERRIDE;
>> Can we return PrefServiceBase* for these two?
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/sync/sync_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc>
>> File chrome/browser/sync/sync_**prefs.cc (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/sync/sync_**prefs.cc#newcode303<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc#newcode303>
>> chrome/browser/sync/sync_**prefs.cc:303: void
>> SyncPrefs::**RegisterPreferences() {
>> Random note: This should be called via browser_prefs::**
>> RegisterUserPrefs.
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/browser_ui_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc>
>> File chrome/browser/ui/browser_ui_**prefs.cc (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/browser_ui_**prefs.cc#newcode158<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc#newcode158>
>> chrome/browser/ui/browser_ui_**prefs.cc:158:
>> window_pref.append(app_name);
>> random note: This should register a single dictionary preference instead
>> of construction pref names.
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/views/**browser_actions_container.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc>
>> File chrome/browser/ui/views/**browser_actions_container.cc (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/views/**browser_actions_container.cc#**newcode123<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc#newcode123>
>> chrome/browser/ui/views/**browser_actions_container.cc:**123:
>> RegisterUserPrefs(profile_->**GetPrefs());
>> Random note: This should be called via
>> browser_prefs::**RegisterUserPrefs();
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h>
>> File chrome/browser/ui/webui/ntp/**new_tab_page_handler.h (right):
>>
>> https://codereview.chromium.**org/11570009/diff/21004/**
>>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h#**newcode26<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h#newcode26>
>> chrome/browser/ui/webui/ntp/**new_tab_page_handler.h:26: static void
>> RegisterPrefs(**PrefServiceSimple* prefs);
>> dead code?
>>
>>
https://codereview.chromium.**org/11570009/<https://codereview.chromium.org/1...
>>
>
>

[Mattias Nissler (ping if slow), 2012-12-20 15:21:52]

Comments inline.

On Thu, Dec 20, 2012 at 3:58 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> BTW, I'm looking into this some more; perhaps there is a way to mutate the
> current change more effectively into the PrefRegistry idea.  Perhaps the
> ChromePrefService (née PrefServiceSyncable) can also be something that
> aggregates a PrefService and handles the sync stuff, rather than being a
> subclass.
>

I'd think I'd like that approach.


> It might still be most efficient to land the current change first, then
> try to use just PrefService* everywhere we can, then we'll have fewer
> places to fix in the end (I think).  The two TestingPrefService subclasses
> will remain a bit ugly in the interim, but should in the end fold back down
> to one as you suggested we could do.
>

Sounds good to me.


>
>
> On Thu, Dec 20, 2012 at 2:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Hi Mattias,
>>
>> Thanks for the review.  I was in the process of working through your
>> specific comments, but it looks like we should first address your
>> PrefRegistry idea before we go further.
>>
>> I agree having a separate PrefRegistry object might be cleaner.  I wish
>> we had thought of it in our design discussion thread a week ago, or 6 days
>> ago in the preview version of this change, but we can't change the past :)
>>
>
I apologize that this idea comes up a bit late. The reason here is probably
that I only grok the entire scope of a change when I review the code. I
guess that's not surprising, but obviously causing significant pain here.
My conclusion would be that we should try to work in smaller steps, so I
can provide faster reviews and we avoid moving too far into a direction
that we abandon later.


>
>> I think we'll still end up with a Chrome-specific subclass of PrefService
>> that handles integration with sync, knows about incognito prefs, and a few
>> other things, i.e. what we call PrefServiceSyncable today.
>>
>
That's fine with me. The important point here is that the exposure of that
PrefServiceSyncable to the rest of Chrome would be very limited, most of
the code would just use plain PrefService* or PrefServiceBase*.


>
>> My suggestion for a way forward is this:
>> a) Land the current change after addressing your specific comments.
>> b) In a follow-up, switch BrowserProcess::local_state() to returning a
>> PrefService* (or PrefServiceBase*), and make sure all registration (to
>> local state or to profile prefs) is done through the
>> browser_prefs::RegisterXyz functions.
>>
>
And the same for Profile::GetPrefs() I assume?


>  c) At that point, we'll have code using either PrefService or
>> PrefServiceSyncable, and registrations will be done through only one code
>> path (not counting tests). Then it should be easier to switch from having
>> the RegisterXyzPreference methods in the two different subclasses to having
>> them in separate subclasses of PrefRegistry, at which point we would also
>> remove PrefServiceSimple and be down to PrefService vs. PrefServiceSyncable.
>> d) After that, we could see if it makes sense to also remove the
>> PrefServiceSyncable subclass.  I think this would mean changing the way
>> integration with sync is done, but it's quite possible that it will make
>> sense.
>>
>> WDYT?
>>
>
That plan sounds good to me.

>
>> Cheers,
>> Jói
>>
>>
>>
>> On Thu, Dec 20, 2012 at 1:41 PM, <mnissler@chromium.org> wrote:
>>
>>> Yay, I made it through \o/
>>>
>>> High-level comments (in no particular order):
>>>
>>> * This change is too big, which slows down reviews and thus iteration
>>> speed. I
>>> think this could have been done in steps: Add PrefService derivatives
>>> but keep
>>> PrefService interface intact -> convert consumers one by one -> remove
>>> unused
>>> interface from PrefService. I'm not saying you should change this CL,
>>> but I
>>> guess you'd appreciate faster reviews in the future :)
>>>
>>> * Having looked at the code, I'm no longer convinced the path we're on
>>> is the
>>> right one. We might be in the process of committing the standard
>>> OO-language-crime: Overusing subclassing because that's the main
>>> abstraction
>>> mechanism everybody is used to. Standard remedy: Use delegation instead!
>>>
>>> Dogmas aside, let's take some time to consider the delegation approach:
>>> We do
>>> remove the RegisterXYZ() calls from PrefService, but don't implement
>>> RegisterXYZ
>>> in subclasses, but make that the responsibility of a separate
>>> PrefRegistry (for
>>> lack of a better name) interface. We pass the PrefRegistry to
>>> PrefService on
>>> construction time, and PrefService uses it to  check what preferences are
>>> registered and obtain their types. Then, we can have different
>>> PrefRegistry
>>> implementations for local_state and user prefs, the former without the
>>> syncable
>>> flag, the latter including it.
>>>
>>> Benefits include:
>>> - Pref registration is no longer possible if you just have a PrefService
>>> pointer, but no access to the associated PrefRegistry. This property is
>>> nice
>>> because it enforces the rule that pref registrations are all supposed to
>>> be
>>> handled via browser_prefs::Register(Local|**User)Prefs().
>>> - The awkwardness in PrefServiceBuilder which has to initialize two
>>> different
>>> types of PrefServices goes away. There's now only a single type and we're
>>> passing the PrefRegistry pointer as an additional parameter.
>>> - Same goes for TestingPrefService, no subclassing needed here either.
>>>
>>> WDYT?
>>>
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/io_thread.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h>
>>> File chrome/browser/io_thread.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/io_thread.h#**newcode169<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h#newcode169>
>>> chrome/browser/io_thread.h:**169: IOThread(PrefServiceSimple*
>>> local_state,
>>> Can we pass PrefServiceBase* here?
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/policy/policy_**statistics_collector.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h>
>>> File chrome/browser/policy/policy_**statistics_collector.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/policy/policy_**statistics_collector.h#**newcode15<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/policy_statistics_collector.h#newcode15>
>>> chrome/browser/policy/policy_**statistics_collector.h:15: class
>>> PrefServiceSyncable;
>>> needed?
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/chrome_**pref_service_factory.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc>
>>> File chrome/browser/prefs/chrome_**pref_service_factory.cc (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/chrome_**pref_service_factory.cc#**newcode85<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode85>
>>> chrome/browser/prefs/chrome_**pref_service_factory.cc:85:
>>> PrefServiceSimple* pref_service = new PrefServiceSimple();
>>> I think I posted this somewhere already, but to reiterate: The builder
>>> should really call the constructor, I think we want separate
>>> PrefServiceBuilder subclasses for the separate PrefService derivatives.
>>> I'd rather not see half-initialized PrefService instances.
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/chrome_**pref_service_factory.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h>
>>> File chrome/browser/prefs/chrome_**pref_service_factory.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/chrome_**pref_service_factory.h#**newcode37<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chrome_pref_service_factory.h#newcode37>
>>> chrome/browser/prefs/chrome_**pref_service_factory.h:37: // The
>>> PrefService takes ownership of |extension_pref_store|.
>>> PrefStores are RefCounted, please remove this sentence.
>>>
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_simple.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h>
>>> File chrome/browser/prefs/pref_**service_simple.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_simple.h#newcode28<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_simple.h#newcode28>
>>> chrome/browser/prefs/pref_**service_simple.h:28: PrefServiceSimple();
>>> needs a virtual dtor
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h>
>>> File chrome/browser/prefs/pref_**service_syncable.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h#newcode96<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode96>
>>> chrome/browser/prefs/pref_**service_syncable.h:96: // TODO(zea): Have
>>> PrefService implement syncer::SyncableService directly.
>>> update comment
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h#newcode119<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode119>
>>> chrome/browser/prefs/pref_**service_syncable.h:119: // Constructs
>>> objects.
>>> s/objects/instances/
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h#newcode125<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode125>
>>> chrome/browser/prefs/pref_**service_syncable.h:125:
>>> documentation.
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h#newcode127<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode127>
>>> chrome/browser/prefs/pref_**service_syncable.h:127: const char* path,
>>> Value* default_value, PrefSyncStatus sync_status);
>>> param decls on separate lines.
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/prefs/pref_**service_syncable.h#newcode135<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref_service_syncable.h#newcode135>
>>> chrome/browser/prefs/pref_**service_syncable.h:135:
>>> ObserverList<**PrefServiceObserver> observer_list_;
>>> DISALLOW_COPY_AND_ASSIGN
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/profiles/**profile_impl.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h>
>>> File chrome/browser/profiles/**profile_impl.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/profiles/**profile_impl.h#newcode106<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/profile_impl.h#newcode106>
>>> chrome/browser/profiles/**profile_impl.h:106: virtual
>>> PrefServiceSyncable*
>>> GetOffTheRecordPrefs() OVERRIDE;
>>> Can we return PrefServiceBase* for these two?
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/sync/sync_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc>
>>> File chrome/browser/sync/sync_**prefs.cc (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/sync/sync_**prefs.cc#newcode303<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_prefs.cc#newcode303>
>>> chrome/browser/sync/sync_**prefs.cc:303: void
>>> SyncPrefs::**RegisterPreferences() {
>>> Random note: This should be called via browser_prefs::**
>>> RegisterUserPrefs.
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/browser_ui_**prefs.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc>
>>> File chrome/browser/ui/browser_ui_**prefs.cc (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/browser_ui_**prefs.cc#newcode158<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser_ui_prefs.cc#newcode158>
>>> chrome/browser/ui/browser_ui_**prefs.cc:158:
>>> window_pref.append(app_name);
>>> random note: This should register a single dictionary preference instead
>>> of construction pref names.
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/views/**browser_actions_container.cc<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc>
>>> File chrome/browser/ui/views/**browser_actions_container.cc (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/views/**browser_actions_container.cc#**newcode123<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/browser_actions_container.cc#newcode123>
>>> chrome/browser/ui/views/**browser_actions_container.cc:**123:
>>> RegisterUserPrefs(profile_->**GetPrefs());
>>> Random note: This should be called via
>>> browser_prefs::**RegisterUserPrefs();
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h>
>>> File chrome/browser/ui/webui/ntp/**new_tab_page_handler.h (right):
>>>
>>> https://codereview.chromium.**org/11570009/diff/21004/**
>>>
chrome/browser/ui/webui/ntp/**new_tab_page_handler.h#**newcode26<https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/ntp/new_tab_page_handler.h#newcode26>
>>> chrome/browser/ui/webui/ntp/**new_tab_page_handler.h:26: static void
>>> RegisterPrefs(**PrefServiceSimple* prefs);
>>> dead code?
>>>
>>>
https://codereview.chromium.**org/11570009/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[Jói, 2012-12-20 15:40:15]

Thanks Mattias.

Agreed we probably could have realized this new plan earlier if the
change had been more incremental.  No worries about this idea coming
up later than it could have, that's just the way things happen
sometimes, and I think you're right that we can have a better process
to decrease the cost of such changes.  I am also at fault for not
having brought up a third option I thought of in our design
discussion, which was to have the base PrefService class provide
accessors like GetSimpleRegistrar, GetSyncableRegistrar, which isn't
as good an idea as the completely separate PrefRegistrar interface,
but might have led to it.

In terms of process; because each incremental step adds overhead, I
was trying to avoid the need for doing many incremental steps by using
the preview change (which could not be landed, but which showed the
direction).  In retrospect, that didn't work out too well, and I think
a good model for this change might have been as follows, which I will
plan to use for similar changes in the future:
a) Land a change that split the classes, with virtual methods on the
base class to temporarily delegate to the subclasses, as you
suggested.
b) Do the first 5-10% of converting all callers, and get that
reviewed.  In this model, this would have been the last good point to
realize a change in direction.
c) Do the remaining 90-95% of converting all callers in one fell
swoop; this would then be a purely mechanical change that we would
already have done a "proof of concept" for in (b).

That approach seems like it would have kept the flexibility of
changing course more easily, but avoided too much overhead by not
doing a huge number of incremental changes.

> And the same for Profile::GetPrefs() I assume?

Agreed.  First minimize usage of the PrefServiceSimple and
PrefServiceSyncable classes across the code base, so most places only
know about PrefService, then switch to the PrefRegistry approach.

Cheers,
Jói




On Thu, Dec 20, 2012 at 3:21 PM, Mattias Nissler <mnissler@chromium.org> wrote:
>
> Comments inline.
>
> On Thu, Dec 20, 2012 at 3:58 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> BTW, I'm looking into this some more; perhaps there is a way to mutate the
current change more effectively into the PrefRegistry idea.  Perhaps the
ChromePrefService (née PrefServiceSyncable) can also be something that
aggregates a PrefService and handles the sync stuff, rather than being a
subclass.
>
>
> I'd think I'd like that approach.
>
>>
>> It might still be most efficient to land the current change first, then try
to use just PrefService* everywhere we can, then we'll have fewer places to fix
in the end (I think).  The two TestingPrefService subclasses will remain a bit
ugly in the interim, but should in the end fold back down to one as you
suggested we could do.
>
>
> Sounds good to me.
>
>>
>>
>>
>> On Thu, Dec 20, 2012 at 2:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>
>>> Hi Mattias,
>>>
>>> Thanks for the review.  I was in the process of working through your
specific comments, but it looks like we should first address your PrefRegistry
idea before we go further.
>>>
>>> I agree having a separate PrefRegistry object might be cleaner.  I wish we
had thought of it in our design discussion thread a week ago, or 6 days ago in
the preview version of this change, but we can't change the past :)
>
>
> I apologize that this idea comes up a bit late. The reason here is probably
that I only grok the entire scope of a change when I review the code. I guess
that's not surprising, but obviously causing significant pain here. My
conclusion would be that we should try to work in smaller steps, so I can
provide faster reviews and we avoid moving too far into a direction that we
abandon later.
>
>>>
>>>
>>> I think we'll still end up with a Chrome-specific subclass of PrefService
that handles integration with sync, knows about incognito prefs, and a few other
things, i.e. what we call PrefServiceSyncable today.
>
>
> That's fine with me. The important point here is that the exposure of that
PrefServiceSyncable to the rest of Chrome would be very limited, most of the
code would just use plain PrefService* or PrefServiceBase*.
>
>>>
>>>
>>> My suggestion for a way forward is this:
>>> a) Land the current change after addressing your specific comments.
>>> b) In a follow-up, switch BrowserProcess::local_state() to returning a
PrefService* (or PrefServiceBase*), and make sure all registration (to local
state or to profile prefs) is done through the browser_prefs::RegisterXyz
functions.
>
>
> And the same for Profile::GetPrefs() I assume?
>
>>>
>>> c) At that point, we'll have code using either PrefService or
PrefServiceSyncable, and registrations will be done through only one code path
(not counting tests). Then it should be easier to switch from having the
RegisterXyzPreference methods in the two different subclasses to having them in
separate subclasses of PrefRegistry, at which point we would also remove
PrefServiceSimple and be down to PrefService vs. PrefServiceSyncable.
>>> d) After that, we could see if it makes sense to also remove the
PrefServiceSyncable subclass.  I think this would mean changing the way
integration with sync is done, but it's quite possible that it will make sense.
>>>
>>> WDYT?
>
>
> That plan sounds good to me.
>>>
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>>
>>> On Thu, Dec 20, 2012 at 1:41 PM, <mnissler@chromium.org> wrote:
>>>>
>>>> Yay, I made it through \o/
>>>>
>>>> High-level comments (in no particular order):
>>>>
>>>> * This change is too big, which slows down reviews and thus iteration
speed. I
>>>> think this could have been done in steps: Add PrefService derivatives but
keep
>>>> PrefService interface intact -> convert consumers one by one -> remove
unused
>>>> interface from PrefService. I'm not saying you should change this CL, but
I
>>>> guess you'd appreciate faster reviews in the future :)
>>>>
>>>> * Having looked at the code, I'm no longer convinced the path we're on is
the
>>>> right one. We might be in the process of committing the standard
>>>> OO-language-crime: Overusing subclassing because that's the main
abstraction
>>>> mechanism everybody is used to. Standard remedy: Use delegation instead!
>>>>
>>>> Dogmas aside, let's take some time to consider the delegation approach: We
do
>>>> remove the RegisterXYZ() calls from PrefService, but don't implement
RegisterXYZ
>>>> in subclasses, but make that the responsibility of a separate PrefRegistry
(for
>>>> lack of a better name) interface. We pass the PrefRegistry to PrefService
on
>>>> construction time, and PrefService uses it to  check what preferences are
>>>> registered and obtain their types. Then, we can have different
PrefRegistry
>>>> implementations for local_state and user prefs, the former without the
syncable
>>>> flag, the latter including it.
>>>>
>>>> Benefits include:
>>>> - Pref registration is no longer possible if you just have a PrefService
>>>> pointer, but no access to the associated PrefRegistry. This property is
nice
>>>> because it enforces the rule that pref registrations are all supposed to
be
>>>> handled via browser_prefs::Register(Local|User)Prefs().
>>>> - The awkwardness in PrefServiceBuilder which has to initialize two
different
>>>> types of PrefServices goes away. There's now only a single type and we're
>>>> passing the PrefRegistry pointer as an additional parameter.
>>>> - Same goes for TestingPrefService, no subclassing needed here either.
>>>>
>>>> WDYT?
>>>>
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h
>>>> File chrome/browser/io_thread.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread....
>>>> chrome/browser/io_thread.h:169: IOThread(PrefServiceSimple* local_state,
>>>> Can we pass PrefServiceBase* here?
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
>>>> File chrome/browser/policy/policy_statistics_collector.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
>>>> chrome/browser/policy/policy_statistics_collector.h:15: class
>>>> PrefServiceSyncable;
>>>> needed?
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
>>>> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
>>>> chrome/browser/prefs/chrome_pref_service_factory.cc:85:
>>>> PrefServiceSimple* pref_service = new PrefServiceSimple();
>>>> I think I posted this somewhere already, but to reiterate: The builder
>>>> should really call the constructor, I think we want separate
>>>> PrefServiceBuilder subclasses for the separate PrefService derivatives.
>>>> I'd rather not see half-initialized PrefService instances.
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
>>>> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
>>>> chrome/browser/prefs/chrome_pref_service_factory.h:37: // The
>>>> PrefService takes ownership of |extension_pref_store|.
>>>> PrefStores are RefCounted, please remove this sentence.
>>>>
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> File chrome/browser/prefs/pref_service_simple.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_simple.h:28: PrefServiceSimple();
>>>> needs a virtual dtor
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> File chrome/browser/prefs/pref_service_syncable.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_syncable.h:96: // TODO(zea): Have
>>>> PrefService implement syncer::SyncableService directly.
>>>> update comment
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_syncable.h:119: // Constructs objects.
>>>> s/objects/instances/
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_syncable.h:125:
>>>> documentation.
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_syncable.h:127: const char* path,
>>>> Value* default_value, PrefSyncStatus sync_status);
>>>> param decls on separate lines.
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
>>>> chrome/browser/prefs/pref_service_syncable.h:135:
>>>> ObserverList<PrefServiceObserver> observer_list_;
>>>> DISALLOW_COPY_AND_ASSIGN
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
>>>> File chrome/browser/profiles/profile_impl.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
>>>> chrome/browser/profiles/profile_impl.h:106: virtual PrefServiceSyncable*
>>>> GetOffTheRecordPrefs() OVERRIDE;
>>>> Can we return PrefServiceBase* for these two?
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
>>>> File chrome/browser/sync/sync_prefs.cc (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
>>>> chrome/browser/sync/sync_prefs.cc:303: void
>>>> SyncPrefs::RegisterPreferences() {
>>>> Random note: This should be called via browser_prefs::RegisterUserPrefs.
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
>>>> File chrome/browser/ui/browser_ui_prefs.cc (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
>>>> chrome/browser/ui/browser_ui_prefs.cc:158: window_pref.append(app_name);
>>>> random note: This should register a single dictionary preference instead
>>>> of construction pref names.
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
>>>> File chrome/browser/ui/views/browser_actions_container.cc (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
>>>> chrome/browser/ui/views/browser_actions_container.cc:123:
>>>> RegisterUserPrefs(profile_->GetPrefs());
>>>> Random note: This should be called via
>>>> browser_prefs::RegisterUserPrefs();
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
>>>> File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):
>>>>
>>>>
https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
>>>> chrome/browser/ui/webui/ntp/new_tab_page_handler.h:26: static void
>>>> RegisterPrefs(PrefServiceSimple* prefs);
>>>> dead code?
>>>>
>>>> https://codereview.chromium.org/11570009/
>>>
>>>
>>
>
> --
> You received this message because you are subscribed to the Google Groups
"browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2012-12-20 16:30:30]

PTAL, I've addressed or responded to your specific review comments and uploaded
a new patch set.  You can look at the diff from patch set 4 to patch set 5 if
you wish to see just the changes made in response to comments.

Cheers,
Jói

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/autofill/D...
chrome/browser/autofill/DEPS:15: # Will go away once Prefs completes its move to
base/prefs/.
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> How so? You need the syncable bits, which will never be in base/prefs AFAIU.

Hmm, good point, some more work would be needed e.g. allowing registration of
Autofill preferences via a delegate interface or some such.  For now will just
remove the comment.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_utils.cc:22: #include
"chrome/browser/profiles/profile.h"
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> remove duplicate #include

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:403: PrefServiceSimple*
BrowserProcessImpl::local_state() {
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Is there any reason why we can't return PrefServiceBase* here? Whoever calls
> browser_process->local_state() should never register prefs ad hoc on the
> returned PrefService. Returning PrefServiceBase here would allow us to enforce
> that.

I think we could do that, have added a TODO to switch to that.

One question though; do you mean PrefServiceBase or just PrefService?  I think a
while back we were saying the PrefServiceBase interface would go away once we
finish all this refactoring work.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:754:
local_state_->UpdateCommandLinePrefStore(CommandLine::ForCurrentProcess());
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Ugh, UpdateCommandLinePrefStore shouldn't be in the public PrefService
> interface. Filed https://code.google.com/p/chromium/issues/detail?id=166877

Sounds good, thanks.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/component_...
File chrome/browser/component_updater/recovery_component_installer.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/component_...
chrome/browser/component_updater/recovery_component_installer.cc:66:
prefs->RegisterStringPref(prefs::kRecoveryComponentVersion, "0.0.0.0");
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> random note: This shouldn't be registered here but handled via
> browser_prefs::RegisterLocalState()

Agreed. Added a TODO to fix it.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:282:
PrefServiceSyncable* pref_service() const { return profile_->GetPrefs(); }
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Is this accessor really necessary? It seems the code in SetUp() could do
> without.

Added a TODO to investigate this.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:321:
profile_->SetPrefService(new TestingPrefServiceSyncable());
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> This shouldn't be necessary, TestingProfile already has a PrefService.

As above, added TODO to investigate.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:325:
ProtocolHandlerRegistry::RegisterUserPrefs(pref_service());
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> This should be removed if you remove line 321.

Ditto.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/debugger/d...
File chrome/browser/debugger/devtools_window.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/debugger/d...
chrome/browser/debugger/devtools_window.cc:382:
PrefServiceSyncable::UNSYNCABLE_PREF);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> random note: This should go through browser_prefs::RegisterUserPrefs

Added a TODO to fix.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:311:
pref_service_ = profile()->GetTestingPrefService();
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> used?

Yes, from SetDefaultDownloadPath, SetManagedDownloadPath and
SetPromptForDownload, around line 475.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs.cc:1642: PrefServiceSyncable* service =
profile()->GetPrefs();
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Can we use PrefServiceBase here?

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:185:
TestingPrefServiceSyncable* prefs = profile_.GetTestingPrefService();
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> could just use profile_->GetPrefs()->Set();

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/component_loader.h:29: PrefServiceSimple*
local_state);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Can we use PrefServiceBase* for these two?

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:96: PrefServiceSyncable* prefs,
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Can we pass PrefServiceBase*?

Not right now, because a couple of tests use it to register preferences... but
with a PrefRegistry type of idea we probably could... hmm :)

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:540:
local_state->RegisterBooleanPref(prefs::kShouldShowWelcomePage, false);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> random note: this should happen via browser_prefs::RegisterLocalState()

Added TODO.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:552: local_state->RegisterBooleanPref(
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> same here.

Added TODO.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/gpu/gl_str...
File chrome/browser/gpu/gl_string_manager.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/gpu/gl_str...
chrome/browser/gpu/gl_string_manager.cc:30:
local_state->RegisterStringPref(prefs::kGLVersionString, gl_version_);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Random note: Registrations should go through
browser_process::RegisterLocalState

Added TODO.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/io_thread....
chrome/browser/io_thread.h:169: IOThread(PrefServiceSimple* local_state,
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> Can we pass PrefServiceBase* here?

Unfortunately, not right now, because the constructor calls RegisterPrefs
(grrr).  Added a TODO in the .cc file to look into changing this.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
File chrome/browser/policy/policy_statistics_collector.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/policy/pol...
chrome/browser/policy/policy_statistics_collector.h:15: class
PrefServiceSyncable;
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> needed?

Removed.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:250:
ProtocolHandlerRegistry::RegisterUserPrefs(user_prefs);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> I don't see why you renamed this.

Because the convention is that RegisterPrefs takes a pointer to the local state
prefs, and RegisterUserPrefs takes a pointer to the profile prefs.  So renaming
it makes it conssistent with all the other functions here, and less confusing
when you read the code for the classes where I renamed.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:304:
NetworkProfileBubble::RegisterUserPrefs(user_prefs);
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> why rename?

As above.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.cc:85: PrefServiceSimple*
pref_service = new PrefServiceSimple();
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> I think I posted this somewhere already, but to reiterate: The builder should
> really call the constructor, I think we want separate PrefServiceBuilder
> subclasses for the separate PrefService derivatives. I'd rather not see
> half-initialized PrefService instances.

We addressed that by making the constructor private, and making
ChromePrefServiceFactory a friend.  We can switch back to using the constructor
if you still prefer that approach, but the current approach does avoid any
half-initialized PrefService objects.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:37: // The PrefService takes
ownership of |extension_pref_store|.
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> PrefStores are RefCounted, please remove this sentence.

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:11: class PrefServiceSimple : public
PrefService {
On 2012/12/19 19:50:22, Mattias Nissler wrote:
> Thinking more about these names: I'm wondering whether this would be easier to
> understand for people if we named this LocalStatePrefService and the one used
> for profile prefs ProfilePrefService, respectively. In general, I'm in favor
of
> naming a class based on what it does instead of what it's used for, but in
this
> case we might reduce confusing amongst developers significantly (what exactly
is
> Simple? Does Syncable imply everything gets synced?). WDYT?

I think Syncable implies "can be synced" not "is synced" so as far as a name for
a service that allows synced prefs I think it is fine.

Depending on what we do, this probably goes away anyway, at least
PrefServiceSimple probably goes away or at least is hidden and used only by the
browser_prefs code and the various RegisterPrefs functions, and everywhere else
you primarily use PrefService, sometimes PrefServiceSyncable depending on your
needs.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:28: PrefServiceSimple();
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> needs a virtual dtor

Done, also added DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:96: // TODO(zea): Have PrefService
implement syncer::SyncableService directly.
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> update comment

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:119: // Constructs objects.
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> s/objects/instances/

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:125: 
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> documentation.

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:127: const char* path, Value*
default_value, PrefSyncStatus sync_status);
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> param decls on separate lines.

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:135:
ObserverList<PrefServiceObserver> observer_list_;
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:106: virtual PrefServiceSyncable*
GetOffTheRecordPrefs() OVERRIDE;
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> Can we return PrefServiceBase* for these two?

Not right now, this seems to be primarily because of the various
incorrectly-done RegisterUserPreferences bits we've seen around the code base. 
As discussed, will limit usage of PrefService subclasses in a follow-up, and
this will be one of the places that should only return a PrefService*.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:303: void SyncPrefs::RegisterPreferences() {
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> Random note: This should be called via browser_prefs::RegisterUserPrefs.

Added a TODO(joi).

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:158: window_pref.append(app_name);
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> random note: This should register a single dictionary preference instead of
> construction pref names.

This seems like a case where we should register a separate bug? Do you want to
do the honors or should I?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:123:
RegisterUserPrefs(profile_->GetPrefs());
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> Random note: This should be called via browser_prefs::RegisterUserPrefs();

Added TODO.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:26: static void
RegisterPrefs(PrefServiceSimple* prefs);
On 2012/12/20 13:41:52, Mattias Nissler wrote:
> dead code?

I think so, removed.

[Mattias Nissler (ping if slow), 2012-12-21 13:09:30]

Only remaining issue is the factory vs. builder thing, happy to discuss this via
gtalk/VC if that works better for you.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:403: PrefServiceSimple*
BrowserProcessImpl::local_state() {
On 2012/12/20 16:30:31, Jói wrote:
> On 2012/12/19 19:50:22, Mattias Nissler wrote:
> > Is there any reason why we can't return PrefServiceBase* here? Whoever calls
> > browser_process->local_state() should never register prefs ad hoc on the
> > returned PrefService. Returning PrefServiceBase here would allow us to
enforce
> > that.
> 
> I think we could do that, have added a TODO to switch to that.
> 
> One question though; do you mean PrefServiceBase or just PrefService?  I think
a
> while back we were saying the PrefServiceBase interface would go away once we
> finish all this refactoring work.

I guess I got confused by naming and which is living where. I meant: The
PrefService type that will eventually live in base/prefs and provides all the
standard PrefService APIs for reading and writing values (but not pref
registration) :)

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:250:
ProtocolHandlerRegistry::RegisterUserPrefs(user_prefs);
On 2012/12/20 16:30:31, Jói wrote:
> On 2012/12/19 19:50:22, Mattias Nissler wrote:
> > I don't see why you renamed this.
> 
> Because the convention is that RegisterPrefs takes a pointer to the local
state
> prefs, and RegisterUserPrefs takes a pointer to the profile prefs.  So
renaming
> it makes it conssistent with all the other functions here, and less confusing
> when you read the code for the classes where I renamed.

I see, makes sense. Then please also change lines 230, 243, 257 and 290.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.cc:85: PrefServiceSimple*
pref_service = new PrefServiceSimple();
On 2012/12/20 16:30:31, Jói wrote:
> On 2012/12/20 13:41:52, Mattias Nissler wrote:
> > I think I posted this somewhere already, but to reiterate: The builder
should
> > really call the constructor, I think we want separate PrefServiceBuilder
> > subclasses for the separate PrefService derivatives. I'd rather not see
> > half-initialized PrefService instances.
> 
> We addressed that by making the constructor private, and making
> ChromePrefServiceFactory a friend.  We can switch back to using the
constructor
> if you still prefer that approach, but the current approach does avoid any
> half-initialized PrefService objects.

I would really prefer the ctor approach, which would be a clean implementation
of the builder pattern that people are much more likely to understand than the
convoluted protected-ctor and friended-factory using a builder internally
approach.

Bonus: With a clean builder pattern, it'll be easy to move standalone-usable
core implementations of PrefService and its associated PrefServiceBuilder into
base/prefs, while chrome/browser/prefs can provide an extended builder that can
also construct syncable PrefService instances (possibly instantiating a
different PrefService subclass). The approach you have requires a friended
factory for every PrefService subclass, which mostly annihilates the usefulness
of PrefServiceBuilder since you cannot use it directly to construct special
PrefService instances any longer.

Finally, here's a consistency argument: PrefServiceMockBuilder implements
exactly what I proposed above, why should we do it differently (through the
factory) for production PrefService instances? How about we rename
PrefServiceMockBuilder to ChromePrefServiceBuilder (the CreateSimple() and
CreateSyncable() functions are already there!) and get rid of
ChromePrefServiceFactory entirely?

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:11: class PrefServiceSimple : public
PrefService {
On 2012/12/20 16:30:31, Jói wrote:
> On 2012/12/19 19:50:22, Mattias Nissler wrote:
> > Thinking more about these names: I'm wondering whether this would be easier
to
> > understand for people if we named this LocalStatePrefService and the one
used
> > for profile prefs ProfilePrefService, respectively. In general, I'm in favor
> of
> > naming a class based on what it does instead of what it's used for, but in
> this
> > case we might reduce confusing amongst developers significantly (what
exactly
> is
> > Simple? Does Syncable imply everything gets synced?). WDYT?
> 
> I think Syncable implies "can be synced" not "is synced" so as far as a name
for
> a service that allows synced prefs I think it is fine.
> 
> Depending on what we do, this probably goes away anyway, at least
> PrefServiceSimple probably goes away or at least is hidden and used only by
the
> browser_prefs code and the various RegisterPrefs functions, and everywhere
else
> you primarily use PrefService, sometimes PrefServiceSyncable depending on your
> needs.

Yes, makes sense. Material for follow-up changes then.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:158: window_pref.append(app_name);
On 2012/12/20 16:30:31, Jói wrote:
> On 2012/12/20 13:41:52, Mattias Nissler wrote:
> > random note: This should register a single dictionary preference instead of
> > construction pref names.
> 
> This seems like a case where we should register a separate bug? Do you want to
> do the honors or should I?

https://code.google.com/p/chromium/issues/detail?id=167256
Can you do me the favor and drop a TODO while you're at it?

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/extensions...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/extensions...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:186:
profile_.GetPrefs()->Set(prefs::kGoogleServicesUsername, value);
Also, you could just use SetString() :)

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:28: PrefServiceSimple();
still needs a virtual dtor (I thought there was agreement on chromium-dev to put
it for better-safe-than-sorry reasons even in subclasses?)

[Jói, 2012-12-21 14:34:13]

PTAL.

I've switched to a pure builder model, which involved adding another class to
the PrefServiceBuilder hierarchy; we now hvae PrefServiceBuilder (which will
move to base/prefs/), PrefServiceSyncableBuilder, which has the Chrome-specific
concepts (previously split between PrefServiceMockBuilder and
ChromePrefServiceFactory) and PrefServiceMockBuilder (a tiny specialization of
PrefServiceSyncableBuilder, intended for tests.

The factory is still a factory, not a builder, but I made it into just two
static functions that internally use a builder to construct the instance.

Happy to discuss, but to me this looks like the cleanest way to what I think you
were proposing.  Because the factory is doing Chrome-specific business logic and
we currently expect a set of parameters that we need to be careful to provide, a
factory function seems more appropriate than requiring client code to call
multiple WithXyz methods on a builder (whereas that pattern is quite useful for
tests).

Also addressed the other issues.

Cheers,
Jói

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:250:
ProtocolHandlerRegistry::RegisterUserPrefs(user_prefs);
On 2012/12/21 13:09:31, Mattias Nissler wrote:
> On 2012/12/20 16:30:31, Jói wrote:
> > On 2012/12/19 19:50:22, Mattias Nissler wrote:
> > > I don't see why you renamed this.
> > 
> > Because the convention is that RegisterPrefs takes a pointer to the local
> state
> > prefs, and RegisterUserPrefs takes a pointer to the profile prefs.  So
> renaming
> > it makes it conssistent with all the other functions here, and less
confusing
> > when you read the code for the classes where I renamed.
> 
> I see, makes sense. Then please also change lines 230, 243, 257 and 290.

Done. chrome::RegisterAutolaunchPrefs -> chrome::RegisterAutoLaunchPrefs, the
rest just renamed RegisterPrefs to RegisterUserPrefs.

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/21004/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:158: window_pref.append(app_name);
On 2012/12/21 13:09:31, Mattias Nissler wrote:
> On 2012/12/20 16:30:31, Jói wrote:
> > On 2012/12/20 13:41:52, Mattias Nissler wrote:
> > > random note: This should register a single dictionary preference instead
of
> > > construction pref names.
> > 
> > This seems like a case where we should register a separate bug? Do you want
to
> > do the honors or should I?
> 
> https://code.google.com/p/chromium/issues/detail?id=167256
> Can you do me the favor and drop a TODO while you're at it?

Done.

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/extensions...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/extensions...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:186:
profile_.GetPrefs()->Set(prefs::kGoogleServicesUsername, value);
On 2012/12/21 13:09:31, Mattias Nissler wrote:
> Also, you could just use SetString() :)

Done.

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.h (right):

https://codereview.chromium.org/11570009/diff/33045/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.h:28: PrefServiceSimple();
On 2012/12/21 13:09:31, Mattias Nissler wrote:
> still needs a virtual dtor (I thought there was agreement on chromium-dev to
put
> it for better-safe-than-sorry reasons even in subclasses?)

Done.

[Mattias Nissler (ping if slow), 2012-12-21 15:03:20]

I think the Builder thing is much cleaner now, thanks for doing that. LGTM with
nits.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:19: class
PrefServiceSyncableBuilder;
required?

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_mock_builder.cc:17: #include
"content/public/browser/browser_thread.h"
Please clean up the #includes that are no longer needed.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_mock_builder.cc:27: user_prefs_ = new
TestingPrefStore;
I think you should do this in a ResetDefaultState() override, so the builder
does behave correctly when WithUserPrefs() is called and on repeated CreateXYZ()
invocations.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.cc (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.cc:9: #include
"base/value_conversions.h"
Please clean up #includes. base/values.h might be missing.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:158: // dictionary instead. Also tracked
as http://crbug.com/167256
Thanks!

https://codereview.chromium.org/11570009/diff/39/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_syncable_builder.h (right):

https://codereview.chromium.org/11570009/diff/39/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_syncable_builder.h:11: class FilePath;
nit: unused

[Jói, 2012-12-21 15:29:52]

Thanks, uploaded a patch to address the last nits.  Working on landing this
beast now :)

Thanks for working with Kai and myself on this huge patch.  I'll be glad to see
it land so we can iterate on smaller follow-up changes after.

Cheers,
Jói

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:19: class
PrefServiceSyncableBuilder;
On 2012/12/21 15:03:20, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_mock_builder.cc:17: #include
"content/public/browser/browser_thread.h"
On 2012/12/21 15:03:20, Mattias Nissler wrote:
> Please clean up the #includes that are no longer needed.

Done.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_mock_builder.cc:27: user_prefs_ = new
TestingPrefStore;
On 2012/12/21 15:03:20, Mattias Nissler wrote:
> I think you should do this in a ResetDefaultState() override, so the builder
> does behave correctly when WithUserPrefs() is called and on repeated
CreateXYZ()
> invocations.

Good idea, switched to that approach.

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_simple.cc (right):

https://codereview.chromium.org/11570009/diff/48011/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_simple.cc:9: #include
"base/value_conversions.h"
On 2012/12/21 15:03:20, Mattias Nissler wrote:
> Please clean up #includes. base/values.h might be missing.

Done.

https://codereview.chromium.org/11570009/diff/39/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_syncable_builder.h (right):

https://codereview.chromium.org/11570009/diff/39/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_syncable_builder.h:11: class FilePath;
On 2012/12/21 15:03:21, Mattias Nissler wrote:
> nit: unused

Done.

[commit-bot: I haz the power, 2012-12-21 20:17:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11570009/50010

[commit-bot: I haz the power, 2012-12-21 20:19:17]

Failed to apply patch for chrome/browser/ui/browser_ui_prefs.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/browser_ui_prefs.cc
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 42 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/browser_ui_prefs.cc.rej

Patch:       chrome/browser/ui/browser_ui_prefs.cc
Index: chrome/browser/ui/browser_ui_prefs.cc
diff --git a/chrome/browser/ui/browser_ui_prefs.cc
b/chrome/browser/ui/browser_ui_prefs.cc
index
a401444ea042e30526d1f6454c7b7fc9d4ad3288..fd2e5ecc50d11f44febdf593f344684e12a1c3cd
100644
--- a/chrome/browser/ui/browser_ui_prefs.cc
+++ b/chrome/browser/ui/browser_ui_prefs.cc
@@ -10,25 +10,25 @@
 
 namespace chrome {
 
-void RegisterBrowserPrefs(PrefService* prefs) {
+void RegisterBrowserPrefs(PrefServiceSimple* prefs) {
   prefs->RegisterIntegerPref(prefs::kOptionsWindowLastTabIndex, 0);
   prefs->RegisterBooleanPref(prefs::kAllowFileSelectionDialogs, true);
   prefs->RegisterBooleanPref(prefs::kShouldShowFirstRunBubble, false);
 }
 
-void RegisterBrowserUserPrefs(PrefService* prefs) {
+void RegisterBrowserUserPrefs(PrefServiceSyncable* prefs) {
   prefs->RegisterBooleanPref(prefs::kHomePageChanged,
                              false,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kHomePageIsNewTabPage,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kShowHomeButton,
                              false,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterIntegerPref(prefs::kExtensionsSideloadWipeoutBubbleShown,
                              0,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
 #if defined(OS_MACOSX)
   // This really belongs in platform code, but there's no good place to
   // initialize it between the time when the AppController is created
@@ -40,126 +40,132 @@ void RegisterBrowserUserPrefs(PrefService* prefs) {
   // that use this preference.
   prefs->RegisterBooleanPref(prefs::kShowUpdatePromotionInfoBar,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
 #endif
   prefs->RegisterBooleanPref(prefs::kDeleteBrowsingHistory,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeleteDownloadHistory,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeleteCache,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeleteCookies,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeletePasswords,
                              false,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeleteFormData,
                              false,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDeleteHostedAppsData,
                              false,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterIntegerPref(prefs::kDeleteTimePeriod,
                              0,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kCheckDefaultBrowser,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
 #if defined(OS_WIN)
   // As with Mac-spacific code above, it should be in a platform-specific
   // section somewhere, but there is no good place for it.
   prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault,
                              false,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
 #endif
   prefs->RegisterBooleanPref(prefs::kShowOmniboxSearchHint,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kWebAppCreateOnDesktop,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kWebAppCreateInAppsMenu,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kWebAppCreateInQuickLaunchBar,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kEnableTranslate,
                              true,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterStringPref(prefs::kCloudPrintEmail,
                             std::string(),
-                            PrefService::UNSYNCABLE_PREF);
+                            PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kCloudPrintProxyEnabled,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kCloudPrintSubmitEnabled,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kDevToolsDisabled,
                              false,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterIntegerPref(prefs::kDevToolsHSplitLocation,
                              -1,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterIntegerPref(prefs::kDevToolsVSplitLocation,
                              -1,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kBrowserWindowPlacement,
-                                PrefService::UNSYNCABLE_PREF);
+                                PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kPreferencesWindowPlacement,
-                                PrefService::UNSYNCABLE_PREF);
+                                PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kImportBookmarks,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kImportHistory,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kImportHomepage,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kImportSearchEngine,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kImportSavedPasswords,
                              true,
-                             PrefService::UNSYNCABLE_PREF);
+                             PrefServiceSyncable::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(prefs::kEnableDoNotTrack,
                              false,
-                             PrefService::SYNCABLE_PREF);
+                             PrefServiceSyncable::SYNCABLE_PREF);
 
   // Dictionaries to keep track of default tasks in the file browser.
   prefs->RegisterDictionaryPref(prefs::kDefaultTasksByMimeType,
-                                PrefService::SYNCABLE_PREF);
+                                PrefServiceSyncable::SYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kDefaultTasksBySuffix,
-                                PrefService::SYNCABLE_PREF);
+                                PrefServiceSyncable::SYNCABLE_PREF);
 
   // We need to register the type of these preferences in order to query
   // them even though they're only typically controlled via policy.
   prefs->RegisterBooleanPref(prefs::kPluginsAllowOutdated,
                              false,
-                             PrefService::UNSYNCABLE_…
(message too large)

[Jói, 2012-12-21 20:19:28]

TBR=ben@chromium.org for millions of usage updates of the new PrefService
hierarchy. Already reviewed by mnissler@ for Prefs OWNERS.

Cheers,
Jói

[commit-bot: I haz the power, 2012-12-21 22:42:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11570009/42

[commit-bot: I haz the power, 2012-12-21 22:44:05]

Failed to apply patch for .gitmodules:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file .gitmodules
  Hunk #1 FAILED at 22.
  Hunk #2 FAILED at 166.
  Hunk #3 FAILED at 202.
  Hunk #4 FAILED at 226.
  Hunk #5 FAILED at 250.
  Hunk #6 FAILED at 262.
  Hunk #7 FAILED at 322.
  Hunk #8 FAILED at 464.
  8 out of 8 hunks FAILED -- saving rejects to file .gitmodules.rej

Patch:       .gitmodules
Index: .gitmodules
diff --git a/.gitmodules b/.gitmodules
index
ab8f2b7b675854ebb605d8fb5970eefa9a501e2e..96fb631e8666b19c52360ad3401f9b6a7e75971e
100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -22,10 +22,6 @@
 	path = chrome/test/data/perf/frame_rate/content
 	url = https://chromium.googlesource.com/chromium/frame_rate/content.git
 	os = all
-[submodule "chrome/test/data/perf/third_party/octane"]
-	path = chrome/test/data/perf/third_party/octane
-	url = https://chromium.googlesource.com/external/octane-benchmark.git
-	os = all
 [submodule "chrome/tools/test/reference_build/chrome_linux"]
 	path = chrome/tools/test/reference_build/chrome_linux
 	url =
https://chromium.googlesource.com/chromium/reference_builds/chrome_linux.git
@@ -166,10 +162,6 @@
 	path = third_party/aosp
 	url = https://chromium.googlesource.com/chromium/deps/aosp.git
 	os = android
-[submodule "third_party/apache-mime4j"]
-	path = third_party/apache-mime4j
-	url = https://chromium.googlesource.com/chromium/deps/apache-mime4j.git
-	os = android
 [submodule "third_party/bidichecker"]
 	path = third_party/bidichecker
 	url = https://chromium.googlesource.com/external/bidichecker/lib.git
@@ -202,10 +194,6 @@
 	path = third_party/ffmpeg
 	url = https://chromium.googlesource.com/chromium/third_party/ffmpeg.git
 	os = all
-[submodule "third_party/findbugs"]
-	path = third_party/findbugs
-	url = https://chromium.googlesource.com/chromium/deps/findbugs.git
-	os = android
 [submodule "third_party/flac"]
 	path = third_party/flac
 	url = https://chromium.googlesource.com/chromium/deps/flac.git
@@ -226,18 +214,6 @@
 	path = third_party/gperf
 	url = https://chromium.googlesource.com/chromium/deps/gperf.git
 	os = win
-[submodule "third_party/guava/src"]
-	path = third_party/guava/src
-	url = https://chromium.googlesource.com/external/guava-libraries.git
-	os = android
-[submodule "third_party/httpcomponents-client"]
-	path = third_party/httpcomponents-client
-	url =
https://chromium.googlesource.com/chromium/deps/httpcomponents-client.git
-	os = android
-[submodule "third_party/httpcomponents-core"]
-	path = third_party/httpcomponents-core
-	url = https://chromium.googlesource.com/chromium/deps/httpcomponents-core.git
-	os = android
 [submodule "third_party/hunspell"]
 	path = third_party/hunspell
 	url = https://chromium.googlesource.com/chromium/deps/hunspell.git
@@ -250,10 +226,6 @@
 	path = third_party/icu
 	url = https://chromium.googlesource.com/chromium/deps/icu46.git
 	os = all
-[submodule "third_party/jarjar"]
-	path = third_party/jarjar
-	url = https://chromium.googlesource.com/chromium/deps/jarjar.git
-	os = android
 [submodule "third_party/jsoncpp/source/include"]
 	path = third_party/jsoncpp/source/include
 	url = https://chromium.googlesource.com/external/jsoncpp/jsoncpp/include.git
@@ -262,10 +234,6 @@
 	path = third_party/jsoncpp/source/src/lib_json
 	url =
https://chromium.googlesource.com/external/jsoncpp/jsoncpp/src/lib_json.git
 	os = all
-[submodule "third_party/jsr-305/src"]
-	path = third_party/jsr-305/src
-	url = https://chromium.googlesource.com/external/jsr-305.git
-	os = android
 [submodule "third_party/leveldatabase/src"]
 	path = third_party/leveldatabase/src
 	url = https://chromium.googlesource.com/external/leveldb.git
@@ -322,6 +290,14 @@
 	path = third_party/mingw-w64/mingw/bin
 	url =
https://chromium.googlesource.com/native_client/deps/third_party/mingw-w64/mi...
 	os = win
+[submodule "third_party/mozc/chrome/chromeos/renderer"]
+	path = third_party/mozc/chrome/chromeos/renderer
+	url =
https://chromium.googlesource.com/external/mozc/src/chrome/chromeos/renderer.git
+	os = all
+[submodule "third_party/mozc/session"]
+	path = third_party/mozc/session
+	url = https://chromium.googlesource.com/external/mozc/src/session.git
+	os = all
 [submodule "third_party/mtpd/source"]
 	path = third_party/mtpd/source
 	url = https://chromium.googlesource.com/chromiumos/platform/mtpd.git
@@ -464,7 +440,7 @@
 	os = all
 [submodule "third_party/webrtc"]
 	path = third_party/webrtc
-	url = https://chromium.googlesource.com/external/webrtc/stable/webrtc.git
+	url = https://chromium.googlesource.com/external/webrtc/stable/src.git
 	os = all
 [submodule "third_party/xdg-utils"]
 	path = third_party/xdg-utils

[commit-bot: I haz the power, 2012-12-22 00:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11570009/57767

[commit-bot: I haz the power, 2012-12-22 04:34:31]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-22 08:37:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11570009/49823

[commit-bot: I haz the power, 2012-12-22 17:24:50]

Change committed as 174531