Add font_scale_factor_quirk pref.

Add font_scale_factor_quirk pref.

This is an Android-specific boolean pref defaulting to true, which indicates
whether font_scale_factor includes the 1.05-1.3x multiplier that is currently
applied by Clank on startup.

The goal is to move this multiplier out of Clank and into the content layer.
When font_scale_factor_quirk is false, the multiplier is applied in
content::ApplyWebPreferences.

This pref can be removed after Clank code is updated.

BUG=

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

[skobes, 2013-10-18 03:04:17]

I will need to add a bunch more reviewers but can you guys do a sanity check on
it first?

[aelias_OOO_until_Jul13, 2013-10-18 03:23:39]

I think you might run into the problem that the quirk bool is hard to remove
because the clank/ side sets it to false after the second patch.  It might work
better to introduce another numeric setting called for example
kBlinkFontScaleFactor.

Sorry about this repo split pain.

[skobes, 2013-10-18 16:41:04]

On 2013/10/18 03:23:39, aelias wrote:
> I think you might run into the problem that the quirk bool is hard to remove
> because the clank/ side sets it to false after the second patch.  It might
work
> better to introduce another numeric setting called for example
> kBlinkFontScaleFactor.

I think we can

1. land this, wait for it to roll into Clank
2. set to false in Clank
3. change default to false
4. stop setting it in Clank
5. remove it

The problem with another numeric setting is how to distinguish 1.0 from unset. 
We could default to -1 or something but that seems hacky.  And it will be just
as much work to remove font_scale_factor (assuming we don't want to live with
two settings forever).

[pdr., 2013-10-18 17:20:55]

On 2013/10/18 16:41:04, skobes wrote:
> 1. land this, wait for it to roll into Clank
> 2. set to false in Clank
> 3. change default to false
> 4. stop setting it in Clank
> 5. remove it

I think this is a reasonable plan. Overall, the code looks good to me as well.

I've cc'ed abarth who did a bunch of upstreaming work in the past and may have
some high-level pointers.

[abarth-chromium, 2013-10-18 17:24:36]

On 2013/10/18 17:20:55, pdr wrote:
> On 2013/10/18 16:41:04, skobes wrote:
> > 1. land this, wait for it to roll into Clank
> > 2. set to false in Clank
> > 3. change default to false
> > 4. stop setting it in Clank
> > 5. remove it
> 
> I think this is a reasonable plan. Overall, the code looks good to me as well.
> 
> I've cc'ed abarth who did a bunch of upstreaming work in the past and may have
> some high-level pointers.

That sounds like it could work.  The other option is to use a preprocessor
macro.  That will let you land the Blink and Clank CLs in parallel and also
remove them in parallel after the roll.

[skobes, 2013-10-18 17:32:35]

On 2013/10/18 17:24:36, abarth wrote:
> That sounds like it could work.  The other option is to use a preprocessor
> macro.  That will let you land the Blink and Clank CLs in parallel and also
> remove them in parallel after the roll.

Not sure I understand how a preprocessor macro would work here, could you
elaborate?

[abarth-chromium, 2013-10-18 17:43:24]

On 2013/10/18 17:32:35, skobes wrote:
> Not sure I understand how a preprocessor macro would work here, could you
> elaborate?

A) Land CL downstream:

#define NO_FONT_SCALE_FACTOR_QUIRK_DOWNSTREAM

#ifdef NO_FONT_SCALE_FACTOR_QUIRK_UPSTREAM
  .. act as if you're in the future where the quirk is gone ..
#endif

B) Land CL upstream:

#define NO_FONT_SCALE_FACTOR_QUIRK_UPSTREAM

#ifdef NO_FONT_SCALE_FACTOR_QUIRK_DOWNSTREAM
  .. act as if you're in the future where the quirk is gone ..
#endif

C) Wait for roll.

D) Remove the #ifdefs and #defines.

Notice that (A) and (B) can land in parallel.  The futuristic code will activate
on each side when it can see the other.

The approach you outlined above should work too.

[skobes, 2013-10-18 17:57:45]

On 2013/10/18 17:43:24, abarth wrote:
> #define NO_FONT_SCALE_FACTOR_QUIRK_DOWNSTREAM

What file would this #define appear in?  Seems like it has to be in the Clank
repo but #included in Chromium somehow?

[abarth-chromium, 2013-10-18 18:11:07]

On 2013/10/18 17:57:45, skobes wrote:
> On 2013/10/18 17:43:24, abarth wrote:
> > #define NO_FONT_SCALE_FACTOR_QUIRK_DOWNSTREAM
> 
> What file would this #define appear in?  Seems like it has to be in the Clank
> repo but #included in Chromium somehow?

Ah, this might not work for you.  This trick works for Chromium because Chromium
is both above and below Blink in the dependency diagram.

[skobes, 2013-10-18 23:10:52]

Ok, since there are no high level objections I'll move forward with this
approach.

aelias - can you be the primary reviewer?

bauerb - please review chrome/browser/ui/prefs/...

tedchoc - please review ui/android/java/src/... and ui/gfx/android/...

jamesr - please review content/renderer/... and webkit/common/...

avi - please review content/public/common/...

[Ted C, 2013-10-18 23:17:21]

On 2013/10/18 23:10:52, skobes wrote:
> Ok, since there are no high level objections I'll move forward with this
> approach.
> 
> aelias - can you be the primary reviewer?
> 
> bauerb - please review chrome/browser/ui/prefs/...
> 
> tedchoc - please review ui/android/java/src/... and ui/gfx/android/...
> 
> jamesr - please review content/renderer/... and webkit/common/...
> 
> avi - please review content/public/common/...

lgtm -- ui/android/java/src, ui/gfx/android

[Avi (use Gerrit), 2013-10-19 02:29:56]

OK.

LGTM

[aelias_OOO_until_Jul13, 2013-10-19 03:27:43]

lgtm

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
File content/renderer/web_preferences.cc (right):

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:131: if (minWidth <= kWidthForMinFSM) return
kMinFSM;
nit: return should be on another line

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:131: if (minWidth <= kWidthForMinFSM) return
kMinFSM;
nit: return should be on newline

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:135: float ratio = ((float)(minWidth -
kWidthForMinFSM)) /
nit: use static_cast<float>(), and no need for the second cast.

[Bernhard Bauer, 2013-10-19 10:17:37]

prefs LGTM.

[skobes, 2013-10-21 17:49:29]

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
File content/renderer/web_preferences.cc (right):

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:131: if (minWidth <= kWidthForMinFSM) return
kMinFSM;
On 2013/10/19 03:27:43, aelias wrote:
> nit: return should be on newline

Done.

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:131: if (minWidth <= kWidthForMinFSM) return
kMinFSM;
On 2013/10/19 03:27:43, aelias wrote:
> nit: return should be on another line

Done.

https://codereview.chromium.org/28053002/diff/1/content/renderer/web_preferen...
content/renderer/web_preferences.cc:135: float ratio = ((float)(minWidth -
kWidthForMinFSM)) /
On 2013/10/19 03:27:43, aelias wrote:
> nit: use static_cast<float>(), and no need for the second cast.

Done.

[jamesr, 2013-10-21 17:58:32]

https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
File content/renderer/web_preferences.cc (right):

https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
content/renderer/web_preferences.cc:130: int minWidth =
info.GetSmallestDIPWidth();
we shouldn't be doing logic like this here in the renderer - set up the value in
the browser process

[skobes, 2013-10-21 18:04:39]

On 2013/10/21 17:58:32, jamesr wrote:
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> File content/renderer/web_preferences.cc (right):
> 
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> content/renderer/web_preferences.cc:130: int minWidth =
> info.GetSmallestDIPWidth();
> we shouldn't be doing logic like this here in the renderer - set up the value
in
> the browser process

In ChromeContentBrowserClient::OverrideWebkitPrefs?  Or somewhere else?

[jamesr, 2013-10-21 18:06:37]

On 2013/10/21 18:04:39, skobes wrote:
> On 2013/10/21 17:58:32, jamesr wrote:
> >
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> > File content/renderer/web_preferences.cc (right):
> > 
> >
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> > content/renderer/web_preferences.cc:130: int minWidth =
> > info.GetSmallestDIPWidth();
> > we shouldn't be doing logic like this here in the renderer - set up the
value
> in
> > the browser process
> 
> In ChromeContentBrowserClient::OverrideWebkitPrefs?  Or somewhere else?

That sounds good.

[skobes, 2013-10-21 18:13:35]

On 2013/10/21 18:06:37, jamesr wrote:
> On 2013/10/21 18:04:39, skobes wrote:
> > On 2013/10/21 17:58:32, jamesr wrote:
> > >
> >
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> > > File content/renderer/web_preferences.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/28053002/diff/180001/content/renderer/web_pre...
> > > content/renderer/web_preferences.cc:130: int minWidth =
> > > info.GetSmallestDIPWidth();
> > > we shouldn't be doing logic like this here in the renderer - set up the
> value
> > in
> > > the browser process
> > 
> > In ChromeContentBrowserClient::OverrideWebkitPrefs?  Or somewhere else?
> 
> That sounds good.

The disadvantage is that WebPreferences::font_scale_factor will be different
from the value of webkit.webprefs.font_scale_factor in PrefService, which seems
a bit weird.

[skobes, 2013-10-21 22:29:46]

Moved logic out of ApplyWebPreferences and into
ChromeContentBrowserClient::OverrideWebkitPrefs.  Removed
WebPreferences::font_scale_factor_quirk which is no longer needed.

Renamed WebPreferences::font_scale_factor to text_autosizing_font_scale_factor
to make it more distinct from the webkit.webprefs.font_scale_factor pref.

PTAL

[jamesr, 2013-10-21 22:32:25]

lgtm

https://codereview.chromium.org/28053002/diff/270001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/28053002/diff/270001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:571: const float kMinFSM =
1.05f;
add 'static'

[skobes, 2013-10-21 22:35:57]

Added boliu for OWNERS in android_webview.

https://codereview.chromium.org/28053002/diff/270001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/28053002/diff/270001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:571: const float kMinFSM =
1.05f;
On 2013/10/21 22:32:26, jamesr wrote:
> add 'static'

Done.

[boliu, 2013-10-21 22:47:21]

android_webview lgtm

[commit-bot: I haz the power, 2013-10-21 22:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/28053002/420001

[commit-bot: I haz the power, 2013-10-21 23:08:25]

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

[skobes, 2013-10-21 23:59:10]

Adding yfriedman for ui/gfx/android/device_display_info.*.

Not sure why tedchoc's LGTM was not sufficient (http://crbug.com/309856).

[Yaron, 2013-10-22 00:18:32]

lgtm

https://codereview.chromium.org/28053002/diff/420001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right):

https://codereview.chromium.org/28053002/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:123: public int
getSmallestDIPWidth() {
since this is only accessed by native, please make private

[skobes, 2013-10-22 00:24:07]

https://codereview.chromium.org/28053002/diff/420001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right):

https://codereview.chromium.org/28053002/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:123: public int
getSmallestDIPWidth() {
On 2013/10/22 00:18:33, Yaron wrote:
> since this is only accessed by native, please make private

Done.

[commit-bot: I haz the power, 2013-10-22 00:25:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/28053002/380001

[commit-bot: I haz the power, 2013-10-22 04:04:02]

Change committed as 230042