Clean up PrefServiceBuilder

Clean up PrefServiceBuilder

* Use simple setters rather than With* methods.
* Explicitly transfer memory ownership.
* Rename "Builder" to "Factory"
* Eliminate the side-effect of resetting the builder from the Create* methods.

Along the way, fix a memory leak in the Android WebView.

BUG=315499
TEST=none (code should continue to compile and tests should continue to pass)
R=joi@chromium.org

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

[Jói, 2013-11-08 13:48:37]

I'm not sure what the motivation was to remove the .WithXyz pattern (which is
just a way to make an otherwise huge constructor with lots of optional items
more readable by turning it into an object of .With methods), but I personally
don't mind that change and the other stuff is all good.

I'm no longer an OWNER for base/prefs (I was only an OWNER temporarily while
extracting Prefs from //chrome) so you'll need to get one of those fine folks to
take a look.

Cheers,
Jói

https://codereview.chromium.org/64193003/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/64193003/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service_unittest.cc:483:
base::MessageLoopProxy::current().get());
indent one more space

[Ilya Sherman, 2013-11-11 21:27:55]

On 2013/11/08 13:48:37, Jói wrote:
> I'm not sure what the motivation was to remove the .WithXyz pattern (which is
> just a way to make an otherwise huge constructor with lots of optional items
> more readable by turning it into an object of .With methods), but I personally
> don't mind that change and the other stuff is all good.

The motivation is mostly that these methods have somewhat unusual semantics
(since they return |this| by reference); and in practice, very few clients were
actually using this return value as a way to abbreviate object construction.

https://codereview.chromium.org/64193003/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/64193003/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service_unittest.cc:483:
base::MessageLoopProxy::current().get());
On 2013/11/08 13:48:38, Jói wrote:
> indent one more space

Done.

[Ilya Sherman, 2013-11-11 21:29:58]

Mattias, could you take a look at the base/prefs/ changes?  Thanks.

[Ilya Sherman, 2013-11-11 21:30:21]

On 2013/11/11 21:29:58, Ilya Sherman wrote:
> Mattias, could you take a look at the base/prefs/ changes?  Thanks.

And the chrome/browser/prefs/ changes as well, actually :)

[Ilya Sherman, 2013-11-14 01:05:15]

Dominic, could you take a look at base/prefs and chrome/browser/prefs?  Thanks.

[battre, 2013-11-14 01:58:20]

LGTM - thanks for the cleanup work

[Ilya Sherman, 2013-11-14 02:02:14]

tommi@chromium.org: Please review changes in //chrome_frame.

joth@chromium.org: Please review changes in //android_webview.

thakis@chromium.org: Please review changes in //chrome/browser.

[joth__google, 2013-11-14 02:04:23]

aw/ lgtm

[Ilya Sherman, 2013-11-14 02:05:29]

On 2013/11/14 02:04:23, joth__google wrote:
> aw/ lgtm

Hmm, I think the CQ is going to complain that only your @chromium.org address is
listed as an owner for aw/.  Mind re-stamping with that account?  Thanks.

[tommi (sloooow) - chröme, 2013-11-14 09:21:27]

lgtm for chrome_frame/*

[Nico, 2013-11-14 14:41:40]

(sorry if this was discussed above, it wasn't clear to me from cl description or
bug)

https://chromiumcodereview.appspot.com/64193003/diff/130001/chrome/browser/ex...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/64193003/diff/130001/chrome/browser/ex...
chrome/browser/extensions/test_extension_prefs.cc:110: pref_service_ =
builder.CreateSyncable(pref_registry_.get()).Pass();
Why the With -> set_ change? I thought builders are usually used like

  pref_service_ = PrefServiceMockBuilder()
                    .WithUserFilePrefs(...)
                    .WithExtensionsPrefs(...)
                    .CreateSyncable(...)
                    .Pass();

With the set_ change, that's no longer possible.

[joth, 2013-11-14 16:28:20]

Lgtm reprised

On Wednesday, 13 November 2013, wrote:

> On 2013/11/14 02:04:23, joth__google wrote:
>
>> aw/ lgtm
>>
>
> Hmm, I think the CQ is going to complain that only your @chromium.orgaddress
is
> listed as an owner for aw/.  Mind re-stamping with that account?  Thanks.
>
> https://chromiumcodereview.appspot.com/64193003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2013-11-14 22:34:49]

https://chromiumcodereview.appspot.com/64193003/diff/130001/chrome/browser/ex...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/64193003/diff/130001/chrome/browser/ex...
chrome/browser/extensions/test_extension_prefs.cc:110: pref_service_ =
builder.CreateSyncable(pref_registry_.get()).Pass();
On 2013/11/14 14:41:40, Nico (in Tokyo until Nov 25) wrote:
> Why the With -> set_ change? I thought builders are usually used like
> 
>   pref_service_ = PrefServiceMockBuilder()
>                     .WithUserFilePrefs(...)
>                     .WithExtensionsPrefs(...)
>                     .CreateSyncable(...)
>                     .Pass();
> 
> With the set_ change, that's no longer possible.

In practice, these builders were almost never used that way.  Instead, the
WithFoo methods were being called just like set_foo methods (as you can see in
this file, for example).  So, I updated the method names to match how they were
predominantly being used.

[Nico, 2013-11-17 07:18:16]

On Thu, Nov 14, 2013 at 10:34 PM, <isherman@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/64193003/diff/
> 130001/chrome/browser/extensions/test_extension_prefs.cc
> File chrome/browser/extensions/test_extension_prefs.cc (right):
>
> https://chromiumcodereview.appspot.com/64193003/diff/
> 130001/chrome/browser/extensions/test_extension_prefs.cc#newcode110
> chrome/browser/extensions/test_extension_prefs.cc:110: pref_service_ =
> builder.CreateSyncable(pref_registry_.get()).Pass();
> On 2013/11/14 14:41:40, Nico (in Tokyo until Nov 25) wrote:
>
>> Why the With -> set_ change? I thought builders are usually used like
>>
>
>     pref_service_ = PrefServiceMockBuilder()
>>                      .WithUserFilePrefs(...)
>>                      .WithExtensionsPrefs(...)
>>                      .CreateSyncable(...)
>>                      .Pass();
>>
>
>  With the set_ change, that's no longer possible.
>>
>
> In practice, these builders were almost never used that way.  Instead,
> the WithFoo methods were being called just like set_foo methods (as you
> can see in this file, for example).  So, I updated the method names to
> match how they were predominantly being used.
>

Right, but then the class shouldn't be called "Builder" I think. If it's
called "Builder", it should behave like one. I'm fine with either renaming
the class (Factory? :-|) or changing the calling sites to look like the
usual builder thing. As is, I feel this makes the code weirder. (But if you
feel strongly otherwise, you might be able to convince me that I'm wrong.)


>
> https://chromiumcodereview.appspot.com/64193003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mattias Nissler (ping if slow), 2013-11-18 09:34:36]

On 2013/11/17 07:18:16, Nico (in Tokyo until Nov 25) wrote:
> On Thu, Nov 14, 2013 at 10:34 PM, <mailto:isherman@chromium.org> wrote:
> 
> >
> > https://chromiumcodereview.appspot.com/64193003/diff/
> > 130001/chrome/browser/extensions/test_extension_prefs.cc
> > File chrome/browser/extensions/test_extension_prefs.cc (right):
> >
> > https://chromiumcodereview.appspot.com/64193003/diff/
> > 130001/chrome/browser/extensions/test_extension_prefs.cc#newcode110
> > chrome/browser/extensions/test_extension_prefs.cc:110: pref_service_ =
> > builder.CreateSyncable(pref_registry_.get()).Pass();
> > On 2013/11/14 14:41:40, Nico (in Tokyo until Nov 25) wrote:
> >
> >> Why the With -> set_ change? I thought builders are usually used like
> >>
> >
> >     pref_service_ = PrefServiceMockBuilder()
> >>                      .WithUserFilePrefs(...)
> >>                      .WithExtensionsPrefs(...)
> >>                      .CreateSyncable(...)
> >>                      .Pass();
> >>
> >
> >  With the set_ change, that's no longer possible.
> >>
> >
> > In practice, these builders were almost never used that way.  Instead,
> > the WithFoo methods were being called just like set_foo methods (as you
> > can see in this file, for example).  So, I updated the method names to
> > match how they were predominantly being used.
> >
> 
> Right, but then the class shouldn't be called "Builder" I think. If it's
> called "Builder", it should behave like one. I'm fine with either renaming
> the class (Factory? :-|) or changing the calling sites to look like the
> usual builder thing. As is, I feel this makes the code weirder. (But if you
> feel strongly otherwise, you might be able to convince me that I'm wrong.)

Non-scoring drive-by: I'm with Nico, the thing looks more like a factory after
your change, so +1 to update the names as well. FWIW, I'm indifferent in my
preference for factory/builder, if you feel strong that we should go with the
factory version, that's fine with me :)

> 
> 
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/__extension_id__.png);">https://chromiumcodereview.appspot.com/64193003/</crxref>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2013-11-19 00:09:29]

Alright, renamed from "builder" to "factory".  PTAL :)

[Nico, 2013-11-19 00:22:58]

lgtm

[commit-bot: I haz the power, 2013-11-19 00:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/64193003/410001

[commit-bot: I haz the power, 2013-11-19 01:53:46]

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

[commit-bot: I haz the power, 2013-11-19 02:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/64193003/570001

[commit-bot: I haz the power, 2013-11-19 04:22:18]

Change committed as 235913