Remove PrefObserver usages, batch 13.

Remove PrefObserver usages, batch 13.

BUG=155525
R=mnissler@chromium.org,joi@chromium.org
TBR=jonnhyg@chromium.org # for notifications

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

[tfarina, 2012-11-21 13:18:21]

Please, take a look.

Specially at the use of base::DoNothing in font_settings_handler.

[Mattias Nissler (ping if slow), 2012-11-21 13:29:26]

Looks good in general, deferring to falken@ for the question on no callbacks for
prefs::kDefaultCharset changes.

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:128:
base::Bind(&base::DoNothing));
If we really don't want a change notification callback (which I doubt), you can
drop the 3rd parameter here. Let's ask an OWNER of this file.

[tfarina, 2012-11-21 13:36:24]

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:128:
base::Bind(&base::DoNothing));
On 2012/11/21 13:29:26, Mattias Nissler wrote:
> If we really don't want a change notification callback (which I doubt), you
can
> drop the 3rd parameter here.
That was my first attempt.

But dropping it causes:
../../chrome/browser/api/prefs/pref_member.h:177:50: error: too few arguments to
function call, expected 3, have 2
    subtle::PrefMemberBase::Init(pref_name, prefs);
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~                 ^
../../chrome/browser/ui/webui/options/font_settings_handler.cc:122:15: note: in
instantiation of member function 'PrefMember<std::basic_string<char> >::Init'
requested here
  fixed_font_.Init(prefs::kWebKitFixedFontFamily,
              ^
../../chrome/browser/api/prefs/pref_member.h:103:3: note: 'Init' declared here
  void Init(const char* pref_name, PrefServiceBase* prefs,
  ^

[Jói, 2012-11-21 13:37:45]

Looks good, let's get falken@'s feedback before you commit.

Cheers,
Jói

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:124:
base::Bind(&FontSettingsHandler::SetUpFixedFontSample,
I would group the initialization of fixed_font_ and default_fixed_font_size_ and
provide them both the same callback object.

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:128:
base::Bind(&base::DoNothing));
On 2012/11/21 13:29:26, Mattias Nissler wrote:
> If we really don't want a change notification callback (which I doubt), you
can
> drop the 3rd parameter here. Let's ask an OWNER of this file.

Agreed we should ask an OWNER.

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/media_galleries_handler.cc:113: void
MediaGalleriesHandler::OnGalleriesChanged() {
You've reordered methods here, which is OK, but in general when doing
refactoring changes I suggest not changing the order unless you find the .cc and
.h file orders already match before your change.

Reviewing the change when methods are reordered is slightly more difficult, as
the diff from the before and after versions of the methods are not as clear.

[tfarina, 2012-11-21 13:48:33]

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:124:
base::Bind(&FontSettingsHandler::SetUpFixedFontSample,
On 2012/11/21 13:37:45, Jói wrote:
> I would group the initialization of fixed_font_ and default_fixed_font_size_
and
> provide them both the same callback object.

Done.

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/media_galleries_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/media_galleries_handler.cc:113: void
MediaGalleriesHandler::OnGalleriesChanged() {
On 2012/11/21 13:37:45, Jói wrote:
> You've reordered methods here, which is OK, but in general when doing
> refactoring changes I suggest not changing the order unless you find the .cc
and
> .h file orders already match before your change.
> 
> Reviewing the change when methods are reordered is slightly more difficult, as
> the diff from the before and after versions of the methods are not as clear.

Done.

[Jói, 2012-11-21 13:52:38]

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:128:
base::Bind(&base::DoNothing));
On 2012/11/21 13:36:24, tfarina wrote:
> On 2012/11/21 13:29:26, Mattias Nissler wrote:
> > If we really don't want a change notification callback (which I doubt), you
> can
> > drop the 3rd parameter here.
> That was my first attempt.
> 
> But dropping it causes:
> ../../chrome/browser/api/prefs/pref_member.h:177:50: error: too few arguments
to
> function call, expected 3, have 2
>     subtle::PrefMemberBase::Init(pref_name, prefs);
>     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~                 ^
> ../../chrome/browser/ui/webui/options/font_settings_handler.cc:122:15: note:
in
> instantiation of member function 'PrefMember<std::basic_string<char> >::Init'
> requested here
>   fixed_font_.Init(prefs::kWebKitFixedFontFamily,
>               ^
> ../../chrome/browser/api/prefs/pref_member.h:103:3: note: 'Init' declared here
>   void Init(const char* pref_name, PrefServiceBase* prefs,
>   ^

For now, you can pass a NULL as the 3rd argument.  I will clean it up when I
remove the ability to pass a PrefObserver* to PrefMember::Init (and at that
point will remove all NULL 3rd arguments in favor of a 2-argument version).

[tfarina, 2012-11-21 14:01:30]

On 2012/11/21 13:52:38, Jói wrote:
>
https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/font_settings_handler.cc:128:
> base::Bind(&base::DoNothing));
> On 2012/11/21 13:36:24, tfarina wrote:
> > On 2012/11/21 13:29:26, Mattias Nissler wrote:
> > > If we really don't want a change notification callback (which I doubt),
you
> > can
> > > drop the 3rd parameter here.
> > That was my first attempt.
> > 
> > But dropping it causes:
> > ../../chrome/browser/api/prefs/pref_member.h:177:50: error: too few
arguments
> to
> > function call, expected 3, have 2
> >     subtle::PrefMemberBase::Init(pref_name, prefs);
> >     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~                 ^
> > ../../chrome/browser/ui/webui/options/font_settings_handler.cc:122:15: note:
> in
> > instantiation of member function 'PrefMember<std::basic_string<char>
>::Init'
> > requested here
> >   fixed_font_.Init(prefs::kWebKitFixedFontFamily,
> >               ^
> > ../../chrome/browser/api/prefs/pref_member.h:103:3: note: 'Init' declared
here
> >   void Init(const char* pref_name, PrefServiceBase* prefs,
> >   ^
> 
> For now, you can pass a NULL as the 3rd argument.  I will clean it up when I
> remove the ability to pass a PrefObserver* to PrefMember::Init (and at that
> point will remove all NULL 3rd arguments in favor of a 2-argument version).

Done.

[falken, 2012-11-21 14:21:45]

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.cc:128:
base::Bind(&base::DoNothing));
On 2012/11/21 13:29:26, Mattias Nissler wrote:
> If we really don't want a change notification callback (which I doubt), you
can
> drop the 3rd parameter here. Let's ask an OWNER of this file.

Right, there's nothing to do when the encoding changes. The sample text is all
Latin anyway ("Lorem ipsum...") and no style change is needed (unlike when the
font family or size changes).

For the record, though, I'm not an OWNER.

[Jói, 2012-11-21 14:23:50]

Thanks Matt.

+jhawkins for chrome/browser/ui/webui/options/OWNERS.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-21 14:36:48]

LGTM then.

[James Hawkins, 2012-11-26 18:19:35]

LGTM with nit.

https://codereview.chromium.org/11421029/diff/6/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/font_settings_handler.cc (right):

https://codereview.chromium.org/11421029/diff/6/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/font_settings_handler.cc:248:
SetUpStandardFontSample();
nit: Indentation is off.

[commit-bot: I haz the power, 2012-11-27 00:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11421029/14001

[commit-bot: I haz the power, 2012-11-27 01:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11421029/6007

[commit-bot: I haz the power, 2012-11-27 01:04:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11421029/1038