Switch Preferences to use Factory

Switch Preferences to use Factory

Update the Preferences mojom to include a PreferencesFactory. This factory will
be responsible for creating a PreferencesManager, and enforcing the binding of
the PreferencesObserver at creation time. Vs the current method which does not
guarantee this.

Chrome will no longer expose PreferencesManager as a service directly. Ash has
been updated to connect to the factory. PrefObserverStore has been updated to
receive the factory and to bind to the manager this way.

TEST=manual testing, preferences_unittests
BUG=674140
Review-Url: https://codereview.chromium.org/2635093002
Cr-Commit-Position: refs/heads/master@{#444832}
Committed: https://chromium.googlesource.com/chromium/src/+/016032b4d15cd6583a5249c10d59c4de854a2b03

[jonross, 2017-01-16 21:03:15]

jonross@chromium.org changed reviewers:
+ dcheng@chromium.org

[jonross, 2017-01-16 21:03:15]

Hey dcheng@:

You had suggested the use of a Factory pattern for the preferences mojom in a
previous review. As a way of ensuring the two-way binding of the api at creation
time.

This review introduces that and updates the usage to it. Eliminating the old
pattern.

Could you please take a look?

Thanks,
Jon

[dcheng, 2017-01-17 22:36:33]

LGTM.

I am curious about how you feel about the overall before/after of the code here:
if you'd be willing to chime in on
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/j-kiK17V-Qc,
I think that'd be valuable feedback.

https://codereview.chromium.org/2635093002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_manager.cc:21: DCHECK(profile);
Maybe DCHECK(client.is_bound()) here?

[jonross, 2017-01-17 23:05:43]

I'll add comments on the thread.

https://codereview.chromium.org/2635093002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_manager.cc:21: DCHECK(profile);
On 2017/01/17 22:36:33, dcheng wrote:
> Maybe DCHECK(client.is_bound()) here?

Done.

[jonross, 2017-01-17 23:09:03]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-01-17 23:09:03]

sky@chromium.org: Please review changes in 

chrome/browser

This change switches the setup of PreferencesManager to use a factory pattern.

Thanks,
Jon

[sky, 2017-01-17 23:45:04]

LGTM

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_connection_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_connection_manager.cc:50: for (auto it =
std::begin(manager_bindings_);
Why do you need std::begin/std::end here?

[jonross, 2017-01-18 23:13:02]

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_connection_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_connection_manager.cc:50: for (auto it =
std::begin(manager_bindings_);
On 2017/01/17 23:45:04, sky wrote:
> Why do you need std::begin/std::end here?

I need the iterator for std::vector.erase, and ranged-based for loops do not
resolve to that. :(

Nor do we support bound params for lambdas yet, so I can't just use
std::remove_if :(

[jonross, 2017-01-18 23:13:43]

jonross@chromium.org changed reviewers:
+ ben@chromium.org

[jonross, 2017-01-18 23:13:43]

ben@chromium.org: Please review changes in 

services/preferences/

This change switches the preferences mojom to use a factory interface, so that
two-way binding is enforced at creation time.

Thanks,
Jon

[sky, 2017-01-18 23:38:25]

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_connection_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_connection_manager.cc:50: for (auto it =
std::begin(manager_bindings_);
On 2017/01/18 23:13:02, jonross wrote:
> On 2017/01/17 23:45:04, sky wrote:
> > Why do you need std::begin/std::end here?
> 
> I need the iterator for std::vector.erase, and ranged-based for loops do not
> resolve to that. :(

Sorry for not being clear. I'm asking why you can't do:

for (auto it = manager_bindings_.begin(); it != manager_bindings_.end(); ++it) {
...
}

Maybe I'm missing something?

> 
> Nor do we support bound params for lambdas yet, so I can't just use
> std::remove_if :(

[jonross, 2017-01-19 15:24:13]

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences_connection_manager.cc (right):

https://codereview.chromium.org/2635093002/diff/80001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences_connection_manager.cc:50: for (auto it =
std::begin(manager_bindings_);
On 2017/01/18 23:38:25, sky wrote:
> On 2017/01/18 23:13:02, jonross wrote:
> > On 2017/01/17 23:45:04, sky wrote:
> > > Why do you need std::begin/std::end here?
> > 
> > I need the iterator for std::vector.erase, and ranged-based for loops do not
> > resolve to that. :(
> 
> Sorry for not being clear. I'm asking why you can't do:
> 
> for (auto it = manager_bindings_.begin(); it != manager_bindings_.end(); ++it)
{
> ...
> }
> 
> Maybe I'm missing something?
> 
> > 
> > Nor do we support bound params for lambdas yet, so I can't just use
> > std::remove_if :(
> 

You aren't missing anything, and now I'm wondering why I wrote it that way to
being with... fixed.

[sky, 2017-01-19 17:00:03]

LGTM

[jonross, 2017-01-19 18:38:42]

On 2017/01/19 17:00:03, sky wrote:
> LGTM

Hey Ben,

Could you provide an owners review for services/preferences/*?

Thanks,
Jon

[Ben Goodger (Google), 2017-01-19 18:56:54]

lgtm

https://codereview.chromium.org/2635093002/diff/120001/services/preferences/p...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2635093002/diff/120001/services/preferences/p...
services/preferences/public/interfaces/preferences.mojom:15:
Create(PreferencesObserver observer, PreferencesManager& manager);
fwiw, now that you've enforced a more strict 1:1 mapping, I'd try and find a
name that used the term "Client" instead of observer. Also the prefix should
probably be the same. So, e.g.

PreferencesService
PreferencesServiceClient

This is just a suggestion & can be pursued in a subsequent CL.

[jonross, 2017-01-19 19:07:05]

https://codereview.chromium.org/2635093002/diff/120001/services/preferences/p...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2635093002/diff/120001/services/preferences/p...
services/preferences/public/interfaces/preferences.mojom:15:
Create(PreferencesObserver observer, PreferencesManager& manager);
On 2017/01/19 18:56:54, Ben Goodger (Google) wrote:
> fwiw, now that you've enforced a more strict 1:1 mapping, I'd try and find a
> name that used the term "Client" instead of observer. Also the prefix should
> probably be the same. So, e.g.
> 
> PreferencesService
> PreferencesServiceClient
> 
> This is just a suggestion & can be pursued in a subsequent CL.

I was planning for a rename in a follow up.

I'll make the switch:
  PreferencesManager  -> PreferencesService
  PreferencesObserver -> PreferencesServiceClient
  PreferencesFactory  -> PreferencesServiceFactory

[jonross, 2017-01-19 19:07:10]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-01-19 19:07:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2635093002/#ps120001
(title: "Missed test")

[commit-bot: I haz the power, 2017-01-19 19:07:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 20:20:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484852830524490,
"parent_rev": "dde7820b828009f0ad4741a1917c9757036c3801", "commit_rev":
"016032b4d15cd6583a5249c10d59c4de854a2b03"}

[commit-bot: I haz the power, 2017-01-19 20:20:53]

Description was changed from

==========
Switch Preferences to use Factory

Update the Preferences mojom to include a PreferencesFactory. This factory will
be responsible for creating a PreferencesManager, and enforcing the binding of
the PreferencesObserver at creation time. Vs the current method which does not
guarantee this.

Chrome will no longer expose PreferencesManager as a service directly. Ash has
been updated to connect to the factory. PrefObserverStore has been updated to
receive the factory and to bind to the manager this way.

TEST=manual testing, preferences_unittests
BUG=674140
==========

to

==========
Switch Preferences to use Factory

Update the Preferences mojom to include a PreferencesFactory. This factory will
be responsible for creating a PreferencesManager, and enforcing the binding of
the PreferencesObserver at creation time. Vs the current method which does not
guarantee this.

Chrome will no longer expose PreferencesManager as a service directly. Ash has
been updated to connect to the factory. PrefObserverStore has been updated to
receive the factory and to bind to the manager this way.

TEST=manual testing, preferences_unittests
BUG=674140

Review-Url: https://codereview.chromium.org/2635093002
Cr-Commit-Position: refs/heads/master@{#444832}
Committed:
https://chromium.googlesource.com/chromium/src/+/016032b4d15cd6583a5249c10d59...
==========

[commit-bot: I haz the power, 2017-01-19 20:20:54]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/016032b4d15cd6583a5249c10d59...