Use correct favicon scale factor on Android.

Use correct favicon scale factor on Android.

- Splits and moves Android's display DeviceInfo from content to gfx;
- Uses DeviceInfo for Android's Screen implementation;
- Uses PrimaryDisplay's scale to figure out favicon scale factor.

BUG=168319
BUG=117839
TEST=manual as in 168319; AwSettingsTest#testUseWideViewportLayoutWidth

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

[aruslan, 2013-01-16 20:01:06]

Please help with the review.
I'll add OWNERS once the initial pass is done.
Thanks!

[nilesh, 2013-01-16 20:23:55]

On 2013/01/16 20:01:06, aruslan wrote:
> Please help with the review.
> I'll add OWNERS once the initial pass is done.
> Thanks!

android lg with nits

[nilesh, 2013-01-16 20:24:01]

https://codereview.chromium.org/11886074/diff/4001/ui/gfx/android/device_info.h
File ui/gfx/android/device_info.h (right):

https://codereview.chromium.org/11886074/diff/4001/ui/gfx/android/device_info...
ui/gfx/android/device_info.h:23: int GetHeight();
It will be nice to have one line documentation on some of these. Is it Device
physical height in some unit or display height in pixels ? :)

Better if rename this to GetDisplayHeight and have a comment on the unit.

[aruslan, 2013-01-16 23:14:45]

Thanks, Nilesh! PTAL.

https://codereview.chromium.org/11886074/diff/4001/ui/gfx/android/device_info.h
File ui/gfx/android/device_info.h (right):

https://codereview.chromium.org/11886074/diff/4001/ui/gfx/android/device_info...
ui/gfx/android/device_info.h:23: int GetHeight();
On 2013/01/16 20:24:01, nileshagrawal1 wrote:
> It will be nice to have one line documentation on some of these. Is it Device
> physical height in some unit or display height in pixels ? :)
> 
> Better if rename this to GetDisplayHeight and have a comment on the unit.

Done.

[nilesh, 2013-01-16 23:22:07]

LGTMq

[aruslan, 2013-01-16 23:50:40]

Thanks, Nilesh!

Scott, Yaron -- please review and apply OWNERS seal to the following if the CL
makes sense.
I'll invite other OWNERS once this round is complete.

sky@ - ui/base/layout.cc
sky@ - ui/ui.gyp
yfriedman@ - content/browser/android/
yfriedman@ - content/common/android
yfriedman@ - content/content_jni.gypi
yfriedman@ - content/public/android/
yfriedman@ - ui/android/
yfriedman@ - ui/android/ui_jni_registrar.cc

[Yaron, 2013-01-17 00:38:48]

Seems reasonable but I'm not really familiar with the ui stuff.

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/device_info.h
File ui/gfx/android/device_info.h (right):

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/device_info...
ui/gfx/android/device_info.h:18: class DeviceInfo {
NIT: make this "class UI_EXPORT DeviceInfo" (for the component build)

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/gfx_jni_reg...
File ui/gfx/android/gfx_jni_registrar.cc (right):

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/gfx_jni_reg...
ui/gfx/android/gfx_jni_registrar.cc:14: RegisterBitmapAndroid(env);
Can you make this consistent with other registrars? Declare an array of
based::android::RegistrationMethod and call RegisterNativeMethods.

[sky, 2013-01-17 01:02:32]

LGTM

https://codereview.chromium.org/11886074/diff/8001/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/11886074/diff/8001/ui/base/layout.cc#newcode85
ui/base/layout.cc:85: for (int i = NUM_SCALE_FACTORS; --i > SCALE_FACTOR_100P;)
{
It's pretty unusual to have -- outside the last part of the if, why not the more
conventional:
for (int i = NSF - 1; i > SCALE_FACTORS_100P; --i) ?

[aruslan, 2013-01-17 02:11:47]

Scott -- thank you for the review!
Yaron -- PTAL at content/ and JNI/gyp-JNI stuff.

https://codereview.chromium.org/11886074/diff/8001/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/11886074/diff/8001/ui/base/layout.cc#newcode85
ui/base/layout.cc:85: for (int i = NUM_SCALE_FACTORS; --i > SCALE_FACTOR_100P;)
{
On 2013/01/17 01:02:32, sky wrote:
> It's pretty unusual to have -- outside the last part of the if, why not the
more
> conventional:
> for (int i = NSF - 1; i > SCALE_FACTORS_100P; --i) ?

Done.

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/device_info.h
File ui/gfx/android/device_info.h (right):

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/device_info...
ui/gfx/android/device_info.h:18: class DeviceInfo {
On 2013/01/17 00:38:48, Yaron wrote:
> NIT: make this "class UI_EXPORT DeviceInfo" (for the component build)

Done.

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/gfx_jni_reg...
File ui/gfx/android/gfx_jni_registrar.cc (right):

https://codereview.chromium.org/11886074/diff/8001/ui/gfx/android/gfx_jni_reg...
ui/gfx/android/gfx_jni_registrar.cc:14: RegisterBitmapAndroid(env);
On 2013/01/17 00:38:48, Yaron wrote:
> Can you make this consistent with other registrars? Declare an array of
> based::android::RegistrationMethod and call RegisterNativeMethods.

Moved everything to ui/android/ui_jni_registrar.cc.

[aruslan, 2013-01-17 02:18:12]

Summoning the OWNERS for review and OWNERS seals of approval if the changes make
sense:

avi@ - content/content_common.gypi
joth@ -
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
aelias@ - content/browser/renderer_host/render_widget_host_view_android.cc
jamesr@ - content/renderer/render_view_impl.cc
danakj@ - ui/gfx/screen_android.cc, ui/gfx/android/

Thank you!

[aruslan, 2013-01-17 02:20:40]

Please take a look and OWNERS-approve if the change is sensible.  Thanks!

avi@ - content/content_common.gypi
joth@ -
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
aelias@ - content/browser/renderer_host/render_widget_host_view_android.cc
jamesr@ - content/renderer/render_view_impl.cc
danakj@ - ui/gfx/screen_android.cc, ui/gfx/android/

[Yaron, 2013-01-17 02:20:50]

lgtm

https://codereview.chromium.org/11886074/diff/19001/chrome/test/base/chrome_t...
File chrome/test/base/chrome_test_suite.cc (right):

https://codereview.chromium.org/11886074/diff/19001/chrome/test/base/chrome_t...
chrome/test/base/chrome_test_suite.cc:206:
ui::android::RegisterJni(base::android::AttachCurrentThread());
It's a little strange that your change introduces this dep and yet you don't
need to register content deps. I guess it must be chrome native->content pure
native -> ui native + java?

[danakj, 2013-01-17 02:21:41]

I'm an owner of geometry types in ui/gfx. You'll need to find another ui owner
for this, sorry.

[nilesh, 2013-01-17 02:26:48]

https://codereview.chromium.org/11886074/diff/19001/ui/android/ui_jni_registr...
File ui/android/ui_jni_registrar.cc (right):

https://codereview.chromium.org/11886074/diff/19001/ui/android/ui_jni_registr...
ui/android/ui_jni_registrar.cc:14: bool RegisterBitmapAndroid(JNIEnv* env);
This is weird. Why is this declared here instead of java_bitmap.h? Similar to
how DeviceInfo has a static method to register its jni, JavaBitmap can have a
static method RegisterJavaBitmap ?

[aruslan, 2013-01-17 02:45:01]

https://codereview.chromium.org/11886074/diff/19001/chrome/test/base/chrome_t...
File chrome/test/base/chrome_test_suite.cc (right):

https://codereview.chromium.org/11886074/diff/19001/chrome/test/base/chrome_t...
chrome/test/base/chrome_test_suite.cc:206:
ui::android::RegisterJni(base::android::AttachCurrentThread());
On 2013/01/17 02:20:50, Yaron wrote:
> It's a little strange that your change introduces this dep and yet you don't
> need to register content deps. I guess it must be chrome native->content pure
> native -> ui native + java? 

Yes; it doesn't touch content/ at all:

chrome native FaviconHandlerTest::SetUp
ui native ui::test::SetSupportedScaleFactors
ui native ui::test::GetSupportedScaleFactors
ui/gfx native gfx::Screen::GetNativeScreen
ui/gfx native gfx::DeviceInfo
JNI

(That doesn't mean I added all necessary dependencies though.)

https://codereview.chromium.org/11886074/diff/19001/ui/android/ui_jni_registr...
File ui/android/ui_jni_registrar.cc (right):

https://codereview.chromium.org/11886074/diff/19001/ui/android/ui_jni_registr...
ui/android/ui_jni_registrar.cc:14: bool RegisterBitmapAndroid(JNIEnv* env);
On 2013/01/17 02:26:49, nileshagrawal1 wrote:
> This is weird. Why is this declared here instead of java_bitmap.h? Similar to
> how DeviceInfo has a static method to register its jni, JavaBitmap can have a
> static method RegisterJavaBitmap ?

Done.

[aelias_OOO_until_Jul13, 2013-01-17 04:24:12]

lgtm for content/browser/renderer_host/render_widget_host_view_android.cc

[jamesr, 2013-01-17 06:12:30]

content/renderer changes don't immediately make sense to me.

+cc thakis for favicon/scale stuff

https://codereview.chromium.org/11886074/diff/2026/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11886074/diff/2026/content/renderer/render_vi...
content/renderer/render_view_impl.cc:226: #include
"ui/gfx/android/device_info.h"
i don't see this file in my tree

https://codereview.chromium.org/11886074/diff/2026/content/renderer/render_vi...
content/renderer/render_view_impl.cc:668: scoped_ptr<gfx::DeviceInfo>
device_info(new gfx::DeviceInfo());
why are you changing the type go a gfx:: type when it's used only for a
GetNetworkCountryIso() call?  Why would a gfx:: type know about that?

[jamesr, 2013-01-17 06:18:06]

https://codereview.chromium.org/11886074/diff/2026/ui/gfx/android/device_info.h
File ui/gfx/android/device_info.h (right):

https://codereview.chromium.org/11886074/diff/2026/ui/gfx/android/device_info...
ui/gfx/android/device_info.h:44: std::string GetNetworkCountryIso();
wait - what's this have to do with graphics?

[Nico, 2013-01-17 06:31:32]

Sorry if this has been asked above.

https://codereview.chromium.org/11886074/diff/2026/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11886074/diff/2026/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2103:
int displayWidth = (int) (deviceInfo.getDisplayWidth() /
deviceInfo.getDIPScale());
Math.round()? You have real fractional DIP scaled on android, right?

https://codereview.chromium.org/11886074/diff/2026/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11886074/diff/2026/content/renderer/render_vi...
content/renderer/render_view_impl.cc:226: #include
"ui/gfx/android/device_info.h"
On 2013/01/17 06:12:30, jamesr wrote:
> i don't see this file in my tree

It's being added / moved in this CL.

https://codereview.chromium.org/11886074/diff/2026/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/11886074/diff/2026/ui/base/layout.cc#newcode86
ui/base/layout.cc:86: if (display_density >= kScaleFactorScales[i]) {
Is 2.0 high enough for android? Nexus 10 has close to 3x, right? Are 1.4 and 1.8
good fits?

Also, you seem to pick the largest factor <= the display scale factor – isn't it
better to pick the smallest >=?

[Avi (use Gerrit), 2013-01-17 19:53:53]

I was asked about gyp changes? lgtm

[joth, 2013-01-17 20:38:55]

+acleung FYI. (don't think this impacts your favicon patch, but just in case...)

[acleung1, 2013-01-17 22:22:25]

This seems to be orthogonal to what I am doing.

LGTM

[aruslan, 2013-01-17 23:01:10]

Thanks a ton for the suggestions and comments!
+mnaganov for AwSettingsTest ceiling/rounding review.

https://chromiumcodereview.appspot.com/11886074/diff/2026/android_webview/jav...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://chromiumcodereview.appspot.com/11886074/diff/2026/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2103:
int displayWidth = (int) (deviceInfo.getDisplayWidth() /
deviceInfo.getDIPScale());
On 2013/01/17 06:31:33, Nico wrote:
> Math.round()? You have real fractional DIP scaled on android, right?

The code below seems to tolerate off-by-one rounding errors.
I leave it up to mnaganov@ to decide.

https://chromiumcodereview.appspot.com/11886074/diff/2026/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/11886074/diff/2026/content/renderer/re...
content/renderer/render_view_impl.cc:668: scoped_ptr<gfx::DeviceInfo>
device_info(new gfx::DeviceInfo());
On 2013/01/17 06:12:30, jamesr wrote:
> why are you changing the type go a gfx:: type when it's used only for a
> GetNetworkCountryIso() call?  Why would a gfx:: type know about that?
Errr, thanks!
I moved this method back to content/ and renamed the classes to be
DeviceDisplayInfo and DeviceTelephonyInfo, to avoid confusion.

https://chromiumcodereview.appspot.com/11886074/diff/2026/ui/base/layout.cc
File ui/base/layout.cc (right):

https://chromiumcodereview.appspot.com/11886074/diff/2026/ui/base/layout.cc#n...
ui/base/layout.cc:86: if (display_density >= kScaleFactorScales[i]) {
On 2013/01/17 06:31:33, Nico wrote:
> Is 2.0 high enough for android? Nexus 10 has close to 3x, right? Are 1.4 and
1.8
> good fits?

1.5 and 3.0 are closer matches for 240 (mdpi) and 480 (xxhdpi) dpi with the
baseline of 160 dpi.  I extended the enum and array to match.
Done.

> 
> Also, you seem to pick the largest factor <= the display scale factor
–&nbsp;isn't it
> better to pick the smallest >=?

Done.

https://chromiumcodereview.appspot.com/11886074/diff/2026/ui/gfx/android/devi...
File ui/gfx/android/device_info.h (right):

https://chromiumcodereview.appspot.com/11886074/diff/2026/ui/gfx/android/devi...
ui/gfx/android/device_info.h:44: std::string GetNetworkCountryIso();
On 2013/01/17 06:18:07, jamesr wrote:
> wait - what's this have to do with graphics?
Moved the method out and renamed the classes to avoid confusion.
Done.

[mnaganov (inactive), 2013-01-18 00:39:50]

LGTM for
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
changes

https://chromiumcodereview.appspot.com/11886074/diff/2026/android_webview/jav...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://chromiumcodereview.appspot.com/11886074/diff/2026/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2103:
int displayWidth = (int) (deviceInfo.getDisplayWidth() /
deviceInfo.getDIPScale());
On 2013/01/17 23:01:10, aruslan wrote:
> On 2013/01/17 06:31:33, Nico wrote:
> > Math.round()? You have real fractional DIP scaled on android, right?
> 
> The code below seems to tolerate off-by-one rounding errors.
> I leave it up to mnaganov@ to decide.

I think it's fine as it is.

1) When I was writing the original code, I have checked other parts that do
similar density-related calculations, and they were also using floor.

2) As aruslan@ says, we don't check for exact match, but rather tolerate
off-by-one errors.

[jamesr, 2013-01-18 00:53:30]

content/renderer lgtm

[Nico, 2013-01-18 01:27:33]

lgtm

(ruslan checked with oshima and pkotwicz that the new constants shouldn't
surprise the asset system.)

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc
File ui/base/layout.cc (right):

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc#...
ui/base/layout.cc:85: for (int i = SCALE_FACTOR_100P; i < NUM_SCALE_FACTORS - 1;
++i) {
nit: I'd start with 0

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc#...
ui/base/layout.cc:87: if (i != SCALE_FACTOR_100P)
do you need to static_cast<ScaleFactor>(i) here?

[joth, 2013-01-18 01:41:04]

aw/ lgtm

[aruslan, 2013-01-18 03:21:07]

Thank you very much for reviews!

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc
File ui/base/layout.cc (right):

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc#...
ui/base/layout.cc:85: for (int i = SCALE_FACTOR_100P; i < NUM_SCALE_FACTORS - 1;
++i) {
On 2013/01/18 01:27:33, Nico wrote:
> nit: I'd start with 0
Discussed off-line; keeping as is.

https://chromiumcodereview.appspot.com/11886074/diff/28001/ui/base/layout.cc#...
ui/base/layout.cc:87: if (i != SCALE_FACTOR_100P)
On 2013/01/18 01:27:33, Nico wrote:
> do you need to static_cast<ScaleFactor>(i) here?

Done.

[commit-bot: I haz the power, 2013-01-23 23:04:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/11886074/45001

[commit-bot: I haz the power, 2013-01-24 01:50:13]

Change committed as 178472