Android: Add support for returning real screen resolution.

Android: Add support for returning real screen resolution.

This is supported since JB-MR1, and doesn't subtract
window decorations etc. like other size metrics do.

TBR=yfriedman@chromium.org
BUG=329439
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243795

[epennerAtGoogle, 2013-12-20 02:55:33]

Ptal.

[epennerAtGoogle, 2013-12-20 03:11:30]

On 2013/12/20 02:55:33, epennerAtGoogle wrote:
> Ptal.

I figured returning -1 only on ICS was asking for bugs. So I made the public
method return an extra boolean as to whether it was supported.  We could
silently fall back on the not-real size, but in my case I also need to know if
the real-size is really real :P

[aruslan, 2013-12-20 17:55:15]

I'd suggest adding a separate "bool hasRealDimensions()" or something similar
given that you need this check anyway, and checking that it is supported in both
getRealWidth() and getRealHeight() is awkward.

If you'd prefer to keep it the way it is: the Chromium style required "bool*"
for output params instead of "bool&".

Otherwise it's good to go.

[epenner, 2013-12-20 19:27:58]

+sievers. Ptal.

[no sievers, 2013-12-20 19:32:44]

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
File ui/gfx/android/shared_device_display_info.cc (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
ui/gfx/android/shared_device_display_info.cc:51: DCHECK_NE(0,
real_display_height_);
I think if you throw an exception in Java instead, then the JNI call below would
trip some native CHECK() or DCHECK() in the auto-generated JNI code.

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
File ui/gfx/android/shared_device_display_info.h (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
ui/gfx/android/shared_device_display_info.h:25: int GetBitsPerPixel();
nit: I think GetPhysicalDisplayWidth/Height() would better match the naming in
other places (ContentViewCore).

[epennerAtGoogle, 2013-12-20 19:42:54]

> I'd suggest adding a separate "bool hasRealDimensions()" or something similar
> given that you need this check anyway, and checking that it is supported in
both
> getRealWidth() and getRealHeight() is awkward.
> 
> If you'd prefer to keep it the way it is: the Chromium style required "bool*"
> for output params instead of "bool&".
> 
> Otherwise it's good to go.

That makes sense too. The reason I didn't do that is that I was worried about is
someone calling the function and not realizing there is a 'hasRealDimensions'
function. This will work everywhere and then possibly cause nasty bugs only on
ICS.

However, if you guys like hasRealDimensions, I'll go with that.

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
File ui/gfx/android/shared_device_display_info.cc (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
ui/gfx/android/shared_device_display_info.cc:51: DCHECK_NE(0,
real_display_height_);
On 2013/12/20 19:32:45, sievers wrote:
> I think if you throw an exception in Java instead, then the JNI call below
would
> trip some native CHECK() or DCHECK() in the auto-generated JNI code.

Are you thinking we should just never call the function on ICS where it's not
supported (it would result in DCHECK)? If we added a 'hasPhysicalDimensions'
function this would work.

See my response to Ruslan on that, and let me know if that's what you would
like.

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
File ui/gfx/android/shared_device_display_info.h (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
ui/gfx/android/shared_device_display_info.h:25: int GetBitsPerPixel();
On 2013/12/20 19:32:45, sievers wrote:
> nit: I think GetPhysicalDisplayWidth/Height() would better match the naming in
> other places (ContentViewCore).

Will do.

[no sievers, 2013-12-20 19:51:19]

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/device_dis...
File ui/gfx/android/device_display_info.cc (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/device_dis...
ui/gfx/android/device_display_info.cc:28: if (!(supported = height > 0))
And could we just return 0 here and below if not supported (pre-JB MR1)?

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
File ui/gfx/android/shared_device_display_info.cc (right):

https://codereview.chromium.org/93933016/diff/40001/ui/gfx/android/shared_dev...
ui/gfx/android/shared_device_display_info.cc:51: DCHECK_NE(0,
real_display_height_);
On 2013/12/20 19:42:55, epennerAtGoogle wrote:
> On 2013/12/20 19:32:45, sievers wrote:
> > I think if you throw an exception in Java instead, then the JNI call below
> would
> > trip some native CHECK() or DCHECK() in the auto-generated JNI code.
> 
> Are you thinking we should just never call the function on ICS where it's not
> supported (it would result in DCHECK)? If we added a 'hasPhysicalDimensions'
> function this would work.
> 

I think you can actually do the same/similar check in the native code here and
not call into Java:

base::android::BuildInfo::GetInstance()->sdk_int() >= 17 // is JB MR1 or greater

[epennerAtGoogle, 2013-12-20 20:31:36]

> I think you can actually do the same/similar check in the native code here and
> not call into Java:
> 
> base::android::BuildInfo::GetInstance()->sdk_int() >= 17 // is JB MR1 or
greater

And the behavior when you call it without checking that? DCHECK? It's really
fine by me, I just went (possibly to far) out of my way to ensure nobody creates
an ICS bug.

Is everyone okay with DCHECK on ICS (I'll add a comment on the getter header)
and using build number directly to check for support?

[no sievers, 2013-12-20 21:32:39]

On 2013/12/20 20:31:36, epennerAtGoogle wrote:
> > I think you can actually do the same/similar check in the native code here
and
> > not call into Java:
> > 
> > base::android::BuildInfo::GetInstance()->sdk_int() >= 17 // is JB MR1 or
> greater
> 
> And the behavior when you call it without checking that? DCHECK? It's really
> fine by me, I just went (possibly to far) out of my way to ensure nobody
creates
> an ICS bug.
> 
> Is everyone okay with DCHECK on ICS (I'll add a comment on the getter header)
> and using build number directly to check for support?

Oh that's why I was thinking just return 0 if < JB_MR1 and let the caller handle
0.
I'd put a comment with the function though that it might return 0 if not
supported.

[epenner, 2013-12-20 23:50:15]

> Oh that's why I was thinking just return 0 if < JB_MR1 and let the caller
handle
> 0.
> I'd put a comment with the function though that it might return 0 if not
> supported.

Yeah, this is basically like patch-set 2, except I used -1.

Okay I've changed the name and just have a 'WARNING' comment to check for 0.

[no sievers, 2013-12-21 00:21:20]

lgtm

[epennerAtGoogle, 2013-12-21 00:24:26]

On 2013/12/21 00:21:20, sievers wrote:
> lgtm

Hope I don't regret landing this before the holidays :P

[commit-bot: I haz the power, 2013-12-21 00:26:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/93933016/230001

[commit-bot: I haz the power, 2013-12-21 00:44:35]

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

[epennerAtGoogle, 2013-12-21 00:48:45]

I'm TBRing... Since nobody is here to stop me! :P

But seriously, I'll address any concerns if there are any.

[commit-bot: I haz the power, 2013-12-21 00:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/93933016/230001

[commit-bot: I haz the power, 2013-12-21 01:21:22]

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

[epenner, 2014-01-08 03:56:38]

On 2013/12/21 01:21:22, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Yaron is the only OWNERS for ui/gfx/android. Ruslan, IIRC you made changes in
this area, can you review on behalf of Yaron and I'll TBR for him (I think he's
out most of this week)?

Also, +bulach/tedc for ui/android OWNERS lgtm.

[bulach, 2014-01-08 10:34:56]

lgtm (but please wait for ted, he's a better owner for all things UI). small
suggestion:

https://codereview.chromium.org/93933016/diff/420001/ui/gfx/android/shared_de...
File ui/gfx/android/shared_device_display_info.h (right):

https://codereview.chromium.org/93933016/diff/420001/ui/gfx/android/shared_de...
ui/gfx/android/shared_device_display_info.h:23: int GetPhysicalDisplayHeight();
nit: since this is the effective API via GetInstance(), maybe worth commenting
here (or adding in line 16, "see more in |DeviceDisplayInfo|") ?

[Ted C, 2014-01-08 21:43:22]

lgtm (w/ a couple small comments)

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

https://codereview.chromium.org/93933016/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:61: return 0;
braces are required in java always (unless the statement and condition can all
fit on a single line, which these might).

Same below.

https://codereview.chromium.org/93933016/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:62: Point size =
new Point();
How often is this called?

If it gets called very often, then we probably should keep a temp point around
instead of always creating new ones.

[epenner, 2014-01-08 22:32:17]

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

https://codereview.chromium.org/93933016/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:61: return 0;
On 2014/01/08 21:43:23, Ted C wrote:
> braces are required in java always (unless the statement and condition can all
> fit on a single line, which these might).
> 
> Same below.

Done.

https://codereview.chromium.org/93933016/diff/420001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:62: Point size =
new Point();
On 2014/01/08 21:43:23, Ted C wrote:
> How often is this called?
> 
> If it gets called very often, then we probably should keep a temp point around
> instead of always creating new ones.

Ooh, good one. It's not called often now, but did that anyway by adding
mTempPoint.

[epennerAtGoogle, 2014-01-08 23:09:22]

Okay addressed all feedback. I'm adding yfriedman as TBR since he's the only
OWNER for one file and won't be in until Friday.

[commit-bot: I haz the power, 2014-01-08 23:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/93933016/510001

[commit-bot: I haz the power, 2014-01-09 00:12:19]

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

[commit-bot: I haz the power, 2014-01-09 04:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/93933016/510001

[commit-bot: I haz the power, 2014-01-09 07:31:37]

Change committed as 243795

[Yaron, 2014-01-09 17:21:16]

rubberstamp lgtm. ted should be an owner