Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409833004:
Add DeviceDisplayInfo getter in WindowAndroid.
|
Created:
5 years, 2 months ago by gsennton Modified:
3 years, 11 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, James Su, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd DeviceDisplayInfo getter in WindowAndroid.
To support external displays (i.e. use the metrics of those displays) we
need to fetch Context specific display metrics. This will be done
through WindowAndroid and in this CL we add the interface for doing so
and also start using that interface.
The actual functionality for using the metrics of the current
context/display rather than the application context will be added in a
follow-up.
BUG=310763
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed variable naming and jdduke nit. #
Total comments: 3
Patch Set 3 : Pass display area instead of NativeWindow to GLHelperHolder. #
Total comments: 39
Patch Set 4 : Add screen_android stub and minor fixes like const-qualifiers. #Patch Set 5 : Call Screen.SetScreenInstance before any calls to Screen.GetScreen to allow implementing ScreenAndr… #Patch Set 6 : Revert (unnecessary) changes caused by rebasing. #
Total comments: 14
Patch Set 7 : Some minor/comment/nits changes for sky@ and boliu@. #
Total comments: 6
Patch Set 8 : Fix failing bot, revert RWHVA changes, add NOTIMPLEMENTED to some functions in screen_android #
Messages
Total messages: 48 (9 generated)
gsennton@chromium.org changed reviewers: + boliu@chromium.org, jdduke@chromium.org
Second part of https://codereview.chromium.org/1144333004/ boliu@ doesn't own anything here but I'm adding you so you see the change :)
jdduke@chromium.org changed reviewers: + sievers@chromium.org
+sievers for content/browser/renderer_host/ bits, as I technically only own input-related code there. https://codereview.chromium.org/1409833004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/1409833004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.h:289: gfx::NativeWindow nativeWindow, nativeWindow should be native_window here and throughout. https://codereview.chromium.org/1409833004/diff/1/ui/android/window_android.h File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/1/ui/android/window_android.h... ui/android/window_android.h:86: // TODO(gsennton) update display info on screen orientation updates Nit: "TODO(gsennton): Update... updates."
gsennton@chromium.org changed reviewers: + sadrul@chromium.org
Adding sadrul@chromium.org for ui/gfx/BUILD.gn ui/gfx/screen_android.cc ui/snapshot/snapshot_android.cc i.e. snapshot and the moving of screen_android. I moved screen_android so that it can depend on view_android and window_android. Should we let screen_android stay in ui/gfx and add a dependency on ui/android from ui/gfx instead (though that would create a circual dependency...)? https://codereview.chromium.org/1409833004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/1409833004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.h:289: gfx::NativeWindow nativeWindow, On 2015/10/21 17:18:25, jdduke wrote: > nativeWindow should be native_window here and throughout. Done. https://codereview.chromium.org/1409833004/diff/1/ui/android/window_android.h File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/1/ui/android/window_android.h... ui/android/window_android.h:86: // TODO(gsennton) update display info on screen orientation updates On 2015/10/21 17:18:25, jdduke wrote: > Nit: "TODO(gsennton): Update... updates." Done.
https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:2026: start_time, base::Passed(&bitmap_pixels_lock), native_window), sievers@ is it safe to bind this pointer? I know WindowAndroid should outlast most things, but we've had some related lifetime issues in the past... If all that is really needed here is to get the display dimensions (width+height), any reason we shouldn't pass that in directly?
It would be really cool if we could keep the platform abstraction, rather than making the code totally android-specific now. We'll really probably have to redo it in such a way afterwards anyways for this to work with 'USE_AURA && OS_ANDROID'. Haven't looked in detail, but something along the lines of gfx::Screen::GetScreenFor(view/window) etc. would maybe be nice.
On 2015/10/22 20:17:30, sievers wrote: > It would be really cool if we could keep the platform abstraction, rather than > making the code totally android-specific now. We'll really probably have to redo > it in such a way afterwards anyways for this to work with 'USE_AURA && > OS_ANDROID'. > > Haven't looked in detail, but something along the lines of > gfx::Screen::GetScreenFor(view/window) etc. would maybe be nice. Right, so we could use gfx::Display gfx::Screen::GetDisplayNearestWindow(gfx::NativeView view) for places where we have a NativeView. Can we just add a function to gfx::Screen for retrieving a display from a NativeWindow?
On 2015/10/23 16:45:53, gsennton wrote: > On 2015/10/22 20:17:30, sievers wrote: > > It would be really cool if we could keep the platform abstraction, rather than > > making the code totally android-specific now. We'll really probably have to > redo > > it in such a way afterwards anyways for this to work with 'USE_AURA && > > OS_ANDROID'. > > > > Haven't looked in detail, but something along the lines of > > gfx::Screen::GetScreenFor(view/window) etc. would maybe be nice. > > Right, so we could use gfx::Display > gfx::Screen::GetDisplayNearestWindow(gfx::NativeView view) for places where we > have a NativeView. > Can we just add a function to gfx::Screen for retrieving a display from a > NativeWindow? That sounds good. On Aura NativeView+NativeWindow are actually both of type aura::window, and maybe we could consider a common interface for both on Android too - but probably not for this patch.
On 2015/10/23 17:13:06, sievers wrote: > On 2015/10/23 16:45:53, gsennton wrote: > > On 2015/10/22 20:17:30, sievers wrote: > > > It would be really cool if we could keep the platform abstraction, rather > than > > > making the code totally android-specific now. We'll really probably have to > > redo > > > it in such a way afterwards anyways for this to work with 'USE_AURA && > > > OS_ANDROID'. > > > > > > Haven't looked in detail, but something along the lines of > > > gfx::Screen::GetScreenFor(view/window) etc. would maybe be nice. > > > > Right, so we could use gfx::Display > > gfx::Screen::GetDisplayNearestWindow(gfx::NativeView view) for places where we > > have a NativeView. > > Can we just add a function to gfx::Screen for retrieving a display from a > > NativeWindow? > > That sounds good. On Aura NativeView+NativeWindow are actually both of type > aura::window, and maybe we could consider a common interface for both on Android > too - but probably not for this patch. Are you saying that we should add a function NativeWindow -> Display in a follow-up or are you referring to something else than that function? If we don't add a function for getting a Display from a NativeWindow in this CL snapshot_android.cc will stay the same as PS2 (i.e. use DeviceDisplayInfo). render_widget_host_view_android.cc could be changed to use a Display instead of DeviceDisplayInfo but then we have the problem of whether we should use the display area from GetPrimaryDisplayWidth/Height or just GetDisplayWidth/Height. Before this CL we used GetDisplayWidth/Height but by using the conversion from DeviceDisplayInfo to Display we would use GetPrimaryDisplayWidth/Height if defined and GetDisplayWidth/Height otherwise...
https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:2026: start_time, base::Passed(&bitmap_pixels_lock), native_window), On 2015/10/22 15:37:05, jdduke (slow) wrote: > sievers@ is it safe to bind this pointer? I know WindowAndroid should outlast > most things, but we've had some related lifetime issues in the past... > > If all that is really needed here is to get the display dimensions > (width+height), any reason we shouldn't pass that in directly? No this is actually not safe, since the pointer is mutable. We recently discussed the supported or expected use cases for SetContentViewCore() in https://codereview.chromium.org/1385543003/#msg20. And that it would be good to document that implicitly or explicitly, so this would be useful here now. (TLDR: we support detach from CVC, possibly deferred attach, but not attaching to a different one after we're attached.) We use the screen size to tweak the transfer buffer (shared memory size) when creating the GL helper. So this patch actually only handles that to the extent (same for compositor_impl_android.cc) that you create a compositor instance or RWHV while the initial WindowAndroid is already on the external screen. For us to be able to move it around screens more work is needed. You'd then also need to update the shared memory limits on the fly (or tear down and create a new context). So if we moved to another screen, then either WindowAndroid would have to change (then we can't store the pointer here), or the information it returns. Really I think we should have an extra level of indirection where this class stores a ViewAndroid that never changes and has a getter to get to the root WindowAndroid. Then you can support detaching and even moving without having to worry about it too much here, since you always use the getter to get to WindowAndroid. We probably want to deal with this incrementally. For this patch you can probably just bind the display size (instead of the pointer) to avoid use-after-free.
On 2015/10/26 14:41:56, gsennton wrote: > On 2015/10/23 17:13:06, sievers wrote: > > On 2015/10/23 16:45:53, gsennton wrote: > > > On 2015/10/22 20:17:30, sievers wrote: > > > > It would be really cool if we could keep the platform abstraction, rather > > than > > > > making the code totally android-specific now. We'll really probably have > to > > > redo > > > > it in such a way afterwards anyways for this to work with 'USE_AURA && > > > > OS_ANDROID'. > > > > > > > > Haven't looked in detail, but something along the lines of > > > > gfx::Screen::GetScreenFor(view/window) etc. would maybe be nice. > > > > > > Right, so we could use gfx::Display > > > gfx::Screen::GetDisplayNearestWindow(gfx::NativeView view) for places where > we > > > have a NativeView. > > > Can we just add a function to gfx::Screen for retrieving a display from a > > > NativeWindow? > > > > That sounds good. On Aura NativeView+NativeWindow are actually both of type > > aura::window, and maybe we could consider a common interface for both on > Android > > too - but probably not for this patch. > > Are you saying that we should add a function NativeWindow -> Display in a > follow-up or are you referring to something else than that function? > > If we don't add a function for getting a Display from a NativeWindow in this CL > snapshot_android.cc will stay the same as PS2 (i.e. use DeviceDisplayInfo). > > render_widget_host_view_android.cc could be changed to use a Display instead of > DeviceDisplayInfo but then we have the problem of whether we should use the > display area from GetPrimaryDisplayWidth/Height or just GetDisplayWidth/Height. > Before this CL we used GetDisplayWidth/Height but by using the conversion from > DeviceDisplayInfo to Display we would use GetPrimaryDisplayWidth/Height if > defined and GetDisplayWidth/Height otherwise... Maybe let's ignore this for now, since the patch doesn't make it less platform-independent than before (DveiceDisplayInfo is still only used on non-aura builds for example).
ptal :) sievers@ for content/ sadrul@ for ui/ https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:2026: start_time, base::Passed(&bitmap_pixels_lock), native_window), On 2015/10/26 20:42:25, sievers wrote: > On 2015/10/22 15:37:05, jdduke (slow) wrote: > > sievers@ is it safe to bind this pointer? I know WindowAndroid should outlast > > most things, but we've had some related lifetime issues in the past... > > > > If all that is really needed here is to get the display dimensions > > (width+height), any reason we shouldn't pass that in directly? > > No this is actually not safe, since the pointer is mutable. > > We recently discussed the supported or expected use cases for > SetContentViewCore() in https://codereview.chromium.org/1385543003/#msg20. And > that it would be good to document that implicitly or explicitly, so this would > be useful here now. (TLDR: we support detach from CVC, possibly deferred attach, > but not attaching to a different one after we're attached.) > > We use the screen size to tweak the transfer buffer (shared memory size) when > creating the GL helper. So this patch actually only handles that to the extent > (same for compositor_impl_android.cc) that you create a compositor instance or > RWHV while the initial WindowAndroid is already on the external screen. For us > to be able to move it around screens more work is needed. You'd then also need > to update the shared memory limits on the fly (or tear down and create a new > context). > > So if we moved to another screen, then either WindowAndroid would have to change > (then we can't store the pointer here), or the information it returns. > > Really I think we should have an extra level of indirection where this class > stores a ViewAndroid that never changes and has a getter to get to the root > WindowAndroid. Then you can support detaching and even moving without having to > worry about it too much here, since you always use the getter to get to > WindowAndroid. > > We probably want to deal with this incrementally. For this patch you can > probably just bind the display size (instead of the pointer) to avoid > use-after-free. Since all we need from the window_android pointer is the display area I've updated the code so that it only passes the display area. To make sure that I follow the discussion: why is it a problem that the pointer is mutable? Regarding moving screens, if I understand correctly WindowAndroid is basically a wrapper around a Context, and a Context targets one single display so unless we are reusing the same compositor/RWHV for several WindowAndroids targeting different displays a moving of screens should never occur..?
lgtm On 2015/10/27 11:40:24, gsennton wrote: > To make sure that I follow the discussion: why is it a problem that the pointer > is mutable? > I think that a RWHV might be able to outlive WindowAndroid in certain corner cases (or at least we had races like that in the past). If you bind the pointer to the callback we wouldn't see if SetContentViewCore(nullptr) was getting called, and possibly get a use-after-free. > Regarding moving screens, if I understand correctly WindowAndroid is basically a > wrapper around a Context, and a Context targets one single display so unless we > are reusing the same compositor/RWHV for several WindowAndroids targeting > different displays a moving of screens should never occur..? Ok good, that makes it simpler.
> > Regarding moving screens, if I understand correctly WindowAndroid is basically > a > > wrapper around a Context, and a Context targets one single display so unless > we > > are reusing the same compositor/RWHV for several WindowAndroids targeting > > different displays a moving of screens should never occur..? > > Ok good, that makes it simpler. I would like to revise my latest comment - Android does not prevent a Context from changing displays during its lifetime (and it is possible that this will have to be supported in the future) but I guess this is something we can consider in another CL.... (sorry for the incorrect info) sadrul@ could you ptal at ui/? :)
Can you add an owner from ui/android/ for changes in there? https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:14: namespace gfx { namespace ui https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.h:76: const gfx::DeviceDisplayInfo& GetDeviceDisplayInfo(); const method too? https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.cc File ui/gfx/screen_android.cc (left): https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.c... ui/gfx/screen_android.cc:83: return new ScreenAndroid; You still need to define CreateNativeScreen() in the gfx component, right? You can maybe reuse the one aura has (screen_aura.cc)
ajith.v@chromium.org changed reviewers: + ajith.v@chromium.org
Please check in line comments. https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() { DCHECK(device_display_info_) ?
gsennton@chromium.org changed reviewers: + tedchoc@chromium.org
Ok, adding tedchoc@ here for ui/android. This change is related to https://codereview.chromium.org/1419843002/ (these changes are split up from one larger CL which was very difficult to handle). I am moving screen_android so that it can depend on ViewAndroid and WindowAndroid (so that we can use GetDisplayNearestWindow using a ViewAndroid to get display metrics for a display related to a certain View/WindowAndroid rather than just the primary display) As sadrul@ pointed out, if we move screen_android we would have to replace the CreateNativeScreen with a call to SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, ...) since CreateNativeScreen needs to be defined in ui/gfx (and we now move ScreenAndroid so we can't create a ScreenAndroid from CreateNativeScreen). Do you (tedchoc@) know where we could put a SetScreenInstance call for android so that it is called before we ever need a screen instance?
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); What is gfx::Display used for? How do they get notified of display changes? In particular, rotation seems to very quickly become inaccurate if it is held onto and not updated. Is Display supposed to be a snapshot in time or is it support to always reflect the current state? If the latter, then it looks like it was wrong before and likely to be "more wrong" if we are dealing with an activity? https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); DeviceDisplayInfo seems very tied to the application context. If we change it to support holding onto a Context, it has the same memory leak possibilities of WindowAndroid.
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/11/05 23:15:58, Ted C wrote: > What is gfx::Display used for? How do they get notified of display changes? In > particular, rotation seems to very quickly become inaccurate if it is held onto > and not updated. > > Is Display supposed to be a snapshot in time or is it support to always reflect > the current state? If the latter, then it looks like it was wrong before and > likely to be "more wrong" if we are dealing with an activity? In all of the cases I have seen gfx::Display in use we simply grab some piece of info from it (especially device scale factor / DPI), i.e. we use the Display as a snapshot (though, I guess that observation is not a guarantee that we are/will always use it in that way..). I don't quite follow how it would be more wrong (if we hold on to Displays) when dealing with an activity? https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/11/05 23:15:58, Ted C wrote: > DeviceDisplayInfo seems very tied to the application context. If we change it > to support holding onto a Context, it has the same memory leak possibilities of > WindowAndroid. Right, so the idea would be to no longer hold on to any Context in DeviceDisplayInfo. AFAICT there are two ways we could do this: 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can fetch a WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is tied to a certain display so we can fetch display metrics from that. 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the presentation context or the application context, shouldn't matter) and pass the display ID (associated with the presentation context) to DeviceDisplayInfo as a reference to the display we are interested in - and then fetch display info through the DisplayManager.getDisplay(displayID).
@tedchoc did #23 answer your questions sufficiently or is there something I should rethink here?
Sadly, it seems Bo is out for a while :-/. If you investigate and see that we can hold onto something and be sure it won't leak, then we can go forward with that for now. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/11/06 16:52:14, gsennton wrote: > On 2015/11/05 23:15:58, Ted C wrote: > > What is gfx::Display used for? How do they get notified of display changes? > In > > particular, rotation seems to very quickly become inaccurate if it is held > onto > > and not updated. > > > > Is Display supposed to be a snapshot in time or is it support to always > reflect > > the current state? If the latter, then it looks like it was wrong before and > > likely to be "more wrong" if we are dealing with an activity? > > In all of the cases I have seen gfx::Display in use we simply grab some piece of > info from it (especially device scale factor / DPI), i.e. we use the Display as > a snapshot (though, I guess that observation is not a guarantee that we are/will > always use it in that way..). > > I don't quite follow how it would be more wrong (if we hold on to Displays) when > dealing with an activity? I must say much of my skepticism is around my lack of knowledge of what a Display represents in Android. Does it truly represent the screen dimensions and not the window bounds/available draw area? In samsung multi window, the draw area != the screen size and I don't know what this is supposed to be. Also, the fact that this is a snapshot in time is really really bad for things like orientation. To me, we should not set orientation via this at all or we need to make this actually track updates from the display (outside the scope of this I suspect). https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/11/06 16:52:14, gsennton wrote: > On 2015/11/05 23:15:58, Ted C wrote: > > DeviceDisplayInfo seems very tied to the application context. If we change it > > to support holding onto a Context, it has the same memory leak possibilities > of > > WindowAndroid. > > Right, so the idea would be to no longer hold on to any Context in > DeviceDisplayInfo. > > AFAICT there are two ways we could do this: > > 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can fetch a > WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is tied > to a certain display so we can fetch display metrics from that. > > 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the > presentation context or the application context, shouldn't matter) and pass the > display ID (associated with the presentation context) to DeviceDisplayInfo as a > reference to the display we are interested in - and then fetch display info > through the DisplayManager.getDisplay(displayID). Again, I don't know what the lifecycles of either WindowManager or DisplayManager are to know whether they are actually better than Context. Do they transitively hold onto a Context ref by any means that we don't expect? If it does, then it could potentially causes leaks on webview if we aren't clearing the references where appropriate. If the java side of WindowAndroid was in charge of creating display infos, then it could just pass a weak ref to the java class and it could use the same leaky behavior as WindowAndroid. If sievers@ and maybe boliu@ don't think this is an issue than I can be convinced, but as this code is in areas I'm not familiar with I have to operate on my standard level of skepticism.
Thanks for being skeptical :) https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/11/25 19:35:52, Ted C wrote: > On 2015/11/06 16:52:14, gsennton wrote: > > On 2015/11/05 23:15:58, Ted C wrote: > > > What is gfx::Display used for? How do they get notified of display changes? > > > In > > > particular, rotation seems to very quickly become inaccurate if it is held > > onto > > > and not updated. > > > > > > Is Display supposed to be a snapshot in time or is it support to always > > reflect > > > the current state? If the latter, then it looks like it was wrong before > and > > > likely to be "more wrong" if we are dealing with an activity? > > > > In all of the cases I have seen gfx::Display in use we simply grab some piece > of > > info from it (especially device scale factor / DPI), i.e. we use the Display > as > > a snapshot (though, I guess that observation is not a guarantee that we > are/will > > always use it in that way..). > > > > I don't quite follow how it would be more wrong (if we hold on to Displays) > when > > dealing with an activity? > > I must say much of my skepticism is around my lack of knowledge of > what a Display represents in Android. > > Does it truly represent the screen dimensions and not the window > bounds/available draw area? In samsung multi window, the draw area > != the screen size and I don't know what this is supposed to be. So there are two different sizes that can be fetched from Display - the screen size (getRealSize) and the size of the application window (getSize). > Also, the fact that this is a snapshot in time is really really bad > for things like orientation. To me, we should not set orientation > via this at all or we need to make this actually track updates from > the display (outside the scope of this I suspect). I might be misunderstanding your comment here about orientation changes but the idea would be to update the DeviceDisplayInfo by using ScreenOrientationListener which listens to changes in orientation. Right now that class assumes there is only one display, so it needs to be changed to listen to several displays (the biggest problem with this is making sure that the listeners are not dependent on our activity/display context in any way, because then we can't unregister them if our context goes away). https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/11/25 19:35:52, Ted C wrote: > On 2015/11/06 16:52:14, gsennton wrote: > > On 2015/11/05 23:15:58, Ted C wrote: > > > DeviceDisplayInfo seems very tied to the application context. If we change > it > > > to support holding onto a Context, it has the same memory leak possibilities > > of > > > WindowAndroid. > > > > Right, so the idea would be to no longer hold on to any Context in > > DeviceDisplayInfo. > > > > AFAICT there are two ways we could do this: > > > > 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can fetch a > > WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is > tied > > to a certain display so we can fetch display metrics from that. > > > > 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the > > presentation context or the application context, shouldn't matter) and pass > the > > display ID (associated with the presentation context) to DeviceDisplayInfo as > a > > reference to the display we are interested in - and then fetch display info > > through the DisplayManager.getDisplay(displayID). > > Again, I don't know what the lifecycles of either WindowManager or > DisplayManager are to know whether they are actually better than > Context. Do they transitively hold onto a Context ref by any means > that we don't expect? > > If it does, then it could potentially causes leaks on webview if we > aren't clearing the references where appropriate. > > If the java side of WindowAndroid was in charge of creating display > infos, then it could just pass a weak ref to the java class and it > could use the same leaky behavior as WindowAndroid. > > If sievers@ and maybe boliu@ don't think this is an issue than I can > be convinced, but as this code is in areas I'm not familiar with I > have to operate on my standard level of skepticism. Looking at this again, I don't think we should hold references to any non-trivial Objects (fetched through our Context) beyond the Context lifetime (since it is unsustainable to keep track of which Objects are OK to leak and we might even leak the Context itself through some chain of references). This also means that we should probably not hold a reference to the display/activity context Resources in ResourceManager as we do since https://codereview.chromium.org/1419843002/. We should probably fetch non-display resources from the application context rather than the current context in that class. Since we shouldn't hold strong references to objects fetched through our Context this limits our alternatives in DeviceDisplayInfo. We could possibly use the DisplayManager fetched from the application context (not the activity/presentation context!) to fetch info about different displays. And then store a displayID in each WindowAndroid (fetched through its display/activity context). This display ID would be used to fetch the correct display for our context.
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/12/02 15:37:05, gsennton wrote: > On 2015/11/25 19:35:52, Ted C wrote: > > On 2015/11/06 16:52:14, gsennton wrote: > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > What is gfx::Display used for? How do they get notified of display > changes? > > > > > In > > > > particular, rotation seems to very quickly become inaccurate if it is held > > > onto > > > > and not updated. > > > > > > > > Is Display supposed to be a snapshot in time or is it support to always > > > reflect > > > > the current state? If the latter, then it looks like it was wrong before > > and > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > In all of the cases I have seen gfx::Display in use we simply grab some > piece > > of > > > info from it (especially device scale factor / DPI), i.e. we use the Display > > as > > > a snapshot (though, I guess that observation is not a guarantee that we > > are/will > > > always use it in that way..). > > > > > > I don't quite follow how it would be more wrong (if we hold on to Displays) > > when > > > dealing with an activity? > > > > I must say much of my skepticism is around my lack of knowledge of > > what a Display represents in Android. > > > > Does it truly represent the screen dimensions and not the window > > bounds/available draw area? In samsung multi window, the draw area > > != the screen size and I don't know what this is supposed to be. > > So there are two different sizes that can be fetched from Display - the screen > size (getRealSize) and the size of the application window (getSize). > > > Also, the fact that this is a snapshot in time is really really bad > > for things like orientation. To me, we should not set orientation > > via this at all or we need to make this actually track updates from > > the display (outside the scope of this I suspect). > > I might be misunderstanding your comment here about orientation changes but the > idea would be to update the DeviceDisplayInfo by using ScreenOrientationListener > which listens to changes in orientation. I think the "easiest" thing would be to actually not create a snapshot of the display values at all but simply pull them as needed. But that requires owning a context ref and could potentially slow everything down as we are now generating these once and reading the values are cheap. > Right now that class assumes there is > only one display, so it needs to be changed to listen to several displays (the > biggest problem with this is making sure that the listeners are not dependent on > our activity/display context in any way, because then we can't unregister them > if our context goes away). https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/12/02 15:37:05, gsennton wrote: > On 2015/11/25 19:35:52, Ted C wrote: > > On 2015/11/06 16:52:14, gsennton wrote: > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > DeviceDisplayInfo seems very tied to the application context. If we > change > > it > > > > to support holding onto a Context, it has the same memory leak > possibilities > > > of > > > > WindowAndroid. > > > > > > Right, so the idea would be to no longer hold on to any Context in > > > DeviceDisplayInfo. > > > > > > AFAICT there are two ways we could do this: > > > > > > 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can fetch > a > > > WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is > > tied > > > to a certain display so we can fetch display metrics from that. > > > > > > 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the > > > presentation context or the application context, shouldn't matter) and pass > > the > > > display ID (associated with the presentation context) to DeviceDisplayInfo > as > > a > > > reference to the display we are interested in - and then fetch display info > > > through the DisplayManager.getDisplay(displayID). > > > > Again, I don't know what the lifecycles of either WindowManager or > > DisplayManager are to know whether they are actually better than > > Context. Do they transitively hold onto a Context ref by any means > > that we don't expect? > > > > If it does, then it could potentially causes leaks on webview if we > > aren't clearing the references where appropriate. > > > > If the java side of WindowAndroid was in charge of creating display > > infos, then it could just pass a weak ref to the java class and it > > could use the same leaky behavior as WindowAndroid. > > > > If sievers@ and maybe boliu@ don't think this is an issue than I can > > be convinced, but as this code is in areas I'm not familiar with I > > have to operate on my standard level of skepticism. > > Looking at this again, I don't think we should hold references to any > non-trivial Objects (fetched through our Context) beyond the Context lifetime > (since it is unsustainable to keep track of which Objects are OK to leak and we > might even leak the Context itself through some chain of references). This also > means that we should probably not hold a reference to the display/activity > context Resources in ResourceManager as we do since > https://codereview.chromium.org/1419843002/. > We should probably fetch non-display resources from the application context > rather than the current context in that class. > > Since we shouldn't hold strong references to objects fetched through our Context > this limits our alternatives in DeviceDisplayInfo. We could possibly use the > DisplayManager fetched from the application context (not the > activity/presentation context!) to fetch info about different displays. And then > store a displayID in each WindowAndroid (fetched through its display/activity > context). This display ID would be used to fetch the correct display for our > context. I think the best approach in many of these cases would to just hold a reference to the WindowAndroid. Then fetch the context as needed (handling the case where it has gone away, which in non-webview cases indicates a coding error BTW) and update what is necessary. But if we can't always get a WindowAndroid because of the screen apis, then I would try to keep a weak ref to the context in the devicedisplayinfo directly and handle it nicely if it goes away. If you keep a weak ref to only the context, then you'd need to refetch any dependent variables (services, resources, etc..) but hopefully we could cache the final values and discard the intermediary bits.
/me no owner review https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:14: namespace gfx { On 2015/11/03 18:14:27, sadrul wrote: > namespace ui screen_android.h header is still under gfx. This is not completely wrong since things like gfx::NativeView/NativeWindow are defined to ViewAndroid/WindowAndroid under ui/android. And they are under ui/android presumably because they talk through to java classes in jni. I guess ViewAndroid/WindowAndroid should live in like ui/gfx/android, but WindowAndroid.cc has more dependencies on ui/android/window_android_compositor.h and ui/android/window_android_observer.h, so it's the same problem. Given the current state of things, this is not that much worse. Not sure if owners actually agree with this. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:16: gfx::Display DisplayFromDeviceDisplayInfo( should this be in an anonymous namespace? https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:22: const gfx::Rect bounds_in_pixels = existing code but a bit weird, all display on android are at the origin, so they are like on top of each other? I guess nothing really cares about that fact https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/12/03 04:23:56, Ted C wrote: > On 2015/12/02 15:37:05, gsennton wrote: > > On 2015/11/25 19:35:52, Ted C wrote: > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > What is gfx::Display used for? How do they get notified of display > > changes? > > > > > > > In > > > > > particular, rotation seems to very quickly become inaccurate if it is > held > > > > onto > > > > > and not updated. > > > > > > > > > > Is Display supposed to be a snapshot in time or is it support to always > > > > reflect > > > > > the current state? If the latter, then it looks like it was wrong > before > > > and > > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > > > In all of the cases I have seen gfx::Display in use we simply grab some > > piece > > > of > > > > info from it (especially device scale factor / DPI), i.e. we use the > Display > > > as > > > > a snapshot (though, I guess that observation is not a guarantee that we > > > are/will > > > > always use it in that way..). > > > > > > > > I don't quite follow how it would be more wrong (if we hold on to > Displays) > > > when > > > > dealing with an activity? > > > > > > I must say much of my skepticism is around my lack of knowledge of > > > what a Display represents in Android. > > > > > > Does it truly represent the screen dimensions and not the window > > > bounds/available draw area? In samsung multi window, the draw area > > > != the screen size and I don't know what this is supposed to be. > > > > So there are two different sizes that can be fetched from Display - the screen > > size (getRealSize) and the size of the application window (getSize). > > > > > Also, the fact that this is a snapshot in time is really really bad > > > for things like orientation. To me, we should not set orientation > > > via this at all or we need to make this actually track updates from > > > the display (outside the scope of this I suspect). > > > > I might be misunderstanding your comment here about orientation changes but > the > > idea would be to update the DeviceDisplayInfo by using > ScreenOrientationListener > > which listens to changes in orientation. > > I think the "easiest" thing would be to actually not create a snapshot of the > display > values at all but simply pull them as needed. But that requires owning a > context ref > and could potentially slow everything down as we are now generating these once > and reading the values are cheap. Maybe as an aspirational end goal after all the other stuff like this stupid SharedDeviceDisplayInfo is removed? > > > Right now that class assumes there is > > only one display, so it needs to be changed to listen to several displays (the > > biggest problem with this is making sure that the listeners are not dependent > on > > our activity/display context in any way, because then we can't unregister them > > if our context goes away). > https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/12/03 04:23:56, Ted C wrote: > On 2015/12/02 15:37:05, gsennton wrote: > > On 2015/11/25 19:35:52, Ted C wrote: > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > DeviceDisplayInfo seems very tied to the application context. If we > > change > > > it > > > > > to support holding onto a Context, it has the same memory leak > > possibilities > > > > of > > > > > WindowAndroid. > > > > > > > > Right, so the idea would be to no longer hold on to any Context in > > > > DeviceDisplayInfo. > > > > > > > > AFAICT there are two ways we could do this: > > > > > > > > 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can > fetch > > a > > > > WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is > > > tied > > > > to a certain display so we can fetch display metrics from that. > > > > > > > > 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the > > > > presentation context or the application context, shouldn't matter) and > pass > > > the > > > > display ID (associated with the presentation context) to DeviceDisplayInfo > > as > > > a > > > > reference to the display we are interested in - and then fetch display > info > > > > through the DisplayManager.getDisplay(displayID). > > > > > > Again, I don't know what the lifecycles of either WindowManager or > > > DisplayManager are to know whether they are actually better than > > > Context. Do they transitively hold onto a Context ref by any means > > > that we don't expect? > > > > > > If it does, then it could potentially causes leaks on webview if we > > > aren't clearing the references where appropriate. > > > > > > If the java side of WindowAndroid was in charge of creating display > > > infos, then it could just pass a weak ref to the java class and it > > > could use the same leaky behavior as WindowAndroid. > > > > > > If sievers@ and maybe boliu@ don't think this is an issue than I can > > > be convinced, but as this code is in areas I'm not familiar with I > > > have to operate on my standard level of skepticism. > > > > Looking at this again, I don't think we should hold references to any > > non-trivial Objects (fetched through our Context) beyond the Context lifetime > > (since it is unsustainable to keep track of which Objects are OK to leak and > we > > might even leak the Context itself through some chain of references). This > also > > means that we should probably not hold a reference to the display/activity > > context Resources in ResourceManager as we do since > > https://codereview.chromium.org/1419843002/. > > We should probably fetch non-display resources from the application context > > rather than the current context in that class. > > > > Since we shouldn't hold strong references to objects fetched through our > Context > > this limits our alternatives in DeviceDisplayInfo. We could possibly use the > > DisplayManager fetched from the application context (not the > > activity/presentation context!) to fetch info about different displays. And > then > > store a displayID in each WindowAndroid (fetched through its display/activity > > context). This display ID would be used to fetch the correct display for our > > context. > > I think the best approach in many of these cases would to just hold a reference > to the WindowAndroid. Then fetch the context as needed (handling the case > where it has gone away, which in non-webview cases indicates a coding error > BTW) and update what is necessary. +1 > > But if we can't always get a WindowAndroid because of the screen apis, I think you mean DeviceDisplayInfo instead of gfx::Screen here? DeviceDisplayInfo is an android type, so we probably pass in whatever we need. > then > I would try to keep a weak ref to the context in the devicedisplayinfo directly > and handle it nicely if it goes away. If you keep a weak ref to only the > context, > then you'd need to refetch any dependent variables (services, resources, etc..) > but hopefully we could cache the final values and discard the intermediary > bits. I think the only time the display values can change is on orientation change. So we can cache everything as long as we listen orientation change events. All this is getting a bit head of this patch :) https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() { On 2015/11/04 14:20:48, AKV wrote: > DCHECK(device_display_info_) ? or just make device_display_info_ const https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.h:24: int GetDisplayHeight() const; do this in a separate CL?
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:16: gfx::Display DisplayFromDeviceDisplayInfo( On 2015/12/08 08:21:54, boliu wrote: > should this be in an anonymous namespace? Yup, will update in next PS. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:22: const gfx::Rect bounds_in_pixels = On 2015/12/08 08:21:54, boliu wrote: > existing code but a bit weird, all display on android are at the origin, so they > are like on top of each other? > > I guess nothing really cares about that fact Up until N we haven't been able to move applications around anyway so the location of the 'Display' shouldn't matter? (if that is what you are referring to). https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/12/08 08:21:54, boliu wrote: > On 2015/12/03 04:23:56, Ted C wrote: > > On 2015/12/02 15:37:05, gsennton wrote: > > > On 2015/11/25 19:35:52, Ted C wrote: > > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > > What is gfx::Display used for? How do they get notified of display > > > changes? > > > > > > > > > In > > > > > > particular, rotation seems to very quickly become inaccurate if it is > > held > > > > > onto > > > > > > and not updated. > > > > > > > > > > > > Is Display supposed to be a snapshot in time or is it support to > always > > > > > reflect > > > > > > the current state? If the latter, then it looks like it was wrong > > before > > > > and > > > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > > > > > In all of the cases I have seen gfx::Display in use we simply grab some > > > piece > > > > of > > > > > info from it (especially device scale factor / DPI), i.e. we use the > > Display > > > > as > > > > > a snapshot (though, I guess that observation is not a guarantee that we > > > > are/will > > > > > always use it in that way..). > > > > > > > > > > I don't quite follow how it would be more wrong (if we hold on to > > Displays) > > > > when > > > > > dealing with an activity? > > > > > > > > I must say much of my skepticism is around my lack of knowledge of > > > > what a Display represents in Android. > > > > > > > > Does it truly represent the screen dimensions and not the window > > > > bounds/available draw area? In samsung multi window, the draw area > > > > != the screen size and I don't know what this is supposed to be. > > > > > > So there are two different sizes that can be fetched from Display - the > screen > > > size (getRealSize) and the size of the application window (getSize). > > > > > > > Also, the fact that this is a snapshot in time is really really bad > > > > for things like orientation. To me, we should not set orientation > > > > via this at all or we need to make this actually track updates from > > > > the display (outside the scope of this I suspect). > > > > > > I might be misunderstanding your comment here about orientation changes but > > the > > > idea would be to update the DeviceDisplayInfo by using > > ScreenOrientationListener > > > which listens to changes in orientation. > > > > I think the "easiest" thing would be to actually not create a snapshot of the > > display > > values at all but simply pull them as needed. But that requires owning a > > context ref > > and could potentially slow everything down as we are now generating these once > > and reading the values are cheap. > > Maybe as an aspirational end goal after all the other stuff like this stupid > SharedDeviceDisplayInfo is removed? > > > > > > Right now that class assumes there is > > > only one display, so it needs to be changed to listen to several displays > (the > > > biggest problem with this is making sure that the listeners are not > dependent > > on > > > our activity/display context in any way, because then we can't unregister > them > > > if our context goes away). > > > @Bo now I'm a bit lost here, what would be the aspirational goal? If I'm not mistaken, the whole idea here is to make as few jni calls / java object constructions/destructions as possible and I guess we wouldn't have the SharedDeviceDisplayInfo class if this wasn't a problem. https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:26: device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo()); On 2015/12/08 08:21:54, boliu wrote: > On 2015/12/03 04:23:56, Ted C wrote: > > On 2015/12/02 15:37:05, gsennton wrote: > > > On 2015/11/25 19:35:52, Ted C wrote: > > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > > DeviceDisplayInfo seems very tied to the application context. If we > > > change > > > > it > > > > > > to support holding onto a Context, it has the same memory leak > > > possibilities > > > > > of > > > > > > WindowAndroid. > > > > > > > > > > Right, so the idea would be to no longer hold on to any Context in > > > > > DeviceDisplayInfo. > > > > > > > > > > AFAICT there are two ways we could do this: > > > > > > > > > > 1. Pass the (presentation) Context to DeviceDisplayInfo so that it can > > fetch > > > a > > > > > WindowManager which DeviceDisplayInfo can hold on to. The WindowManager > is > > > > tied > > > > > to a certain display so we can fetch display metrics from that. > > > > > > > > > > 2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through > the > > > > > presentation context or the application context, shouldn't matter) and > > pass > > > > the > > > > > display ID (associated with the presentation context) to > DeviceDisplayInfo > > > as > > > > a > > > > > reference to the display we are interested in - and then fetch display > > info > > > > > through the DisplayManager.getDisplay(displayID). > > > > > > > > Again, I don't know what the lifecycles of either WindowManager or > > > > DisplayManager are to know whether they are actually better than > > > > Context. Do they transitively hold onto a Context ref by any means > > > > that we don't expect? > > > > > > > > If it does, then it could potentially causes leaks on webview if we > > > > aren't clearing the references where appropriate. > > > > > > > > If the java side of WindowAndroid was in charge of creating display > > > > infos, then it could just pass a weak ref to the java class and it > > > > could use the same leaky behavior as WindowAndroid. > > > > > > > > If sievers@ and maybe boliu@ don't think this is an issue than I can > > > > be convinced, but as this code is in areas I'm not familiar with I > > > > have to operate on my standard level of skepticism. > > > > > > Looking at this again, I don't think we should hold references to any > > > non-trivial Objects (fetched through our Context) beyond the Context > lifetime > > > (since it is unsustainable to keep track of which Objects are OK to leak and > > we > > > might even leak the Context itself through some chain of references). This > > also > > > means that we should probably not hold a reference to the display/activity > > > context Resources in ResourceManager as we do since > > > https://codereview.chromium.org/1419843002/. > > > We should probably fetch non-display resources from the application context > > > rather than the current context in that class. > > > > > > Since we shouldn't hold strong references to objects fetched through our > > Context > > > this limits our alternatives in DeviceDisplayInfo. We could possibly use the > > > DisplayManager fetched from the application context (not the > > > activity/presentation context!) to fetch info about different displays. And > > then > > > store a displayID in each WindowAndroid (fetched through its > display/activity > > > context). This display ID would be used to fetch the correct display for our > > > context. > > > > I think the best approach in many of these cases would to just hold a > reference > > to the WindowAndroid. Then fetch the context as needed (handling the case > > where it has gone away, which in non-webview cases indicates a coding error > > BTW) and update what is necessary. > > +1 > > > > > But if we can't always get a WindowAndroid because of the screen apis, > > I think you mean DeviceDisplayInfo instead of gfx::Screen here? > DeviceDisplayInfo is an android type, so we probably pass in whatever we need. > > > then > > I would try to keep a weak ref to the context in the devicedisplayinfo > directly > > and handle it nicely if it goes away. If you keep a weak ref to only the > > context, > > then you'd need to refetch any dependent variables (services, resources, > etc..) > > but hopefully we could cache the final values and discard the intermediary > > bits. > > I think the only time the display values can change is on orientation change. So > we can cache everything as long as we listen orientation change events. > > All this is getting a bit head of this patch :) Yeah, the idea with this patch is to only add the interface for getting DeviceDisplayInfo from WindowAndroid, so that we can then focus on the DeviceDisplayInfo implementation changes in the next patch (that implementation includes changing the values cached in DeviceDisplayInfo on orientation changes). https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() { On 2015/12/08 08:21:54, boliu wrote: > On 2015/11/04 14:20:48, AKV wrote: > > DCHECK(device_display_info_) ? > > or just make device_display_info_ const I don't think we want to make it const since it should be updated on orientation changes. https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.h:76: const gfx::DeviceDisplayInfo& GetDeviceDisplayInfo(); On 2015/11/03 18:14:27, sadrul wrote: > const method too? Ye, will update in next PS. https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.h:24: int GetDisplayHeight() const; On 2015/12/08 08:21:54, boliu wrote: > do this in a separate CL? Added https://codereview.chromium.org/1505993009 Will remove these changes in next PS.
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:22: const gfx::Rect bounds_in_pixels = On 2015/12/10 12:29:54, gsennton wrote: > On 2015/12/08 08:21:54, boliu wrote: > > existing code but a bit weird, all display on android are at the origin, so > they > > are like on top of each other? > > > > I guess nothing really cares about that fact > > Up until N we haven't been able to move applications around anyway so the > location of the 'Display' shouldn't matter? (if that is what you are referring > to). Think of a desktop with two monitors, each 1000x1000 (for simplicity), placed side-by-side, forming one virtual desktop. gfx::Display here corresponds to the physical monitors, so you'd have two of them, one at 0,0 1000x1000, and one at 1000,0 1000x1000 FWIW gfx::Screen corresponds to the "virtual desktop" part, at least on aura desktop. So if android doesn't supporting moving windows between displays, then that's like having one Screen per Display? Then 0,0 is actually correct. Either way, don't need to worry about it too much until it becomes a problem I think. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/12/10 12:29:54, gsennton wrote: > On 2015/12/08 08:21:54, boliu wrote: > > On 2015/12/03 04:23:56, Ted C wrote: > > > On 2015/12/02 15:37:05, gsennton wrote: > > > > On 2015/11/25 19:35:52, Ted C wrote: > > > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > > > What is gfx::Display used for? How do they get notified of display > > > > changes? > > > > > > > > > > > In > > > > > > > particular, rotation seems to very quickly become inaccurate if it > is > > > held > > > > > > onto > > > > > > > and not updated. > > > > > > > > > > > > > > Is Display supposed to be a snapshot in time or is it support to > > always > > > > > > reflect > > > > > > > the current state? If the latter, then it looks like it was wrong > > > before > > > > > and > > > > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > > > > > > > In all of the cases I have seen gfx::Display in use we simply grab > some > > > > piece > > > > > of > > > > > > info from it (especially device scale factor / DPI), i.e. we use the > > > Display > > > > > as > > > > > > a snapshot (though, I guess that observation is not a guarantee that > we > > > > > are/will > > > > > > always use it in that way..). > > > > > > > > > > > > I don't quite follow how it would be more wrong (if we hold on to > > > Displays) > > > > > when > > > > > > dealing with an activity? > > > > > > > > > > I must say much of my skepticism is around my lack of knowledge of > > > > > what a Display represents in Android. > > > > > > > > > > Does it truly represent the screen dimensions and not the window > > > > > bounds/available draw area? In samsung multi window, the draw area > > > > > != the screen size and I don't know what this is supposed to be. > > > > > > > > So there are two different sizes that can be fetched from Display - the > > screen > > > > size (getRealSize) and the size of the application window (getSize). > > > > > > > > > Also, the fact that this is a snapshot in time is really really bad > > > > > for things like orientation. To me, we should not set orientation > > > > > via this at all or we need to make this actually track updates from > > > > > the display (outside the scope of this I suspect). > > > > > > > > I might be misunderstanding your comment here about orientation changes > but > > > the > > > > idea would be to update the DeviceDisplayInfo by using > > > ScreenOrientationListener > > > > which listens to changes in orientation. > > > > > > I think the "easiest" thing would be to actually not create a snapshot of > the > > > display > > > values at all but simply pull them as needed. But that requires owning a > > > context ref > > > and could potentially slow everything down as we are now generating these > once > > > and reading the values are cheap. > > > > Maybe as an aspirational end goal after all the other stuff like this stupid > > SharedDeviceDisplayInfo is removed? > > > > > > > > > Right now that class assumes there is > > > > only one display, so it needs to be changed to listen to several displays > > (the > > > > biggest problem with this is making sure that the listeners are not > > dependent > > > on > > > > our activity/display context in any way, because then we can't unregister > > them > > > > if our context goes away). > > > > > > > @Bo now I'm a bit lost here, what would be the aspirational goal? > If I'm not mistaken, the whole idea here is to make as few jni calls / java > object constructions/destructions as possible and I guess we wouldn't have the > SharedDeviceDisplayInfo class if this wasn't a problem. Tradeoffs.. perf comes after correctness and having no leaks, and probably on-par with code simplicity. I doubt we actually poll this enough times for the jni to matter, but we'll have to measure that I guess Either way, this part is out of scope for this CL, no? https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() { On 2015/12/10 12:29:54, gsennton wrote: > On 2015/12/08 08:21:54, boliu wrote: > > On 2015/11/04 14:20:48, AKV wrote: > > > DCHECK(device_display_info_) ? > > > > or just make device_display_info_ const > > I don't think we want to make it const since it should be updated on orientation > changes. Pointer can be const, pointee (?) don't need to be const (unless DeviceDisplayInfo is immutable)
I'd say this looks fine to me at this point. I think sievers should take another pass as this in general is more his area than mine. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:14: namespace gfx { On 2015/12/08 08:21:54, boliu wrote: > On 2015/11/03 18:14:27, sadrul wrote: > > namespace ui > > screen_android.h header is still under gfx. This is not completely wrong since > things like gfx::NativeView/NativeWindow are defined to > ViewAndroid/WindowAndroid under ui/android. And they are under ui/android > presumably because they talk through to java classes in jni. > > I guess ViewAndroid/WindowAndroid should live in like ui/gfx/android, but > WindowAndroid.cc has more dependencies on ui/android/window_android_compositor.h > and ui/android/window_android_observer.h, so it's the same problem. > > Given the current state of things, this is not that much worse. Not sure if > owners actually agree with this. I would follow what other files of similar nature due. I don't honestly care.
So I've fixed the minor stuff, still need to find a way of setting the screen instance using SetScreenInstance or somehow keeping screen_android under gfx/ (clank crashes at start-up with this CL). https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:16: gfx::Display DisplayFromDeviceDisplayInfo( On 2015/12/10 12:29:54, gsennton wrote: > On 2015/12/08 08:21:54, boliu wrote: > > should this be in an anonymous namespace? > > Yup, will update in next PS. Done. https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:56: return DisplayFromDeviceDisplayInfo(device_info); On 2015/12/10 16:38:20, boliu wrote: > On 2015/12/10 12:29:54, gsennton wrote: > > On 2015/12/08 08:21:54, boliu wrote: > > > On 2015/12/03 04:23:56, Ted C wrote: > > > > On 2015/12/02 15:37:05, gsennton wrote: > > > > > On 2015/11/25 19:35:52, Ted C wrote: > > > > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > > > > What is gfx::Display used for? How do they get notified of > display > > > > > changes? > > > > > > > > > > > > > In > > > > > > > > particular, rotation seems to very quickly become inaccurate if it > > is > > > > held > > > > > > > onto > > > > > > > > and not updated. > > > > > > > > > > > > > > > > Is Display supposed to be a snapshot in time or is it support to > > > always > > > > > > > reflect > > > > > > > > the current state? If the latter, then it looks like it was wrong > > > > before > > > > > > and > > > > > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > > > > > > > > > In all of the cases I have seen gfx::Display in use we simply grab > > some > > > > > piece > > > > > > of > > > > > > > info from it (especially device scale factor / DPI), i.e. we use the > > > > Display > > > > > > as > > > > > > > a snapshot (though, I guess that observation is not a guarantee that > > we > > > > > > are/will > > > > > > > always use it in that way..). > > > > > > > > > > > > > > I don't quite follow how it would be more wrong (if we hold on to > > > > Displays) > > > > > > when > > > > > > > dealing with an activity? > > > > > > > > > > > > I must say much of my skepticism is around my lack of knowledge of > > > > > > what a Display represents in Android. > > > > > > > > > > > > Does it truly represent the screen dimensions and not the window > > > > > > bounds/available draw area? In samsung multi window, the draw area > > > > > > != the screen size and I don't know what this is supposed to be. > > > > > > > > > > So there are two different sizes that can be fetched from Display - the > > > screen > > > > > size (getRealSize) and the size of the application window (getSize). > > > > > > > > > > > Also, the fact that this is a snapshot in time is really really bad > > > > > > for things like orientation. To me, we should not set orientation > > > > > > via this at all or we need to make this actually track updates from > > > > > > the display (outside the scope of this I suspect). > > > > > > > > > > I might be misunderstanding your comment here about orientation changes > > but > > > > the > > > > > idea would be to update the DeviceDisplayInfo by using > > > > ScreenOrientationListener > > > > > which listens to changes in orientation. > > > > > > > > I think the "easiest" thing would be to actually not create a snapshot of > > the > > > > display > > > > values at all but simply pull them as needed. But that requires owning a > > > > context ref > > > > and could potentially slow everything down as we are now generating these > > once > > > > and reading the values are cheap. > > > > > > Maybe as an aspirational end goal after all the other stuff like this stupid > > > SharedDeviceDisplayInfo is removed? > > > > > > > > > > > > Right now that class assumes there is > > > > > only one display, so it needs to be changed to listen to several > displays > > > (the > > > > > biggest problem with this is making sure that the listeners are not > > > dependent > > > > on > > > > > our activity/display context in any way, because then we can't > unregister > > > them > > > > > if our context goes away). > > > > > > > > > > > @Bo now I'm a bit lost here, what would be the aspirational goal? > > If I'm not mistaken, the whole idea here is to make as few jni calls / java > > object constructions/destructions as possible and I guess we wouldn't have the > > SharedDeviceDisplayInfo class if this wasn't a problem. > > Tradeoffs.. perf comes after correctness and having no leaks, and probably > on-par with code simplicity. I doubt we actually poll this enough times for the > jni to matter, but we'll have to measure that I guess Alright > Either way, this part is out of scope for this CL, no? Yeah this needs to be considered in the follow-up. The worrying part is that I haven't touched any places under chrome/ using the default DeviceDisplayInfo ctor so just removing the SharedDeviceDisplayInfo would make each of those calls create a new DeviceDisplayInfo (while the call-sites I have changed would at least cache the DeviceDisplayInfo in WindowAndroid). https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() { On 2015/12/10 16:38:20, boliu wrote: > On 2015/12/10 12:29:54, gsennton wrote: > > On 2015/12/08 08:21:54, boliu wrote: > > > On 2015/11/04 14:20:48, AKV wrote: > > > > DCHECK(device_display_info_) ? > > > > > > or just make device_display_info_ const > > > > I don't think we want to make it const since it should be updated on > orientation > > changes. > > Pointer can be const, pointee (?) don't need to be const (unless > DeviceDisplayInfo is immutable) Done. https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... ui/android/window_android.h:76: const gfx::DeviceDisplayInfo& GetDeviceDisplayInfo(); On 2015/12/10 12:29:54, gsennton wrote: > On 2015/11/03 18:14:27, sadrul wrote: > > const method too? > > Ye, will update in next PS. Done. https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.h:24: int GetDisplayHeight() const; On 2015/12/10 12:29:54, gsennton wrote: > On 2015/12/08 08:21:54, boliu wrote: > > do this in a separate CL? > > Added > https://codereview.chromium.org/1505993009 > > Will remove these changes in next PS. Done. https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.cc File ui/gfx/screen_android.cc (left): https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.c... ui/gfx/screen_android.cc:83: return new ScreenAndroid; On 2015/11/03 18:14:27, sadrul wrote: > You still need to define CreateNativeScreen() in the gfx component, right? You > can maybe reuse the one aura has (screen_aura.cc) Right, I've added the stub implementation but I am not yet setting the Screen implementation anywhere with SetScreenInstance.
On 2015/12/14 18:10:32, gsennton wrote: > So I've fixed the minor stuff, still need to find a way of setting the screen > instance using SetScreenInstance or somehow keeping screen_android under gfx/ > (clank crashes at start-up with this CL). How/where do other ports do this? sievers@, this seems up your wheelhouse (or more so than mine), do you have any preferences? > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... > File ui/android/screen_android.cc (right): > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... > ui/android/screen_android.cc:16: gfx::Display DisplayFromDeviceDisplayInfo( > On 2015/12/10 12:29:54, gsennton wrote: > > On 2015/12/08 08:21:54, boliu wrote: > > > should this be in an anonymous namespace? > > > > Yup, will update in next PS. > > Done. > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... > ui/android/screen_android.cc:56: return > DisplayFromDeviceDisplayInfo(device_info); > On 2015/12/10 16:38:20, boliu wrote: > > On 2015/12/10 12:29:54, gsennton wrote: > > > On 2015/12/08 08:21:54, boliu wrote: > > > > On 2015/12/03 04:23:56, Ted C wrote: > > > > > On 2015/12/02 15:37:05, gsennton wrote: > > > > > > On 2015/11/25 19:35:52, Ted C wrote: > > > > > > > On 2015/11/06 16:52:14, gsennton wrote: > > > > > > > > On 2015/11/05 23:15:58, Ted C wrote: > > > > > > > > > What is gfx::Display used for? How do they get notified of > > display > > > > > > changes? > > > > > > > > > > > > > > > In > > > > > > > > > particular, rotation seems to very quickly become inaccurate if > it > > > is > > > > > held > > > > > > > > onto > > > > > > > > > and not updated. > > > > > > > > > > > > > > > > > > Is Display supposed to be a snapshot in time or is it support to > > > > always > > > > > > > > reflect > > > > > > > > > the current state? If the latter, then it looks like it was > wrong > > > > > before > > > > > > > and > > > > > > > > > likely to be "more wrong" if we are dealing with an activity? > > > > > > > > > > > > > > > > In all of the cases I have seen gfx::Display in use we simply grab > > > some > > > > > > piece > > > > > > > of > > > > > > > > info from it (especially device scale factor / DPI), i.e. we use > the > > > > > Display > > > > > > > as > > > > > > > > a snapshot (though, I guess that observation is not a guarantee > that > > > we > > > > > > > are/will > > > > > > > > always use it in that way..). > > > > > > > > > > > > > > > > I don't quite follow how it would be more wrong (if we hold on to > > > > > Displays) > > > > > > > when > > > > > > > > dealing with an activity? > > > > > > > > > > > > > > I must say much of my skepticism is around my lack of knowledge of > > > > > > > what a Display represents in Android. > > > > > > > > > > > > > > Does it truly represent the screen dimensions and not the window > > > > > > > bounds/available draw area? In samsung multi window, the draw area > > > > > > > != the screen size and I don't know what this is supposed to be. > > > > > > > > > > > > So there are two different sizes that can be fetched from Display - > the > > > > screen > > > > > > size (getRealSize) and the size of the application window (getSize). > > > > > > > > > > > > > Also, the fact that this is a snapshot in time is really really bad > > > > > > > for things like orientation. To me, we should not set orientation > > > > > > > via this at all or we need to make this actually track updates from > > > > > > > the display (outside the scope of this I suspect). > > > > > > > > > > > > I might be misunderstanding your comment here about orientation > changes > > > but > > > > > the > > > > > > idea would be to update the DeviceDisplayInfo by using > > > > > ScreenOrientationListener > > > > > > which listens to changes in orientation. > > > > > > > > > > I think the "easiest" thing would be to actually not create a snapshot > of > > > the > > > > > display > > > > > values at all but simply pull them as needed. But that requires owning > a > > > > > context ref > > > > > and could potentially slow everything down as we are now generating > these > > > once > > > > > and reading the values are cheap. > > > > > > > > Maybe as an aspirational end goal after all the other stuff like this > stupid > > > > SharedDeviceDisplayInfo is removed? > > > > > > > > > > > > > > > Right now that class assumes there is > > > > > > only one display, so it needs to be changed to listen to several > > displays > > > > (the > > > > > > biggest problem with this is making sure that the listeners are not > > > > dependent > > > > > on > > > > > > our activity/display context in any way, because then we can't > > unregister > > > > them > > > > > > if our context goes away). > > > > > > > > > > > > > > > @Bo now I'm a bit lost here, what would be the aspirational goal? > > > If I'm not mistaken, the whole idea here is to make as few jni calls / java > > > object constructions/destructions as possible and I guess we wouldn't have > the > > > SharedDeviceDisplayInfo class if this wasn't a problem. > > > > Tradeoffs.. perf comes after correctness and having no leaks, and probably > > on-par with code simplicity. I doubt we actually poll this enough times for > the > > jni to matter, but we'll have to measure that I guess > > Alright > > > > Either way, this part is out of scope for this CL, no? > > Yeah this needs to be considered in the follow-up. The worrying part is that I > haven't touched any places under chrome/ using the default DeviceDisplayInfo > ctor so just removing the SharedDeviceDisplayInfo would make each of those calls > create a new DeviceDisplayInfo (while the call-sites I have changed would at > least cache the DeviceDisplayInfo in WindowAndroid). > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... > File ui/android/window_android.cc (right): > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... > ui/android/window_android.cc:139: const gfx::DeviceDisplayInfo& > WindowAndroid::GetDeviceDisplayInfo() { > On 2015/12/10 16:38:20, boliu wrote: > > On 2015/12/10 12:29:54, gsennton wrote: > > > On 2015/12/08 08:21:54, boliu wrote: > > > > On 2015/11/04 14:20:48, AKV wrote: > > > > > DCHECK(device_display_info_) ? > > > > > > > > or just make device_display_info_ const > > > > > > I don't think we want to make it const since it should be updated on > > orientation > > > changes. > > > > Pointer can be const, pointee (?) don't need to be const (unless > > DeviceDisplayInfo is immutable) > > Done. > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... > File ui/android/window_android.h (right): > > https://codereview.chromium.org/1409833004/diff/40001/ui/android/window_andro... > ui/android/window_android.h:76: const gfx::DeviceDisplayInfo& > GetDeviceDisplayInfo(); > On 2015/12/10 12:29:54, gsennton wrote: > > On 2015/11/03 18:14:27, sadrul wrote: > > > const method too? > > > > Ye, will update in next PS. > > Done. > > https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... > File ui/gfx/android/device_display_info.h (right): > > https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/android/device_d... > ui/gfx/android/device_display_info.h:24: int GetDisplayHeight() const; > On 2015/12/10 12:29:54, gsennton wrote: > > On 2015/12/08 08:21:54, boliu wrote: > > > do this in a separate CL? > > > > Added > > https://codereview.chromium.org/1505993009 > > > > Will remove these changes in next PS. > > Done. > > https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.cc > File ui/gfx/screen_android.cc (left): > > https://codereview.chromium.org/1409833004/diff/40001/ui/gfx/screen_android.c... > ui/gfx/screen_android.cc:83: return new ScreenAndroid; > On 2015/11/03 18:14:27, sadrul wrote: > > You still need to define CreateNativeScreen() in the gfx component, right? You > > can maybe reuse the one aura has (screen_aura.cc) > > Right, I've added the stub implementation but I am not yet setting the Screen > implementation anywhere with SetScreenInstance.
On 2015/12/15 00:27:52, Ted C wrote: > On 2015/12/14 18:10:32, gsennton wrote: > > So I've fixed the minor stuff, still need to find a way of setting the screen > > instance using SetScreenInstance or somehow keeping screen_android under gfx/ > > (clank crashes at start-up with this CL). > > How/where do other ports do this? AFAICT we only have a similar solution for aura and the screen seems to be set at different places for different shells/browsers: https://code.google.com/p/chromium/codesearch#search/&q=setscreeninstance%20a... > sievers@, this seems up your wheelhouse (or more so than mine), do you have > any preferences?
Description was changed from ========== Add DeviceDisplayInfo getter in WindowAndroid. To support external displays (i.e. use the metrics of those displays) we need to fetch Context specific display metrics. This will be done through WindowAndroid and in this CL we add the interface for doing so and also start using that interface. The actual functionality for using the metrics of the current context/display rather than the application context will be added in a follow-up. BUG=310763 ========== to ========== Add DeviceDisplayInfo getter in WindowAndroid. To support external displays (i.e. use the metrics of those displays) we need to fetch Context specific display metrics. This will be done through WindowAndroid and in this CL we add the interface for doing so and also start using that interface. The actual functionality for using the metrics of the current context/display rather than the application context will be added in a follow-up. BUG=310763 ==========
gsennton@chromium.org changed reviewers: - sadrul@chromium.org
gsennton@chromium.org changed reviewers: + sky@chromium.org
Hey everyone, sorry for putting this CL on hold, I have been working on more urgent issues :/ I've removed sadrul@ (since he is OOO for quite a while) and added sky@: sky@ could you ptal at the change in chrome/browser (chrome/browser/chrome_browser_main_android.cc) and ui/gfx/screen_android.cc (we are moving the implementation of screen_android from ui/gfx to ui/android so that it can make use of view_android/window_android) also, Bo, I need you for android_webview/ now :) tedchoc@ for ui/android/ I'm not sure whether content/browser/ needs a reevaluation, between PS4 and PS6 there should only be rebase changes in there (right now they are covered by sievers@'s L G T M).
https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:17: gfx::Display DisplayFromDeviceDisplayInfo( If ScreenAndroid only ever returns a single display, how could this return something else? https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:49: return NULL; nullptr (here and 54). https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:66: } else { nit: no else after a return (see style guide).
https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. no (c) and update year. https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c)
I'll approve once UI owners are happy. Also wondering, can DeviceDisplayInfo constructor be made private now, and make WindowAndroid a friend? Would prevent any new code using DeviceDisplayInfo directly. https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:17: gfx::Display DisplayFromDeviceDisplayInfo( On 2016/02/24 19:18:48, sky wrote: > If ScreenAndroid only ever returns a single display, how could this return > something else? Screen implementation would now needs to listen to events for when extra displays are added and removed. I hope android provides these events? That can come from a follow up CL though, like the missing bits in WindowAndroid. https://codereview.chromium.org/1409833004/diff/100001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/window_andr... ui/android/window_android.h:93: // TODO(gsennton) Update display info on screen orientation updates. nit: TODO(gsennton): ... And other places
'Also wondering, can DeviceDisplayInfo constructor be made private now, and make WindowAndroid a friend? Would prevent any new code using DeviceDisplayInfo directly.' We are still using the default DeviceDisplayInfo ctor to fetch the shared devicedisplayinfo from some places under chrome/ so as long as those calls still exist we won't be able to make the DeviceDisplayInfo ctor private. I've focused on fixing this for WebView so far.. though there doesn't seem to be that many places left (a quick search showed only 2) so it *might* be possible to fix this quite easily for chrome as well :) https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/02/24 19:19:08, sky wrote: > no (c) and update year. Done. https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:17: gfx::Display DisplayFromDeviceDisplayInfo( On 2016/02/25 05:33:36, boliu wrote: > On 2016/02/24 19:18:48, sky wrote: > > If ScreenAndroid only ever returns a single display, how could this return > > something else? Yeah, so at the moment we only support one display, but the purpose of this CL is to provide an API/framework to use when we support other displays - the idea is that if you provide a NativeView = ViewAndroid which holds a WindowAndroid (i.e. when you call GetDisplayNearestWindow). That View/WindowAndroid can target an external display (through the Context held by WindowAndroid). Also, GetPrimaryDisplay could still target the primary display by just statically fetching display metrics through the application Context. I created this standalone CL to add this API (and to change several files to use the API) because the initial CL adding both this API and the implementation for fetching display metrics from the correct display was huge and gross and practically impossible to review//change/move forward :/ > Screen implementation would now needs to listen to events for when extra > displays are added and removed. I hope android provides these events? > > That can come from a follow up CL though, like the missing bits in > WindowAndroid. Would it really? If each WindowAndroid takes care of fetching its own display we wouldn't need to keep track of all displays? (though, I'm not yet sure whether we want to fetch display metrics specifically for each WindowAndroid or just try to keep some global list of displays and query that from WindowAndroid instead, so we might want to keep track of the available displays even if we end up not strictly needing to). Anyway, in case we do want to keep track of each display, the following is copy-pasted from the Android documentation: /** * Listens for changes in available display devices. */ public interface DisplayListener { /** * Called whenever a logical display has been added to the system. * Use {@link DisplayManager#getDisplay} to get more information about * the display. * * @param displayId The id of the logical display that was added. */ void onDisplayAdded(int displayId); /** * Called whenever a logical display has been removed from the system. * * @param displayId The id of the logical display that was removed. */ void onDisplayRemoved(int displayId); /** * Called whenever the properties of a logical display have changed. * * @param displayId The id of the logical display that changed. */ void onDisplayChanged(int displayId); } https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:49: return NULL; On 2016/02/24 19:18:48, sky wrote: > nullptr (here and 54). Done. https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:66: } else { On 2016/02/24 19:18:48, sky wrote: > nit: no else after a return (see style guide). Done. https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/02/24 19:19:08, sky wrote: > no (c) Done. https://codereview.chromium.org/1409833004/diff/100001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/window_andr... ui/android/window_android.h:93: // TODO(gsennton) Update display info on screen orientation updates. On 2016/02/25 05:33:36, boliu wrote: > nit: TODO(gsennton): ... > > And other places Done.
https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/100001/ui/android/screen_andr... ui/android/screen_android.cc:17: gfx::Display DisplayFromDeviceDisplayInfo( On 2016/02/25 17:08:04, gsennton wrote: > On 2016/02/25 05:33:36, boliu wrote: > > On 2016/02/24 19:18:48, sky wrote: > > > If ScreenAndroid only ever returns a single display, how could this return > > > something else? > > Yeah, so at the moment we only support one display, but the purpose of this CL > is to provide an API/framework to use when we support other displays - the idea > is that if you provide a NativeView = ViewAndroid which holds a WindowAndroid > (i.e. when you call GetDisplayNearestWindow). That View/WindowAndroid can target > an external display (through the Context held by WindowAndroid). Also, > GetPrimaryDisplay could still target the primary display by just statically > fetching display metrics through the application Context. > > I created this standalone CL to add this API (and to change several files to use > the API) because the initial CL adding both this API and the implementation for > fetching display metrics from the correct display was huge and gross and > practically impossible to review//change/move forward :/ > > > Screen implementation would now needs to listen to events for when extra > > displays are added and removed. I hope android provides these events? > > > > That can come from a follow up CL though, like the missing bits in > > WindowAndroid. > > Would it really? If each WindowAndroid takes care of fetching its own display we > wouldn't need to keep track of all displays? (though, I'm not yet sure whether > we want to fetch display metrics specifically for each WindowAndroid or just try > to keep some global list of displays and query that from WindowAndroid instead, > so we might want to keep track of the available displays even if we end up not > strictly needing to). Current implementation of ScreenAndroid is just wrong if there are multiple displays: GetNumDisplays should not just return 1 GetAllDisplays should return more than 1 display GetDisplayNearestPoint doesn't really make sense on android etc etc Either you have to say these methods are not supported on Android and they must be never called (enforced by NOTIMPLEMENTED or NOTREACHED), or they have to be implemented correctly. I don't know which makes more sense. > Anyway, in case we do want to keep track of each display, the following is > copy-pasted from the Android documentation: > > /** > * Listens for changes in available display devices. > */ > public interface DisplayListener { > /** > * Called whenever a logical display has been added to the system. > * Use {@link DisplayManager#getDisplay} to get more information about > * the display. > * > * @param displayId The id of the logical display that was added. > */ > void onDisplayAdded(int displayId); > > /** > * Called whenever a logical display has been removed from the system. > * > * @param displayId The id of the logical display that was removed. > */ > void onDisplayRemoved(int displayId); > > /** > * Called whenever the properties of a logical display have changed. > * > * @param displayId The id of the logical display that changed. > */ > void onDisplayChanged(int displayId); > }
https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1537: if (content_view_core_) { I'm wondering if it's worth plumbing all this info through all the static functions and callbacks here, because... well it's all static... so if there are multiple displays somebody is using the wrong dimensions. We also don't want to create a context per display just for this. Maybe just keep going with GetDefaultScreenInfo() here? We can discuss separately what this really affects and how in a multi-screen world. I think it's just the UI resource uploads. (And maybe '*3' above is even a bit high'. https://codereview.chromium.org/1409833004/diff/120001/ui/gfx/screen_android.cc File ui/gfx/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/ui/gfx/screen_android.... ui/gfx/screen_android.cc:12: NOTREACHED() << "Implementation should be installed at higher level."; why do we need this file? just odd to have screen_android.cc twice. that being said: why did you move it anyways? seems more obvious to have it next to the other implementations.
https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1537: if (content_view_core_) { On 2016/02/27 01:11:52, sievers wrote: > I'm wondering if it's worth plumbing all this info through all the static > functions and callbacks here, because... well it's all static... so if there are > multiple displays somebody is using the wrong dimensions. It's also used to determine stuff like screen density. > > We also don't want to create a context per display just for this. > Maybe just keep going with GetDefaultScreenInfo() here? I assume you mean CreateContext3D? I don't see how this CL creates more Context3D now? The work is to add multi display support for android webview, not chrome.. > > We can discuss separately what this really affects and how in a multi-screen > world. I think it's just the UI resource uploads. (And maybe '*3' above is even > a bit high'. https://codereview.chromium.org/1409833004/diff/120001/ui/gfx/screen_android.cc File ui/gfx/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/ui/gfx/screen_android.... ui/gfx/screen_android.cc:12: NOTREACHED() << "Implementation should be installed at higher level."; On 2016/02/27 01:11:52, sievers wrote: > why do we need this file? just odd to have screen_android.cc twice. > > that being said: why did you move it anyways? seems more obvious to have it next > to the other implementations. See this discussion: https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro...
https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1409833004/diff/40001/ui/android/screen_andro... ui/android/screen_android.cc:14: namespace gfx { On 2015/12/08 08:21:54, boliu wrote: > On 2015/11/03 18:14:27, sadrul wrote: > > namespace ui > > screen_android.h header is still under gfx. This is not completely wrong since > things like gfx::NativeView/NativeWindow are defined to > ViewAndroid/WindowAndroid under ui/android. And they are under ui/android > presumably because they talk through to java classes in jni. > > I guess ViewAndroid/WindowAndroid should live in like ui/gfx/android, but > WindowAndroid.cc has more dependencies on ui/android/window_android_compositor.h > and ui/android/window_android_observer.h, so it's the same problem. > > Given the current state of things, this is not that much worse. Not sure if > owners actually agree with this. Note that ui/android/java containts java code for all of ui/... https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1537: if (content_view_core_) { On 2016/02/27 01:30:56, boliu wrote: > On 2016/02/27 01:11:52, sievers wrote: > > I'm wondering if it's worth plumbing all this info through all the static > > functions and callbacks here, because... well it's all static... so if there > are > > multiple displays somebody is using the wrong dimensions. > > It's also used to determine stuff like screen density. > > > > > We also don't want to create a context per display just for this. > > Maybe just keep going with GetDefaultScreenInfo() here? > > I assume you mean CreateContext3D? I don't see how this CL creates more > Context3D now? The work is to add multi display support for android webview, not > chrome.. > > > The point was: If this is for multi-display, then there's no need to plumb this through to the static shared context (and I don't think we want to create one context per display either). And I'd just leave this file untouched as far as possible and use the default screen info as before.
https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1409833004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1537: if (content_view_core_) { On 2016/02/29 18:16:03, sievers wrote: > On 2016/02/27 01:30:56, boliu wrote: > > On 2016/02/27 01:11:52, sievers wrote: > > > I'm wondering if it's worth plumbing all this info through all the static > > > functions and callbacks here, because... well it's all static... so if there > > are > > > multiple displays somebody is using the wrong dimensions. > > > > It's also used to determine stuff like screen density. > > > > > > > > We also don't want to create a context per display just for this. > > > Maybe just keep going with GetDefaultScreenInfo() here? > > > > I assume you mean CreateContext3D? I don't see how this CL creates more > > Context3D now? The work is to add multi display support for android webview, > not > > chrome.. > > > > > > > > The point was: If this is for multi-display, then there's no need to plumb this > through to the static shared context (and I don't think we want to create one > context per display either). And I'd just leave this file untouched as far as > possible and use the default screen info as before. I experimented a bit by setting different device scale factors at different points in the code and it seems the main point which actually makes a WebView on a Presentation, targeting a virtual display, look OK is RenderWidgetHostViewAndroid::GetScreenInfo (i.e. if I hardcode the scale factor in GetDefaultScreenInfo to be that of the primary display, even if all other locations use the correct value - the scale factor for the virtual display - then we display content in an incorrect way). :/
tedchoc@chromium.org changed reviewers: - tedchoc@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
