Android: disables observing changes for font prefs.

Android: disables observing changes for font prefs.

On Android, there's no exposed way to change font preferences.
This will save ~725KB of heap.

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

[bulach, 2013-03-18 12:23:33]

following from the discussion in the bug..

[falken, 2013-03-18 19:07:37]

My feeling is this should be okay, but am a little worried it can cause bugs in
cases I haven't considered or if font prefs are later exposed somehow on
Android. We might need to revert this in that case.

For example, do you know what happens when you change the UI language of
Android? I know on Chrome OS we have different default font preferences for
different system locales. However on Chrome OS changing the UI language forces
you to relaunch Chrome.

It might be worth skipping this step and trying your idea in the bug to remove
the use of StringPrintf first. If that's what's eating the memory and not
something intrinsic with the prefs system itself, it should solve this issue...

https://codereview.chromium.org/12529014/diff/1/chrome/browser/ui/prefs/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12529014/diff/1/chrome/browser/ui/prefs/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:141: #if !defined(OS_ANDROID)
Can you add a comment explaining why we don't do this on Android?

[Bernhard Bauer, 2013-03-18 19:19:01]

On 2013/03/18 19:07:37, falken wrote:
> My feeling is this should be okay, but am a little worried it can cause bugs
in
> cases I haven't considered or if font prefs are later exposed somehow on
> Android. We might need to revert this in that case.
> 
> For example, do you know what happens when you change the UI language of
> Android? I know on Chrome OS we have different default font preferences for
> different system locales. However on Chrome OS changing the UI language forces
> you to relaunch Chrome.

Yes, I think in the worst case users would not see the new default fonts until a
browser restart. AFAICT, that is also the case on other platforms right now, as
there is no code that changes the default in the PrefService if the locale
should change. 

> It might be worth skipping this step and trying your idea in the bug to remove
> the use of StringPrintf first. If that's what's eating the memory and not
> something intrinsic with the prefs system itself, it should solve this
issue...
> 
>
https://codereview.chromium.org/12529014/diff/1/chrome/browser/ui/prefs/prefs...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/12529014/diff/1/chrome/browser/ui/prefs/prefs...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:141: #if !defined(OS_ANDROID)
> Can you add a comment explaining why we don't do this on Android?

[bulach, 2013-03-18 19:28:54]

thanks both! I added some more comments trying to clarify the reasons, please
let me know your thoughts.

also: this change itself saves ~725KB.. :)
the other one, avoiding StringPrintf, will save far less (~100KB or so). I
haven't finished yet, but you can take a look at this Work In Progress:
https://codereview.chromium.org/12902021/

I'll clean that up tomorrow and send for proper review :) but if you have any
comments, feel free to send it!

[falken, 2013-03-18 20:52:48]

lgtm

On 2013/03/18 19:28:54, bulach wrote:
> thanks both! I added some more comments trying to clarify the reasons, please
> let me know your thoughts.
> 
> also: this change itself saves ~725KB.. :)
> the other one, avoiding StringPrintf, will save far less (~100KB or so). I
> haven't finished yet, but you can take a look at this Work In Progress:
> https://codereview.chromium.org/12902021/
> 
> I'll clean that up tomorrow and send for proper review :) but if you have any
> comments, feel free to send it!

[bulach, 2013-03-19 09:00:28]

+bauerb for OWNERS :)

[Bernhard Bauer, 2013-03-19 09:50:53]

LGTM

[commit-bot: I haz the power, 2013-03-19 13:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12529014/5001

[commit-bot: I haz the power, 2013-03-19 15:28:34]

Change committed as 189021