Reland: Closure-based API to PrefChangeObserver and PrefMember.

Reland: Closure-based API to PrefChangeObserver and PrefMember.

The original was in http://codereview.chromium.org/11368098/ (landed
as r166670) but seemed to increase memory usage significantly.

This version does away with storing the pref name in callbacks, which
should reduce memory usage.

TBR=ben@chromium.org,finnur@chromium.org
BUG=155525, 160177
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167179

[Jói, 2012-11-09 14:11:42]

Hold off on this for a little bit; while working on updating the first
batch of conversions I see that with a callback-based API it really
sucks not to be able to bind parameter-less methods (a lot of times
the fact that we now have a parameter means I have to introduce a
method where there was already a parameter-less method that could
handle the pref change directly).  Let me try to figure out a way to
enable this without large memory usage, and then I will re-upload.

Cheers,
Jói


On Fri, Nov 9, 2012 at 1:52 PM,  <joi@chromium.org> wrote:
> Reviewers: Mattias Nissler,
>
> Description:
> Reland: Closure-based API to PrefChangeObserver and PrefMember.
>
> The original was in http://codereview.chromium.org/11368098/ (landed
> as r166670) but seemed to increase memory usage significantly.
>
> This version does away with storing the pref name in callbacks, which
> should reduce memory usage.
>
> TBR=ben@chromium.org,finnur@chromium.org
> BUG=155525, 160177
>
> Please review this at http://codereview.chromium.org/11369153/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M base/prefs/public/pref_change_registrar.h
>   M base/prefs/public/pref_change_registrar.cc
>   M base/prefs/public/pref_change_registrar_unittest.cc
>   M chrome/browser/api/prefs/pref_member.h
>   M chrome/browser/api/prefs/pref_member.cc
>   M chrome/browser/extensions/extension_prefs_unittest.cc
>   M chrome/browser/prefs/pref_service_unittest.cc
>   M chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
>   M chrome/browser/ui/webui/options/core_options_handler.cc
>
>

[Jói, 2012-11-09 14:33:27]

OK, now I think it's ready for review.  This is how I should have done it from
the start I think.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-09 14:47:50]

looks good in general, one suggestion.

http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
File chrome/browser/api/prefs/pref_member.h (right):

http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
chrome/browser/api/prefs/pref_member.h:141: scoped_ptr<NamedChangeCallback>
observer_;
Other code uses base::Callback::is_null() for determining whether a callback is
actually valid instead of wrapping it in a scoped_ptr. Is this variant
preferable?

If you went with the is_null solution you could also drop the 2-parameter form
if PreferMemberBase::Init.

[Jói, 2012-11-09 14:56:15]

A Callback contains a scoped_refptr and a function pointer, so it
takes two pointers in both the null and non-null cases, whereas using
the scoped_ptr<NamedChangeCallback> takes one pointer in the null case
and 3 in the non-null case.

I had done a rough count of how many times PrefMember::Init was called
with NULL, and it looked like the majority of cases was using NULL,
but looking again it seems that most of those are actually test code.
So skipping the scoped_ptr looks better for production code, will
switch to that and re-upload.

Cheers,
Jói



On Fri, Nov 9, 2012 at 2:47 PM,  <mnissler@chromium.org> wrote:
> looks good in general, one suggestion.
>
>
>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
> File chrome/browser/api/prefs/pref_member.h (right):
>
>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
> chrome/browser/api/prefs/pref_member.h:141:
> scoped_ptr<NamedChangeCallback> observer_;
> Other code uses base::Callback::is_null() for determining whether a
> callback is actually valid instead of wrapping it in a scoped_ptr. Is
> this variant preferable?
>
> If you went with the is_null solution you could also drop the
> 2-parameter form if PreferMemberBase::Init.
>
> http://codereview.chromium.org/11369153/

[Jói, 2012-11-09 15:06:10]

PTAL, switched to using Callback::is_null().

On Fri, Nov 9, 2012 at 2:55 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> A Callback contains a scoped_refptr and a function pointer, so it
> takes two pointers in both the null and non-null cases, whereas using
> the scoped_ptr<NamedChangeCallback> takes one pointer in the null case
> and 3 in the non-null case.
>
> I had done a rough count of how many times PrefMember::Init was called
> with NULL, and it looked like the majority of cases was using NULL,
> but looking again it seems that most of those are actually test code.
> So skipping the scoped_ptr looks better for production code, will
> switch to that and re-upload.
>
> Cheers,
> Jói
>
>
>
> On Fri, Nov 9, 2012 at 2:47 PM,  <mnissler@chromium.org> wrote:
>> looks good in general, one suggestion.
>>
>>
>>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
>> File chrome/browser/api/prefs/pref_member.h (right):
>>
>>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
>> chrome/browser/api/prefs/pref_member.h:141:
>> scoped_ptr<NamedChangeCallback> observer_;
>> Other code uses base::Callback::is_null() for determining whether a
>> callback is actually valid instead of wrapping it in a scoped_ptr. Is
>> this variant preferable?
>>
>> If you went with the is_null solution you could also drop the
>> 2-parameter form if PreferMemberBase::Init.
>>
>> http://codereview.chromium.org/11369153/

[Jói, 2012-11-12 13:48:40]

mnissler: Friendly ping.

On Fri, Nov 9, 2012 at 3:05 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> PTAL, switched to using Callback::is_null().
>
> On Fri, Nov 9, 2012 at 2:55 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> A Callback contains a scoped_refptr and a function pointer, so it
>> takes two pointers in both the null and non-null cases, whereas using
>> the scoped_ptr<NamedChangeCallback> takes one pointer in the null case
>> and 3 in the non-null case.
>>
>> I had done a rough count of how many times PrefMember::Init was called
>> with NULL, and it looked like the majority of cases was using NULL,
>> but looking again it seems that most of those are actually test code.
>> So skipping the scoped_ptr looks better for production code, will
>> switch to that and re-upload.
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Fri, Nov 9, 2012 at 2:47 PM,  <mnissler@chromium.org> wrote:
>>> looks good in general, one suggestion.
>>>
>>>
>>>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
>>> File chrome/browser/api/prefs/pref_member.h (right):
>>>
>>>
http://codereview.chromium.org/11369153/diff/5001/chrome/browser/api/prefs/pr...
>>> chrome/browser/api/prefs/pref_member.h:141:
>>> scoped_ptr<NamedChangeCallback> observer_;
>>> Other code uses base::Callback::is_null() for determining whether a
>>> callback is actually valid instead of wrapping it in a scoped_ptr. Is
>>> this variant preferable?
>>>
>>> If you went with the is_null solution you could also drop the
>>> 2-parameter form if PreferMemberBase::Init.
>>>
>>> http://codereview.chromium.org/11369153/

[Mattias Nissler (ping if slow), 2012-11-12 13:52:42]

Sorry, I had missed your update somehow. LGTM.

[commit-bot: I haz the power, 2012-11-12 13:55:12]

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

[commit-bot: I haz the power, 2012-11-12 15:29:22]

Change committed as 167179