Use Display.getRealMetrics() to calculate device size

Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861
Committed: https://crrev.com/c6d363b1b16ef1de57ad555ea3efabb61fe38801
Cr-Commit-Position: refs/heads/master@{#379955}

[Theresa, 2016-03-08 00:31:00]

twellington@chromium.org changed reviewers:
+ newt@chromium.org

[Theresa, 2016-03-08 00:31:01]

ptal - relatively small patch blocking next Dev push

[newt (away), 2016-03-08 04:16:13]

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:87:
applyOverrideConfiguration(overrideConfiguration);
Side-note: I just noticed that applyOverrideConfiguration() is only available on
API 17 and above. This method should be annotated with
@TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1) to keep lint from complaining. In
fact, it's a little worrying that lint *hasn't* been complaining.

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java (right):

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:57: * @param
context Android's context.
Does this need to be an Activity context, or will any old context do? You should
specify that here.

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:60:
@SuppressLint("NewApi")
Use the @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1) annotation instead.

Benefits of @TargetApi over @SuppressLint?
 1. If someone later starts calling APIs that are newer than API 17 in this
method, lint will complain about it.
 2. Once we drop support for API 17, it will be obvious that this method needs
updating / can be simplified, since it'll be annotated with
@TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1).

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:65: if
(Build.VERSION.CODENAME.equals("N")) {
Why not use this code path on all versions newer than API 17? That'll allow us
to simplify this method sooner (once we drop API 17).

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:67:
((WindowManager) context.getApplicationContext().getSystemService(
can you just remove "getApplicationContext()" here?

[Theresa, 2016-03-08 16:51:06]

Description was changed from

==========
Use Display.getRealMetrics() to calculate device size on N

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp on pre-N.

BUG=591861
==========

to

==========
Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861
==========

[Theresa, 2016-03-08 16:52:23]

I had originally removed the bit about "does not depend on the current window
size and is not" from the DeviceFormFactor javadocs since technically it can be
affected by window size. Now that this patch only uses smallestsScreenWidthDp on
devices running api level 16, does it make sense to add that language back?

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:87:
applyOverrideConfiguration(overrideConfiguration);
On 2016/03/08 04:16:13, newt wrote:
> Side-note: I just noticed that applyOverrideConfiguration() is only available
on
> API 17 and above. This method should be annotated with
> @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1) to keep lint from complaining.
In
> fact, it's a little worrying that lint *hasn't* been complaining.

Done. Maybe lint is smart and noticed the new API call was in an if block?

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java (right):

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:57: * @param
context Android's context.
On 2016/03/08 04:16:13, newt wrote:
> Does this need to be an Activity context, or will any old context do? You
should
> specify that here.


Any context should work, since it's just used to call getApplicationContext(). I
tested and it works with android.view.ContextThemeWrapper,
com.google.android.apps.chrome.ChromeApplicationInternal, and activities. 

I changed it to "@param context {@link Context} used to get the Application
Context."

Does that sound better?

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:60:
@SuppressLint("NewApi")
On 2016/03/08 04:16:13, newt wrote:
> Use the @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1) annotation instead.
> 
> Benefits of @TargetApi over @SuppressLint?
>  1. If someone later starts calling APIs that are newer than API 17 in this
> method, lint will complain about it.
>  2. Once we drop support for API 17, it will be obvious that this method needs
> updating / can be simplified, since it'll be annotated with
> @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1).

Done.

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:65: if
(Build.VERSION.CODENAME.equals("N")) {
On 2016/03/08 04:16:13, newt wrote:
> Why not use this code path on all versions newer than API 17? That'll allow us
> to simplify this method sooner (once we drop API 17).

Done.

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:67:
((WindowManager) context.getApplicationContext().getSystemService(
On 2016/03/08 04:16:13, newt wrote:
> can you just remove "getApplicationContext()" here?

Nope. The regular context doesn't have the window service so removing it causes
NPE.

[Theresa, 2016-03-08 19:26:20]

ptal

I tested w/ context.getApplicationContext().getResources().getDisplayMetrics()
and that still subtracts window decorations so it won't work. As discussed,
((WindowManager)
context.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay().getRealMetrics(metrics)
returns the size of the current display. I'll file a bug with Android to either
fix the method or update their docs... "real size of this display" sounds like
the real physical size of the screen.

[newt (away), 2016-03-08 21:38:23]

lgtm

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/1770883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:87:
applyOverrideConfiguration(overrideConfiguration);
On 2016/03/08 16:52:23, Theresa Wellington wrote:
> On 2016/03/08 04:16:13, newt wrote:
> > Side-note: I just noticed that applyOverrideConfiguration() is only
available
> on
> > API 17 and above. This method should be annotated with
> > @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1) to keep lint from
complaining.
> In
> > fact, it's a little worrying that lint *hasn't* been complaining.
> 
> Done. Maybe lint is smart and noticed the new API call was in an if block?

So... lint is smart about if blocks that check Build.VERSION.SDK_INT. However,
it doesn't understand "Build.VERSION.CODENAME.equals("N")". On the other hand,
lint doesn't catch NewApi usages when the method is called on "this".

In this case, let's leave out the @TargetApi annotation since lint isn't
complaining and after N is release, lint will understand this if block.

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java (right):

https://codereview.chromium.org/1770883004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/DeviceFormFactor.java:57: * @param
context Android's context.
On 2016/03/08 16:52:23, Theresa Wellington wrote:
> On 2016/03/08 04:16:13, newt wrote:
> > Does this need to be an Activity context, or will any old context do? You
> should
> > specify that here.
> 
> 
> Any context should work, since it's just used to call getApplicationContext().
I
> tested and it works with android.view.ContextThemeWrapper,
> com.google.android.apps.chrome.ChromeApplicationInternal, and activities. 
> 
> I changed it to "@param context {@link Context} used to get the Application
> Context."
> 
> Does that sound better?

Sure. As long as we're clear when a specific context is needed vs any old
context.

[Theresa, 2016-03-08 21:50:48]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-03-08 21:50:49]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1770883004/#ps100001
(title: "Remove target api annotation")

[commit-bot: I haz the power, 2016-03-08 21:51:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1770883004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1770883004/100001

[commit-bot: I haz the power, 2016-03-08 23:03:34]

Description was changed from

==========
Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861
==========

to

==========
Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861
==========

[commit-bot: I haz the power, 2016-03-08 23:03:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-08 23:04:44]

Description was changed from

==========
Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861
==========

to

==========
Use Display.getRealMetrics() to calculate device size

Regular DisplayMetrics subtract the window decorations, causing
the DeviceFormFactor.isTablet() calculation to be incorrect.
To account for multi-window on N, use Display.getRealMetrics() instead.
This API method is only available in level 17+, so use
Configuration.smallestScreenWidthDp as a fallback.

BUG=591861

Committed: https://crrev.com/c6d363b1b16ef1de57ad555ea3efabb61fe38801
Cr-Commit-Position: refs/heads/master@{#379955}
==========

[commit-bot: I haz the power, 2016-03-08 23:04:45]

Patchset 6 (id:??) landed as
https://crrev.com/c6d363b1b16ef1de57ad555ea3efabb61fe38801
Cr-Commit-Position: refs/heads/master@{#379955}

[Benoit L, 2016-03-09 13:16:06]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1776963002/ by lizeb@chromium.org.

The reason for reverting is: Speculative revert for
https://bugs.chromium.org/p/chromium/issues/detail?id=593322


BUG=593322.