Make clear that WindowAndroid::getDisplay must be used when supporting multi-display.

Make clear that WindowAndroid::getDisplay must be used when supporting multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake", displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089
Committed: https://crrev.com/05d1753af587de2e87ea796c4e7608b0254d26b3
Cr-Commit-Position: refs/heads/master@{#429305}

[mthiesse, 2016-10-26 16:51:14]

Description was changed from

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

BUG=625089
==========

to

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct
display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake",
displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089
==========

[mthiesse, 2016-10-26 16:52:02]

mthiesse@chromium.org changed reviewers:
+ boliu@chromium.org

[mthiesse, 2016-10-26 16:52:03]

PTAL

[boliu, 2016-10-26 17:35:53]

alright, name bikeshedding..

If you really want to use the name to discourage callers from calling it, then
getDefaultDisplay probably isn't strong enough. You'd need something like
getMayBreakMultiDisplay or something like that, and then with a more extensive
comment saying when it's ok, and when it's not etc. or..., getMaybeWrong?
maybeWrongGet? don't have any concise ideas..

[boliu, 2016-10-26 17:36:39]

On 2016/10/26 17:35:53, boliu wrote:
> alright, name bikeshedding..
> 
> If you really want to use the name to discourage callers from calling it, then
> getDefaultDisplay probably isn't strong enough. You'd need something like
> getMayBreakMultiDisplay or something like that, and then with a more extensive
> comment saying when it's ok, and when it's not etc. or..., getMaybeWrong?
> maybeWrongGet? don't have any concise ideas..

getUnsafe?

[mthiesse, 2016-10-26 22:32:56]

On 2016/10/26 17:36:39, boliu wrote:
> On 2016/10/26 17:35:53, boliu wrote:
> > alright, name bikeshedding..
> > 
> > If you really want to use the name to discourage callers from calling it,
then
> > getDefaultDisplay probably isn't strong enough. You'd need something like
> > getMayBreakMultiDisplay or something like that, and then with a more
extensive
> > comment saying when it's ok, and when it's not etc. or..., getMaybeWrong?
> > maybeWrongGet? don't have any concise ideas..
> 
> getUnsafe?

As per chat, settled on getNonMultiDisplay, and left a lengthy comment.

[mthiesse, 2016-10-26 22:33:17]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 22:33:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-10-26 22:38:32]

lgtm after you fix all the webview callsites too

[mthiesse, 2016-10-26 22:54:10]

On 2016/10/26 22:38:32, boliu wrote:
> lgtm after you fix all the webview callsites too

Which callsites are you referring to? I thought I got them all in this last
patchset.

[mthiesse, 2016-10-26 22:55:50]

mthiesse@chromium.org changed reviewers:
+ miguelg@chromium.org

[mthiesse, 2016-10-26 22:55:50]

miguelg@chromium.org: Please OWNERS review WindowAndroid.java and
ProfileDataCache.java

[boliu, 2016-10-26 22:56:25]

https://codereview.chromium.org/2455653002/diff/20001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java
(right):

https://codereview.chromium.org/2455653002/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java:325:
DisplayAndroid.getDefaultDisplay(testContainerView.getContext()).getDIPScale();
here? I didn't bother looking further..

[mthiesse, 2016-10-26 22:59:50]

On 2016/10/26 22:56:25, boliu wrote:
>
https://codereview.chromium.org/2455653002/diff/20001/android_webview/javates...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java
> (right):
> 
>
https://codereview.chromium.org/2455653002/diff/20001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java:325:
>
DisplayAndroid.getDefaultDisplay(testContainerView.getContext()).getDIPScale();
> here? I didn't bother looking further..

Oops sorry, somehow those changes didn't get posted. Re-upped.

[Tima Vaisburd, 2016-10-26 23:02:06]

timav@chromium.org changed reviewers:
+ timav@chromium.org

[Tima Vaisburd, 2016-10-26 23:02:07]

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To support
multi-display, obtain DisplayAndroid from WindowAndroid instead.
The name getNonMultiDisplay() implies one display situation, yet WindowAndroid
uses this very method "to support multi-display".

So far my understanding has been that an Activity can be created on a secondary
display, and the default display for that activity will be that secondary
display.

Could you, please, elaborate? Do you need to distinguish Display that correspond
to the physical one from VirtualDisplay?

[boliu, 2016-10-26 23:03:34]

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To support
multi-display, obtain DisplayAndroid from WindowAndroid instead.
On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> The name getNonMultiDisplay() implies one display situation, yet WindowAndroid
> uses this very method "to support multi-display".
> 
> So far my understanding has been that an Activity can be created on a
secondary
> display, and the default display for that activity will be that secondary
> display.
> 
> Could you, please, elaborate? Do you need to distinguish Display that
correspond
> to the physical one from VirtualDisplay?

it won't work for the fake display that will be added for VR

[mthiesse, 2016-10-26 23:17:46]

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To support
multi-display, obtain DisplayAndroid from WindowAndroid instead.
On 2016/10/26 23:03:33, boliu wrote:
> On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> > The name getNonMultiDisplay() implies one display situation, yet
WindowAndroid
> > uses this very method "to support multi-display".
> > 
> > So far my understanding has been that an Activity can be created on a
> secondary
> > display, and the default display for that activity will be that secondary
> > display.
> > 
> > Could you, please, elaborate? Do you need to distinguish Display that
> correspond
> > to the physical one from VirtualDisplay?
> 
> it won't work for the fake display that will be added for VR

That's right, normal WindowAndroid can use getNonMultiDisplay because it wants
the default display for the activity. VrWindowAndroid will overload getDisplay
to allow for the returning of fake displays not associated with a context.

I'll clarify a bit more in the comment here.

[Tima Vaisburd, 2016-10-26 23:23:30]

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To support
multi-display, obtain DisplayAndroid from WindowAndroid instead.
On 2016/10/26 23:17:46, mthiesse wrote:
> On 2016/10/26 23:03:33, boliu wrote:
> > On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> > > The name getNonMultiDisplay() implies one display situation, yet
> WindowAndroid
> > > uses this very method "to support multi-display".
> > > 
> > > So far my understanding has been that an Activity can be created on a
> > secondary
> > > display, and the default display for that activity will be that secondary
> > > display.
> > > 
> > > Could you, please, elaborate? Do you need to distinguish Display that
> > correspond
> > > to the physical one from VirtualDisplay?
> > 
> > it won't work for the fake display that will be added for VR
> 
> That's right, normal WindowAndroid can use getNonMultiDisplay because it wants
> the default display for the activity. VrWindowAndroid will overload getDisplay
> to allow for the returning of fake displays not associated with a context.
> 
> I'll clarify a bit more in the comment here.

I deduce that by NonMultiDisplay you mean context associated display?

[mthiesse, 2016-10-26 23:26:56]

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):

https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To support
multi-display, obtain DisplayAndroid from WindowAndroid instead.
On 2016/10/26 23:23:29, Tima Vaisburd wrote:
> On 2016/10/26 23:17:46, mthiesse wrote:
> > On 2016/10/26 23:03:33, boliu wrote:
> > > On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> > > > The name getNonMultiDisplay() implies one display situation, yet
> > WindowAndroid
> > > > uses this very method "to support multi-display".
> > > > 
> > > > So far my understanding has been that an Activity can be created on a
> > > secondary
> > > > display, and the default display for that activity will be that
secondary
> > > > display.
> > > > 
> > > > Could you, please, elaborate? Do you need to distinguish Display that
> > > correspond
> > > > to the physical one from VirtualDisplay?
> > > 
> > > it won't work for the fake display that will be added for VR
> > 
> > That's right, normal WindowAndroid can use getNonMultiDisplay because it
wants
> > the default display for the activity. VrWindowAndroid will overload
getDisplay
> > to allow for the returning of fake displays not associated with a context.
> > 
> > I'll clarify a bit more in the comment here.
> 
> I deduce that by NonMultiDisplay you mean context associated display?

Yes, but we want to be explicitly clear that code intending to support
MultiDisplay should not use this function. WindowAndroid is an exception, it
lives inside ui and knows what it's doing, and will be overloaded for VR.

[mthiesse, 2016-10-28 19:22:29]

Friendly ping,

PTAL miguelg

[Tima Vaisburd, 2016-10-28 20:28:08]

On 2016/10/26 23:26:56, mthiesse wrote:
>
https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right):
> 
>
https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To
support
> multi-display, obtain DisplayAndroid from WindowAndroid instead.
> On 2016/10/26 23:23:29, Tima Vaisburd wrote:
> > On 2016/10/26 23:17:46, mthiesse wrote:
> > > On 2016/10/26 23:03:33, boliu wrote:
> > > > On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> > > > > The name getNonMultiDisplay() implies one display situation, yet
> > > WindowAndroid
> > > > > uses this very method "to support multi-display".
> > > > > 
> > > > > So far my understanding has been that an Activity can be created on a
> > > > secondary
> > > > > display, and the default display for that activity will be that
> secondary
> > > > > display.
> > > > > 
> > > > > Could you, please, elaborate? Do you need to distinguish Display that
> > > > correspond
> > > > > to the physical one from VirtualDisplay?
> > > > 
> > > > it won't work for the fake display that will be added for VR
> > > 
> > > That's right, normal WindowAndroid can use getNonMultiDisplay because it
> wants
> > > the default display for the activity. VrWindowAndroid will overload
> getDisplay
> > > to allow for the returning of fake displays not associated with a context.
> > > 
> > > I'll clarify a bit more in the comment here.
> > 
> > I deduce that by NonMultiDisplay you mean context associated display?
> 
> Yes, but we want to be explicitly clear that code intending to support
> MultiDisplay should not use this function. WindowAndroid is an exception, it
> lives inside ui and knows what it's doing, and will be overloaded for VR.

I like your explanation.
I wonder who would use something like getDisplay(Context) for something that is
_not_
associated with the Context, but if you and Bo agreed that it is wise to do this
renaming
I do accept it.

[boliu, 2016-10-28 20:36:50]

On 2016/10/28 20:28:08, Tima Vaisburd wrote:
> On 2016/10/26 23:26:56, mthiesse wrote:
> >
>
https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
> > File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java
(right):
> > 
> >
>
https://codereview.chromium.org/2455653002/diff/20001/ui/android/java/src/org...
> > ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:93: * To
> support
> > multi-display, obtain DisplayAndroid from WindowAndroid instead.
> > On 2016/10/26 23:23:29, Tima Vaisburd wrote:
> > > On 2016/10/26 23:17:46, mthiesse wrote:
> > > > On 2016/10/26 23:03:33, boliu wrote:
> > > > > On 2016/10/26 23:02:07, Tima Vaisburd wrote:
> > > > > > The name getNonMultiDisplay() implies one display situation, yet
> > > > WindowAndroid
> > > > > > uses this very method "to support multi-display".
> > > > > > 
> > > > > > So far my understanding has been that an Activity can be created on
a
> > > > > secondary
> > > > > > display, and the default display for that activity will be that
> > secondary
> > > > > > display.
> > > > > > 
> > > > > > Could you, please, elaborate? Do you need to distinguish Display
that
> > > > > correspond
> > > > > > to the physical one from VirtualDisplay?
> > > > > 
> > > > > it won't work for the fake display that will be added for VR
> > > > 
> > > > That's right, normal WindowAndroid can use getNonMultiDisplay because it
> > wants
> > > > the default display for the activity. VrWindowAndroid will overload
> > getDisplay
> > > > to allow for the returning of fake displays not associated with a
context.
> > > > 
> > > > I'll clarify a bit more in the comment here.
> > > 
> > > I deduce that by NonMultiDisplay you mean context associated display?
> > 
> > Yes, but we want to be explicitly clear that code intending to support
> > MultiDisplay should not use this function. WindowAndroid is an exception, it
> > lives inside ui and knows what it's doing, and will be overloaded for VR.
> 
> I like your explanation.
> I wonder who would use something like getDisplay(Context) for something that
is
> _not_
> associated with the Context, but if you and Bo agreed that it is wise to do
this
> renaming
> I do accept it.

Client code won't even know there is such a thing, and client code hopefully
won't need to know (other than avoid using htis method..)

[Miguel Garcia, 2016-11-02 15:58:39]

lgtm for the ui/android files

[Miguel Garcia, 2016-11-02 15:59:33]

On 2016/11/02 15:58:39, Miguel Garcia wrote:
> lgtm for the ui/android files

oh and lgtm on ProfileDataCache.java too

[mthiesse, 2016-11-02 16:04:59]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-02 16:04:59]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2455653002/#ps80001
(title: "Revert change that snuck in from other branch")

[commit-bot: I haz the power, 2016-11-02 16:05:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 16:46:13]

Description was changed from

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct
display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake",
displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089
==========

to

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct
display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake",
displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089
==========

[commit-bot: I haz the power, 2016-11-02 16:46:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-02 16:51:27]

Description was changed from

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct
display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake",
displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089
==========

to

==========
Make clear that WindowAndroid::getDisplay must be used when supporting
multi-display.

Contexts cannot be clearly mapped to displays, so while you may get the correct
display from DisplayAndroid::get, you shouldn't count on that.

This is important because future work will be adding virtual, or "fake",
displays that will only be mapped to WindowAndroids, and not Contexts.

BUG=625089

Committed: https://crrev.com/05d1753af587de2e87ea796c4e7608b0254d26b3
Cr-Commit-Position: refs/heads/master@{#429305}
==========

[commit-bot: I haz the power, 2016-11-02 16:51:28]

Patchset 5 (id:??) landed as
https://crrev.com/05d1753af587de2e87ea796c4e7608b0254d26b3
Cr-Commit-Position: refs/heads/master@{#429305}