Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1144333004:
Make WebView work for external displays (over Presentations).
|
Created:
5 years, 6 months ago by gsennton Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, penghuang+watch_chromium.org, piman+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake WebView work for external displays (over Presentations).
On Android we have been using the application context to get information
about the current display which means that we always receive metrics for
the primary display. With the introduction of Presentations we can now
target other displays and thus need to use the Context of the
Presentation to fetch information about the current display.
BUG=310763
Patch Set 1 #
Total comments: 14
Patch Set 2 : Minor fixes according to boliu@'s comments #Patch Set 3 : Store DeviceDisplayInfo in WindowAndroid and pass context to DeviceDisplayInfo #Patch Set 4 : Move screen_android.cc from ui/gfx/ to ui/android/. #
Total comments: 13
Patch Set 5 : Use scoped_ptr to store DeviceDisplayInfo and make DeviceDisplayInfo getters const #
Total comments: 2
Patch Set 6 : Make ScreenOrientationListener context-based and git cl format #
Total comments: 17
Patch Set 7 : Add single global polling in ScreenOrientationListener #
Total comments: 1
Patch Set 8 : Use strong ref to context in ScreenOrientationListener and add display context to WindowAndroid #Patch Set 9 : Fix another context-use through making WindowAndroid::GetJavaContext public #
Total comments: 2
Patch Set 10 : Minor naming/comment fixes #
Total comments: 59
Patch Set 11 : Minor fixes according to boliu/jdduke's comments #
Total comments: 3
Messages
Total messages: 44 (4 generated)
gsennton@chromium.org changed reviewers: + boliu@chromium.org
Hi Bo, does this look like what we discussed? I haven't changed anything in the chrome/ directory, instead I just let the default constructor for (native) DeviceDisplayInfo pass the application context to the java side. I have not added any caching yet (we could have WindowAndroid own a cached DeviceDisplayInfo?).
Looks fine > I have not added any caching yet (we could have WindowAndroid own a cached DeviceDisplayInfo?). I assume WindowAndroid can't include DeviceDisplayInfo. https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:2050: // we can only use the primary display? Yes Although should double check that any caller of this should be updated to use WindowAndroid instead https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:321: // TODO if we cache DeviceDisplayInfo then update the caches here Hmm, this means native device_display_info shouldn't cache any results, and should always go through jni to ask the java side. Otherwise anything caching a device_display_info would get the wrong value after rotation? (Gosh this was a big hack before) https://codereview.chromium.org/1144333004/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:488: public WeakReference<Context> getCurrentContext() { What does "current" mean here? Just getContext? https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.cc:27: DeviceDisplayInfo::DeviceDisplayInfo() { // TODO ugly? c++11 allows just calling the other constructor now I think https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.h:22: explicit DeviceDisplayInfo(jobject windowAndroid); Can this be the native WindowAndroid type instead?
> > I have not added any caching yet (we could have WindowAndroid own a cached > DeviceDisplayInfo?). > > I assume WindowAndroid can't include DeviceDisplayInfo. I did not see any problems when trying to including DeviceDisplayInfo from WindowAndroid (but the other way around did not work). Do we have access to some kind of shared_ptr or similar to hold the cached copy? https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:2050: // we can only use the primary display? On 2015/06/01 15:30:18, boliu wrote: > Yes > > Although should double check that any caller of this should be updated to use > WindowAndroid instead This is called from two places: 1. GetScreenInfo in this same class: void RenderWidgetHostViewAndroid::GetScreenInfo(blink::WebScreenInfo* result) { // ScreenInfo isn't tied to the widget on Android. Always return the default. RenderWidgetHostViewBase::GetDefaultScreenInfo(result); } I am not sure about the comment here about ScreenInfo not being tied to the widget -- we do have a WindowAndroid: content_view_core_window_android_ which probably should be used here? (though we would need to create a gfx::Display from a WindowAndroid/DeviceDisplayInfo similar to how it is currently done in screen_android) 2. void RenderWidgetHostImpl::GetWebScreenInfo(blink::WebScreenInfo* result) { if (view_) view_->GetScreenInfo(result); else RenderWidgetHostViewBase::GetDefaultScreenInfo(result); ... } this should be fine if we fix nr.1? https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:321: // TODO if we cache DeviceDisplayInfo then update the caches here On 2015/06/01 15:30:18, boliu wrote: > Hmm, this means native device_display_info shouldn't cache any results, and > should always go through jni to ask the java side. Otherwise anything caching a > device_display_info would get the wrong value after rotation? > > (Gosh this was a big hack before) What's the problem with updating the caches here? (crbug.com/265008 suggests that we should cache DeviceDisplayInfo) https://codereview.chromium.org/1144333004/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:488: public WeakReference<Context> getCurrentContext() { On 2015/06/01 15:30:18, boliu wrote: > What does "current" mean here? > > Just getContext? Done. https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.cc:27: DeviceDisplayInfo::DeviceDisplayInfo() { // TODO ugly? On 2015/06/01 15:30:18, boliu wrote: > c++11 allows just calling the other constructor now I think I am not sure what you mean by this? The other constructor takes a windowandroid so can't really use it here? We could take in a context here and let each caller explicitly use GetApplicationContext(). Using a default constructor now was easier since I wouldn't have to change everything under chrome/ for this initial CL but to avoid future mistakes it might be better to force a call with a context? https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.h:22: explicit DeviceDisplayInfo(jobject windowAndroid); On 2015/06/01 15:30:18, boliu wrote: > Can this be the native WindowAndroid type instead? I get a presubmit error if I try to include ui/android/window_android.h in this file: You added one or more #includes that violate checkdeps rules. ui/gfx/android/device_display_info.h Illegal include: "ui/android/window_android.h" Because of "-ui" from ui's include_rules.
I didn't read PS2 On 2015/06/04 14:10:30, gsennton wrote: > > > I have not added any caching yet (we could have WindowAndroid own a cached > > DeviceDisplayInfo?). > > > > I assume WindowAndroid can't include DeviceDisplayInfo. > > I did not see any problems when trying to including DeviceDisplayInfo from > WindowAndroid (but the other way around did not work). > Do we have access to some kind of shared_ptr or similar to hold the cached copy? Yeah. WindowAndroid owning a cached DeviceDisplayInfo is fine. I got the layering backwards when I made that comment. https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1144333004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:2050: // we can only use the primary display? On 2015/06/04 14:10:29, gsennton wrote: > On 2015/06/01 15:30:18, boliu wrote: > > Yes > > > > Although should double check that any caller of this should be updated to use > > WindowAndroid instead > > This is called from two places: > > 1. GetScreenInfo in this same class: > void RenderWidgetHostViewAndroid::GetScreenInfo(blink::WebScreenInfo* result) { > // ScreenInfo isn't tied to the widget on Android. Always return the default. > RenderWidgetHostViewBase::GetDefaultScreenInfo(result); > } > I am not sure about the comment here about ScreenInfo not being tied to the > widget -- we do have a WindowAndroid: content_view_core_window_android_ which > probably should be used here? (though we would need to create a gfx::Display > from a WindowAndroid/DeviceDisplayInfo similar to how it is currently done in > screen_android) You are right. Comment assumes android only has a single screen, so comment is wrong. > > 2. > void RenderWidgetHostImpl::GetWebScreenInfo(blink::WebScreenInfo* result) { > if (view_) > view_->GetScreenInfo(result); > else > RenderWidgetHostViewBase::GetDefaultScreenInfo(result); > ... > } > this should be fine if we fix nr.1? Yep https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:321: // TODO if we cache DeviceDisplayInfo then update the caches here On 2015/06/04 14:10:29, gsennton wrote: > On 2015/06/01 15:30:18, boliu wrote: > > Hmm, this means native device_display_info shouldn't cache any results, and > > should always go through jni to ask the java side. Otherwise anything caching > a > > device_display_info would get the wrong value after rotation? > > > > (Gosh this was a big hack before) > > What's the problem with updating the caches here? (crbug.com/265008 suggests > that we should cache DeviceDisplayInfo) Caching is fine. The problem is ui/gfx is its own standalone component. It shouldn't need a higher level construct in content to keep it working. So the right solution is to have ui/gfx listen to orientation change events, and keep itself up-to-date. https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.cc:27: DeviceDisplayInfo::DeviceDisplayInfo() { // TODO ugly? On 2015/06/04 14:10:29, gsennton wrote: > On 2015/06/01 15:30:18, boliu wrote: > > c++11 allows just calling the other constructor now I think > > I am not sure what you mean by this? The other constructor takes a windowandroid > so can't really use it here? Oh good point. But you could factor out the body of these two constructors into a single private function that both constructors calls, to avoid code duplication. > We could take in a context here and let each caller > explicitly use GetApplicationContext(). Using a default constructor now was > easier since I wouldn't have to change everything under chrome/ for this initial > CL but to avoid future mistakes it might be better to force a call with a > context? https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1144333004/diff/1/ui/gfx/android/device_displ... ui/gfx/android/device_display_info.h:22: explicit DeviceDisplayInfo(jobject windowAndroid); On 2015/06/04 14:10:29, gsennton wrote: > On 2015/06/01 15:30:18, boliu wrote: > > Can this be the native WindowAndroid type instead? > > I get a presubmit error if I try to include ui/android/window_android.h in this > file: > > You added one or more #includes that violate checkdeps rules. > ui/gfx/android/device_display_info.h > Illegal include: "ui/android/window_android.h" > Because of "-ui" from ui's include_rules. Right, I have to internalize that ui/gfx is actually at a *lower* level than everything else in ui, including ui/base. Kinda weird. That means layering is wrong here. And having a jobject windowAndroid is basically cheating to get around the layering problem. So lower layers cannot depend on higher layers. Which means this class should know nothing about WindowAndroid, java or native. Maybe it should be a context instead. It's tricker beacuse having the Context be owned by a higher layer object is the wrong thing here. I don't know ui/gfx that well, but glancing through classes, display might make more sense to be holding context. I have no idea if android actually uses that though.. (Pretty much everyone else uses ui/views for native UI, but android uses android views, so android does weird things here)
gsennton@chromium.org changed reviewers: + jdduke@chromium.org
Moved screen_android.cc from ui/gfx/ to ui/android/ because we can't include ui/android/window_android or ui/android/view_android from ui/gfx (and we need those files in screen_android). Added jdduke@ because of that move. Will start another CL for changes where we want to stop calling Screen::GetPrimaryDisplay() and start using the current ViewAndroid.
+mlamouri who worked on the display info/orientation changes previously.
jdduke@chromium.org changed reviewers: + mlamouri@chromium.org
(actually adding mlamouri this time...) +mlamouri who worked on the display info/orientation changes previously.
Ugg, I'm really not a fan of this implicit ScreenOrientation responsibility for ensuring updates of DeviceDisplayInfo, Mounir is there anything we can do about that (https://chromium.googlesource.com/chromium/src/+/0aa9e8b3de6affa566e3bfc6018a... https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:193: return mWinManager.getDefaultDisplay(); So the "default" Display can vary per WindowManager, and the WindowManager can very between activities? Is that how this works? https://codereview.chromium.org/1144333004/diff/60001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/60001/ui/android/window_andro... ui/android/window_android.h:68: std::shared_ptr<gfx::DeviceDisplayInfo> GetDeviceDisplayInfo(); I'd prefer this not be a shared reference. Can we return by const ref? And hold the handle internally as a scoped_ptr? https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( So, there's an assumption currently that creating a DeviceDisplayInfo is a trivially cheap operation, but that's no longer the case. Do you have a sense for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload?
https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:193: return mWinManager.getDefaultDisplay(); On 2015/06/16 15:01:28, jdduke wrote: > So the "default" Display can vary per WindowManager, and the WindowManager can > very between activities? Is that how this works? AFAICT this is indeed the case, e.g. using WebView with a Presentation http://developer.android.com/reference/android/app/Presentation.html and using the window manager of the current activity context here gives us the display targeted by the Presentation (while using the application context instead gives us the primary display). https://codereview.chromium.org/1144333004/diff/60001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/60001/ui/android/window_andro... ui/android/window_android.h:68: std::shared_ptr<gfx::DeviceDisplayInfo> GetDeviceDisplayInfo(); On 2015/06/16 15:01:28, jdduke wrote: > I'd prefer this not be a shared reference. Can we return by const ref? And hold > the handle internally as a scoped_ptr? Done. https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( On 2015/06/16 15:01:28, jdduke wrote: > So, there's an assumption currently that creating a DeviceDisplayInfo is a > trivially cheap operation, but that's no longer the case. Do you have a sense > for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload? Potentially stupid question from my part -- why is creating a DeviceDisplayInfo no longer a cheap operation? I do not know how often this happens, is there a good way of measuring this?
https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( On 2015/06/17 11:40:36, gsennton wrote: > On 2015/06/16 15:01:28, jdduke wrote: > > So, there's an assumption currently that creating a DeviceDisplayInfo is a > > trivially cheap operation, but that's no longer the case. Do you have a sense > > for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload? > > Potentially stupid question from my part -- why is creating a DeviceDisplayInfo > no longer a cheap operation? > Now we have to construct a new Java object, and call ~10 JNI methods. Before, creating a DeviceDisplayInfo instance was a no-op. A cursory grep shows a few places where we create throw-away DeviceDisplayInfo instances, e.g., tab_android.cc, compositor_impl_android.cc, etc... I don't want us to be doing all this extra work if we can avoid it, maybe those callsites can be updated to pull the info from the WindowAndroid object? > I do not know how often this happens, is there a good way of measuring this? Well, you can just add some log statements here and see how frequently it's called when you load a new page, scroll around, etc... https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:20: j_device_info_.Reset( It's a shame to have to create a Java object here that's really only used once (upon creation). https://codereview.chromium.org/1144333004/diff/80001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/80001/ui/android/window_andro... ui/android/window_android.h:67: const gfx::DeviceDisplayInfo & GetDeviceDisplayInfo(); Nit: no space before &, this patch probably deserves a "git cl format".
https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( On 2015/06/17 15:56:04, jdduke wrote: > On 2015/06/17 11:40:36, gsennton wrote: > > On 2015/06/16 15:01:28, jdduke wrote: > > > So, there's an assumption currently that creating a DeviceDisplayInfo is a > > > trivially cheap operation, but that's no longer the case. Do you have a > sense > > > for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload? > > > > Potentially stupid question from my part -- why is creating a > DeviceDisplayInfo > > no longer a cheap operation? > > > > Now we have to construct a new Java object, and call ~10 JNI methods. Before, > creating a DeviceDisplayInfo instance was a no-op. A cursory grep shows a few > places where we create throw-away DeviceDisplayInfo instances, e.g., > tab_android.cc, compositor_impl_android.cc, etc... I don't want us to be doing > all this extra work if we can avoid it, maybe those callsites can be updated to > pull the info from the WindowAndroid object? The bigger issue is to avoid gc pauses. Eg during each frame, somethings can be creating native DeviceDispalyInfo on the stack, which just creates a java object and throws it away. So then during a fling, there's jvm gc pause every few seconds, which sucks. JNI calls aren't *that* bad.. > > > I do not know how often this happens, is there a good way of measuring this? > > Well, you can just add some log statements here and see how frequently it's > called when you load a new page, scroll around, etc...
In PS6 callers to ScreenOrientationListener need to own a ScreenOrientationListener (so there is no global ScreenOrientationListener which everyone adds an observer to). mlamouri@ would you like to take a look at ScreenOrientationListener.java? The rest of the changes are mostly because of git cl format. I realized one big problem with this CL though -- the Context we want to use to fetch display information is not necessarily an Activity -- the Context of a Presentation (http://developer.android.com/reference/android/app/Presentation.html) does not have to be an Activity. This means that ActivityWindowAndroid will not hold the context we are after when using a Presentation (even though the context we seek is held by AwContents). Could we store a weak reference to the context owned by AwContents in WindowAndroid and use that instead of the activity from ActivityWindowAndroid? https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( On 2015/06/17 16:02:20, boliu wrote: > On 2015/06/17 15:56:04, jdduke wrote: > > On 2015/06/17 11:40:36, gsennton wrote: > > > On 2015/06/16 15:01:28, jdduke wrote: > > > > So, there's an assumption currently that creating a DeviceDisplayInfo is a > > > > trivially cheap operation, but that's no longer the case. Do you have a > > sense > > > > for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload? > > > > > > Potentially stupid question from my part -- why is creating a > > DeviceDisplayInfo > > > no longer a cheap operation? > > > > > > > Now we have to construct a new Java object, and call ~10 JNI methods. Before, > > creating a DeviceDisplayInfo instance was a no-op. A cursory grep shows a few > > places where we create throw-away DeviceDisplayInfo instances, e.g., > > tab_android.cc, compositor_impl_android.cc, etc... I don't want us to be doing > > all this extra work if we can avoid it, maybe those callsites can be updated > to > > pull the info from the WindowAndroid object? > > The bigger issue is to avoid gc pauses. Eg during each frame, somethings can be > creating native DeviceDispalyInfo on the stack, which just creates a java object > and throws it away. So then during a fling, there's jvm gc pause every few > seconds, which sucks. > > JNI calls aren't *that* bad.. > > > > > > I do not know how often this happens, is there a good way of measuring this? > > > > Well, you can just add some log statements here and see how frequently it's > > called when you load a new page, scroll around, etc... > Oh, right, I missed this entirely -- if jni calls are expensive we could just have the java-side send all the values in one single function call...? https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:20: j_device_info_.Reset( On 2015/06/17 15:56:04, jdduke wrote: > It's a shame to have to create a Java object here that's really only used once > (upon creation). We could cache the primary display info in some shared_device_display_info class and let that class add an observer to ScreenOrientationListener as well? Then we should only need to create this java object once. https://codereview.chromium.org/1144333004/diff/80001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/80001/ui/android/window_andro... ui/android/window_android.h:67: const gfx::DeviceDisplayInfo & GetDeviceDisplayInfo(); On 2015/06/17 15:56:05, jdduke wrote: > Nit: no space before &, this patch probably deserves a "git cl format". Done.
On 2015/06/18 11:37:22, gsennton wrote: > In PS6 callers to ScreenOrientationListener need to own a > ScreenOrientationListener (so there is no global ScreenOrientationListener which > everyone adds an observer to). mlamouri@ would you like to take a look at > ScreenOrientationListener.java? > > The rest of the changes are mostly because of git cl format. > > I realized one big problem with this CL though -- the Context we want to use to > fetch display information is not necessarily an Activity -- the Context of a > Presentation > (http://developer.android.com/reference/android/app/Presentation.html) does not > have to be an Activity. This means that ActivityWindowAndroid will not hold the > context we are after when using a Presentation (even though the context we seek > is held by AwContents). Could we store a weak reference to the context owned by > AwContents in WindowAndroid and use that instead of the activity from > ActivityWindowAndroid? Does that mean we aren't allowed to show pop up windows and whatnot inside a presentation context? That's the main difference between ActivityWindowAndroid vs WindowAndroid. But so far we've assumed WindowAndroid is holding the global application context we hold a strong ref there. So what is the presentation context exactly? If it's per-presentation like activity contexts, then webviews would be leaking in a presentation.
The Screen Orientation changes look fine. Though, it's getting a bit messy and I'm a bit concerned with having multiple ScreenOrientationListener each running their own polling loop. Ideally, we should start sending the accurate listening messages to the frame now that the display is associated to a Window so can be reached from the frame. I guess it's worth at least filling a follow-up for that. In the mean time, maybe it would be better to run one loop for all the ScreenOrientationListener so we reduce the interruptions? That loop could be static and call onConfigurationChanged() on the sListeners. Furthermore, could you by any chance manually test on a device running JB (pre MR1) that the API behave as expected? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:103: if (context != null) { if (context == null) return; https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:117: // unregister the callback)? I guess we shouldn't worry about that? The context being destroyed means that we will never be called. https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:120: // stop accurate listening if it is active I don't think you need that comment. https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:121: if (mRunningAccurateListening) { if (mRunningAccurateListening()) stopAccurateListening(); https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:129: if (mRunningAccurateListening) return; // we have already started polling Could you change that to an assert? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:150: mRunningAccurateListening = false; Could you add: assert mRunningAccurateListening; https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:191: // TODO what if the context has already been destroyed (and we can't TODO's need a name. Also, wouldn't the DisplayManager be destroyed if the context is destroyed given that we get it from the system service associated with it? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:245: mContext = new WeakReference<Context>(context); Why do we keep a WeakReference? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:249: // TODO could this be called at the same time as traversing sListeners? TODO's need names. Also, WDYM? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:379: assert sAccurateCount >= 0; assert sAccurateCount == 0; https://codereview.chromium.org/1144333004/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java (right): https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:117: ScreenOrientationListener.staticStartAccurateListening(); Is staticFoo() a common pattern? It sounds odd to my ears.
> The Screen Orientation changes look fine. Though, it's getting a bit messy and > I'm a bit concerned with having multiple ScreenOrientationListener each running > their own polling loop. Ideally, we should start sending the accurate listening > messages to the frame now that the display is associated to a Window so can be > reached from the frame. I guess it's worth at least filling a follow-up for > that. I'm a bit confused here, how would the accurate listening be implemented then (if we send the accurate listening messages to the frame)? > In the mean time, maybe it would be better to run one loop for all the > ScreenOrientationListener so we reduce the interruptions? That loop could be > static and call onConfigurationChanged() on the sListeners. Just calling notifyObservers() right now (onConfigurationChanged() is specific to one of the backend implementations). > Furthermore, could you by any chance manually test on a device running JB (pre > MR1) that the API behave as expected? Sure, how would you test that? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:103: if (context != null) { On 2015/06/18 15:48:46, Mounir Lamouri wrote: > if (context == null) return; Done. https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:117: // unregister the callback)? On 2015/06/18 15:48:45, Mounir Lamouri wrote: > I guess we shouldn't worry about that? The context being destroyed means that we > will never be called. But we would never remove the callbacks either, right? Anyway, it seems the callbacks are registered through the application context https://cs.corp.google.com/#android/frameworks/base/core/java/android/content... so if we only want a weak reference to the context we could store a hard reference to its application context just for the ScreenOrientationConfigurationListener to be able to un/register the callbacks through that. https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:191: // TODO what if the context has already been destroyed (and we can't On 2015/06/18 15:48:46, Mounir Lamouri wrote: > TODO's need a name. > > Also, wouldn't the DisplayManager be destroyed if the context is destroyed given > that we get it from the system service associated with it? It seems we are adding the display listener to a global display manager https://cs.corp.google.com/#android/frameworks/base/core/java/android/hardwar... so we will never remove that/those listeners if the context is destroyed before we call stopListening :( This is only a problem if we hold a weak reference to the context though (see my comment about that below). https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:245: mContext = new WeakReference<Context>(context); On 2015/06/18 15:48:46, Mounir Lamouri wrote: > Why do we keep a WeakReference? I am using a WeakReference to make sure that we are not the last object holding on to the context (but I guess since this is used from ContentViewCore that holds a strong reference to its context anyway the WeakReference should not be necessary? :) https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:249: // TODO could this be called at the same time as traversing sListeners? On 2015/06/18 15:48:45, Mounir Lamouri wrote: > TODO's need names. Also, WDYM? I am just wondering whether we might add an object to sListeners at the same time as some other thread calls one of the static start/stopAccurateListening calls, so that we try to add an element to sListeners while someone else traverses sListeners? And if so, is that a problem? https://codereview.chromium.org/1144333004/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java (right): https://codereview.chromium.org/1144333004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:117: ScreenOrientationListener.staticStartAccurateListening(); On 2015/06/18 15:48:46, Mounir Lamouri wrote: > Is staticFoo() a common pattern? It sounds odd to my ears. The names start/stopAccurateListening clashed with the non-static methods of ScreenOrientationListener.
On 2015/06/19 at 12:07:55, gsennton wrote: > > The Screen Orientation changes look fine. Though, it's getting a bit messy and > > I'm a bit concerned with having multiple ScreenOrientationListener each running > > their own polling loop. Ideally, we should start sending the accurate listening > > messages to the frame now that the display is associated to a Window so can be > > reached from the frame. I guess it's worth at least filling a follow-up for > > that. > > I'm a bit confused here, how would the accurate listening be implemented then (if we send the accurate listening messages to the frame)? Each View/Widget would have an associated Window and Display, right? A frame could tell the ScreenOrientationListener associated to that display to begin accurate listening, right? > > Furthermore, could you by any chance manually test on a device running JB (pre > > MR1) that the API behave as expected? > > Sure, how would you test that? Go to http://oldworld.fr/google/screenorientation.html and rotate your device 180 degrees. You should see orientationchange events. 90 degrees change should always work. The accurate listening is here for the 180 degrees change that wouldn't work otherwise on pre JB-MR1 devices.
On 2015/06/19 12:33:51, Mounir Lamouri wrote: > On 2015/06/19 at 12:07:55, gsennton wrote: > > > The Screen Orientation changes look fine. Though, it's getting a bit messy > and > > > I'm a bit concerned with having multiple ScreenOrientationListener each > running > > > their own polling loop. Ideally, we should start sending the accurate > listening > > > messages to the frame now that the display is associated to a Window so can > be > > > reached from the frame. I guess it's worth at least filling a follow-up for > > > that. > > > > I'm a bit confused here, how would the accurate listening be implemented then > (if we send the accurate listening messages to the frame)? > > Each View/Widget would have an associated Window and Display, right? A frame > could tell the ScreenOrientationListener associated to that display to begin > accurate listening, right? Right, sounds good :)
https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:193: return mWinManager.getDefaultDisplay(); On 2015/06/17 11:40:36, gsennton wrote: > On 2015/06/16 15:01:28, jdduke wrote: > > So the "default" Display can vary per WindowManager, and the WindowManager can > > very between activities? Is that how this works? > > AFAICT this is indeed the case, e.g. using WebView with a Presentation > http://developer.android.com/reference/android/app/Presentation.html and using > the window manager of the current activity context here gives us the display > targeted by the Presentation (while using the application context instead gives > us the primary display). I should clarify this (my comment above is not entirely correct): As described in https://cs.corp.google.com/#android/frameworks/base/core/java/android/view/Wi... a WindowManager is connected/bound to a certain display. What a Presentation does is create its own context using Context#createDisplayContext to target a certain Display. That context is different from the application context and also different from the context of the activity containing the Presentation. That is why we need to use the context passed to WebView rather than its application context to fetch the metrics of the targeted display. I am not sure how this will work with ActivityWindowAndroid since the Presentation context is not necessarily an activity context. https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/60001/ui/gfx/android/device_d... ui/gfx/android/device_display_info.cc:10: DeviceDisplayInfo::DeviceDisplayInfo( On 2015/06/16 15:01:28, jdduke wrote: > So, there's an assumption currently that creating a DeviceDisplayInfo is a > trivially cheap operation, but that's no longer the case. Do you have a sense > for how frequently this is created? 1x/frame? 10x/frame? 1x/pageload? To answer the original question here, Chrome: When starting Chrome I see 9 uses of the java DeviceDisplayInfo constructor: 7 using the default DeviceDisplayInfo constructor from the native side, 1 using the WindowAndroid and 1 from the java side. When clicking a link and loading a new page the java constructor is used twice (both from GetPrimaryDisplay from the native side). Scrolling hits RenderWidgetHostViewAndroid::GetScreenInfo a lot and afaict nothing else (with this patch that callpath uses the cached copy in WindowAndroid). Changing the screen orientation updates the WindowAndroid cache once and then hits RenderWidgetHostViewAndroid::GetScreenInfo a lot (probably to adjust the scroll?) WebView in Browser: (WebView in a simple test app is similar except each screen orientation change seems to recreate WebView) Startup: 8 constructor calls, 6 from the DeviceDisplayInfo default constructor, 1 for WindowAndroid, 1 from the java side Scrolling and clicking links seem to always hit the WindowAndroid cache. Changing the screen orientation behaves similar to Chrome.
The ScreenOrientationListener in PS8 has been tested as directed by mlamouri@ in #20. I am now keeping a strong reference to the context passed to ScreenOrientationListener since we are holding a strong reference to the context in ContentViewCore anyway (that is where we use ScreenOrientationListener).
Ok, so this should be fairly close to done now: I am looking into whether we need to update media/base/android/java/src/org/chromium/media/VideoCapture.java to stop using the current ApplicationContext. Apart from that I am unsure whether we might call ScreenOrientationListener from different threads (and thus mess up the state of its static variables)? The rest should probably be broken out into follow-up CLs: The change needed in render_widget_compositer.cc should only affect performance and does not depend on this CL (and could be fiddly) ViewConfiguration and GestureConfiguration use a single static instance to represent metrics for all displays (should be per-display): These classes are affected by the DPI/density of the current display but since jdduke landed https://codereview.chromium.org/1216933013/ those classes should no longer depend on the current density value (I would prefer to make those classes per-display or per-context in a follow-up CL -- those changes would blob this CL even more..).
Yes, please split this into smaller CLs if possible. And let reviewers know if this is ready to be reviewed. On 2015/07/10 15:35:55, gsennton wrote: > Ok, so this should be fairly close to done now: > > I am looking into whether we need to update > media/base/android/java/src/org/chromium/media/VideoCapture.java > to stop using the current ApplicationContext. > > Apart from that I am unsure whether we might call ScreenOrientationListener from > different threads (and thus mess up the state of its static variables)? ScreenOrientationListener should only be used on the UI thread. > > The rest should probably be broken out into follow-up CLs: > > The change needed in render_widget_compositer.cc should only affect performance > and does not depend on this CL (and could be fiddly) > > ViewConfiguration and GestureConfiguration use a single static instance to > represent metrics for all displays (should be per-display): > These classes are affected by the DPI/density of the current display but since > jdduke landed https://codereview.chromium.org/1216933013/ those classes should > no longer depend on the current density value (I would prefer to make those > classes per-display or per-context in a follow-up CL -- those changes would blob > this CL even more..).
gsennton@chromium.org changed reviewers: + sadrul@chromium.org, sievers@chromium.org
Ok, this should be ready for review, added the rest of the necessary reviewers: boliu android_webview/ sievers@chromium.org content/ (except content/public/android/java/src/org/chromium/content/browser/) mlamouri@ content/public/android/java/src/org/chromium/content/browser/ jdduke@ ui/android/ ui/gfx/android sadrul@ ui/gfx except ui/gfx/android/ ui/snapshot/
https://codereview.chromium.org/1144333004/diff/180001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1144333004/diff/180001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1342: if (windowAndroid) Don't need this null check. window_android_ is never null while CVCI is alive https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; This should be per context, instead of per contentviewcore https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:62: this(activity, true, activity.getApplicationContext()); Should this just be activity? Why is displayContext the application context here? And if that's the case, activity and displayContext are always equal, so don't need to add the new param below. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:79: protected WeakReference<Context> mDisplayContextRef; Add comment why this is needed https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:135: public WindowAndroid(Context context, Context displayContext) { Only need |displayContext|. |context| is always displayContext.getApplicationContext(), right? https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:568: public Context getDisplayContext() { private Why is this needed at all? https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:39: if (context == null) Pass a strong ref in constructor, then this can't ever happen https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:163: return context.getResources().getConfiguration().smallestScreenWidthDp; I wonder if this can ever change. Maybe it's ok to cache the value in the constructor. Throwing exception is not ok. That's the same as crashing in a @CalledByNative method, and an object going away under a weak pointer has to be handled without crashing. https://codereview.chromium.org/1144333004/diff/180001/ui/android/screen_andr... File ui/android/screen_android.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/screen_andr... ui/android/screen_android.cc:9: #include "ui/android/window_android.h" Instead of moving this file, ui/android should just be part of ui/gfx/DEPS. ui/ios is already there. But owners should decide https://codereview.chromium.org/1144333004/diff/180001/ui/android/screen_andr... ui/android/screen_android.cc:16: gfx::Display DisplayFromDeviceDisplayInfo( out of scope of this CL, but... Feels like either android-specific DeviceDisplayInfo should be deprecated and replaced by the more generic gfx::Display. https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.cc:141: device_display_info_ = scoped_ptr<gfx::DeviceDisplayInfo>( make_scoped_ptr https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); Looks like this is public only for ResourceManager construction? Is it possible to hide the java object implementation internal to ui layer, ie maybe ResourceManager can take a WindowAndroid pointer instead? https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:18: JNIEnv* env = base::android::AttachCurrentThread(); can use c++11 delegated constructor here I think? https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.h:26: void FetchDisplayInfoFromJava(JNIEnv* env); private, also this should just be merged with UpdateDisplayInfo
Sorry, realize I had a batch of comments from last week I never bothered sending. Bo hit a few of them already. https://codereview.chromium.org/1144333004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1144333004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:128: static GLHelperHolder* Create(gfx::NativeWindow nativeWindow); native_window everywhere, though if these classes are Android-specific I'd be OK just using the aliased ui::WindowAndroid identifier directly. https://codereview.chromium.org/1144333004/diff/160001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1144333004/diff/160001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:548: gfx::NativeWindow nativeWindow) { Hmm, what about passing it in as "const ui::WindowAndroid&"? https://codereview.chromium.org/1144333004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1144333004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:128: static GLHelperHolder* Create(gfx::NativeWindow nativeWindow); Again, I wonder if it would improve readability to pass in a "const ui::WindowAndroid&" for the GL methods here? Alternatively what about just passing in the display dimensions directly? I'd almost prefer that to holding on to the pointer for the async callback. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:163: return context.getResources().getConfiguration().smallestScreenWidthDp; On 2015/07/15 06:38:47, boliu wrote: > I wonder if this can ever change. Maybe it's ok to cache the value in the > constructor. > > Throwing exception is not ok. That's the same as crashing in a @CalledByNative > method, and an object going away under a weak pointer has to be handled without > crashing. +1. It might also be worth investigating why this constant is needed, and if it's absolutely necessary. Regardless, even if the value does change I don't think we'd react to the change properly anyway, and I'd be OK storing it in the constructor. https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:18: JNIEnv* env = base::android::AttachCurrentThread(); Hmm, it might be nice to have a TODO to get rid of the default constructor. https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( Silly question. Do we really need to hold a reference to a Java object here? It doesn't look like we really support "live" updating of the object. Would it be any better to do something like: Java_DeviceDisplayInfo_requestUpdate(env, context, this); And the Java side would do something like: @CalledByNative private void requestUpdate(context, nativePtr) { DeviceDisplayInfo info = new DeviceDisplayInfo(); nativeUpdate(nativePtr, getFoo(), getBar()); } Alternatively, you could make the Update() public and allow live updating, but it looks like you simply recreate the object when necessary so I don't really see the advantage of storing the temporary Java object. https://codereview.chromium.org/1144333004/diff/180001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_android.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_android.cc:44: ? gfx::Display::GetForcedDeviceScaleFactor() Why not use GetDisplayNearestWindow which should do the scale work for you?
https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/15 16:03:07, jdduke wrote: > Silly question. Do we really need to hold a reference to a Java object here? It > doesn't look like we really support "live" updating of the object. > > Would it be any better to do something like: > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > And the Java side would do something like: > > @CalledByNative > private void requestUpdate(context, nativePtr) { > DeviceDisplayInfo info = new DeviceDisplayInfo(); > nativeUpdate(nativePtr, getFoo(), getBar()); > } > > Alternatively, you could make the Update() public and allow live updating, but > it looks like you simply recreate the object when necessary so I don't really > see the advantage of storing the temporary Java object. Not only will that cut out most of the JNI calls, we could also just use a static temporary Java variable to avoid the allocation for native updates (assuming we only call this on the UI thread? do we?).
https://codereview.chromium.org/1144333004/diff/180001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1144333004/diff/180001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1342: if (windowAndroid) On 2015/07/15 06:38:46, boliu wrote: > Don't need this null check. window_android_ is never null while CVCI is alive Done. https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/07/15 06:38:46, boliu wrote: > This should be per context, instead of per contentviewcore I don't quite understand this, aren't we only holding one context in contentviewcore anyway? (and how do you mean this would look if it was per context?) https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:62: this(activity, true, activity.getApplicationContext()); On 2015/07/15 06:38:46, boliu wrote: > Should this just be activity? Why is displayContext the application context > here? > > And if that's the case, activity and displayContext are always equal, so don't > need to add the new param below. activity and displayContext are not always equal (see AwContents.java) -- especially, in the case where we target an external display the two will not be equal since then the displayContext will have been create through Context.createDisplayContext(Display). I guess that means that we should never create an ActivityWindowAndroid when targeting an external display (since we only do that in AwContents when our Context is an instance of Activity) and in that case we only need to worry about displayContext in WindowAndroid and not in ActivityWindowAndroid? https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:79: protected WeakReference<Context> mDisplayContextRef; On 2015/07/15 06:38:47, boliu wrote: > Add comment why this is needed Done. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:135: public WindowAndroid(Context context, Context displayContext) { On 2015/07/15 06:38:46, boliu wrote: > Only need |displayContext|. |context| is always > displayContext.getApplicationContext(), right? Done. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:568: public Context getDisplayContext() { On 2015/07/15 06:38:46, boliu wrote: > private > > Why is this needed at all? To be called from the native side when creating the cached DeviceDisplayInfo instance. (see WindowAndroid::GetJavaDisplayContext()) https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:39: if (context == null) On 2015/07/15 06:38:47, boliu wrote: > Pass a strong ref in constructor, then this can't ever happen Unless the incoming context is null because we retrieved it from WindowAndroid which only holds a weak reference to its display context (and therefore might pass null)? https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:163: return context.getResources().getConfiguration().smallestScreenWidthDp; On 2015/07/15 16:03:07, jdduke wrote: > On 2015/07/15 06:38:47, boliu wrote: > > I wonder if this can ever change. Maybe it's ok to cache the value in the > > constructor. > > > > Throwing exception is not ok. That's the same as crashing in a @CalledByNative > > method, and an object going away under a weak pointer has to be handled > without > > crashing. > > +1. It might also be worth investigating why this constant is needed, and if > it's absolutely necessary. Regardless, even if the value does change I don't > think we'd react to the change properly anyway, and I'd be OK storing it in the > constructor. Right, going ahead and storing the value in the constructor. Checking with skobes@ whether getSmallestDIPWidth can be removed (it was added in https://codereview.chromium.org/28053002). https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.cc:141: device_display_info_ = scoped_ptr<gfx::DeviceDisplayInfo>( On 2015/07/15 06:38:47, boliu wrote: > make_scoped_ptr Done. https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/07/15 06:38:47, boliu wrote: > Looks like this is public only for ResourceManager construction? > > Is it possible to hide the java object implementation internal to ui layer, ie > maybe ResourceManager can take a WindowAndroid pointer instead? That should work, I am not sure how we would handle a potential null ref to the WindowAndroid Context over in the ResourceManager though (I guess with the code of this PS we aren't handling a potential null reference either...). Another problem is that resource_manager_impl_unittest.cc constructs a ResourceManagerImpl which is easy to do now with base::android::GetApplicationContext() but it might be difficult to pass a WindowAndroid java object instead? (I guess we could create some empty WindowAndroid object where only the Context is important...) https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:18: JNIEnv* env = base::android::AttachCurrentThread(); On 2015/07/15 06:38:47, boliu wrote: > can use c++11 delegated constructor here I think? Done. https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:18: JNIEnv* env = base::android::AttachCurrentThread(); On 2015/07/15 16:03:07, jdduke wrote: > Hmm, it might be nice to have a TODO to get rid of the default constructor. As in changing to use the WindowAndroid context or to just call the other constructor with GetApplicationContext explicitly? https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/15 16:13:41, jdduke wrote: > On 2015/07/15 16:03:07, jdduke wrote: > > Silly question. Do we really need to hold a reference to a Java object here? > It > > doesn't look like we really support "live" updating of the object. > > > > Would it be any better to do something like: > > > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > > > And the Java side would do something like: > > > > @CalledByNative > > private void requestUpdate(context, nativePtr) { > > DeviceDisplayInfo info = new DeviceDisplayInfo(); > > nativeUpdate(nativePtr, getFoo(), getBar()); > > } > > > > Alternatively, you could make the Update() public and allow live updating, but > > it looks like you simply recreate the object when necessary so I don't really > > see the advantage of storing the temporary Java object. > > Not only will that cut out most of the JNI calls, we could also just use a > static temporary Java variable to avoid the allocation for native updates > (assuming we only call this on the UI thread? do we?). I am not sure what you mean by using a static temporary java variable here (and how that would save allocations)? Oh, I am updating DeviceDisplayInfo in window_android.cc in a stupid way (by creating a new native DeviceDisplayInfo and therefore also a new java object for every update) -- we could just have a call on the native side updating all the values through jni calls using the stored java object (unless we prefer your suggestion)? https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.h:26: void FetchDisplayInfoFromJava(JNIEnv* env); On 2015/07/15 06:38:47, boliu wrote: > private, also this should just be merged with UpdateDisplayInfo Done. https://codereview.chromium.org/1144333004/diff/180001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_android.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_android.cc:44: ? gfx::Display::GetForcedDeviceScaleFactor() On 2015/07/15 16:03:07, jdduke wrote: > Why not use GetDisplayNearestWindow which should do the scale work for you? GetDisplayNearestWindow takes a NativeView (contrary to what the method name suggests) while we only have access to a NativeWindow here. (And a ViewAndroid holds a pointer to a WindowAndroid but not the other way around).
https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 06:38:46, boliu wrote: > > This should be per context, instead of per contentviewcore > > I don't quite understand this, aren't we only holding one context in > contentviewcore anyway? (and how do you mean this would look if it was per > context?) Many ContentViewCore can have the same context, in which case they should share the same ScreenOrientationListener. One hacky way to implement it is keep a static WeakHashMap<Context, ScreenOrientationListener>. Cleaner concept would be to move ScreenOrientationListener entirely into WindowAndroid. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:62: this(activity, true, activity.getApplicationContext()); On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 06:38:46, boliu wrote: > > Should this just be activity? Why is displayContext the application context > > here? > > > > And if that's the case, activity and displayContext are always equal, so don't > > need to add the new param below. > > activity and displayContext are not always equal (see AwContents.java) -- > especially, in the case where we target an external display the two will not be > equal since then the displayContext will have been create through > Context.createDisplayContext(Display). Wait. How do you get to that display context from an activity then? I hope it's not activity.getApplicationContext. It's not the activity itself? > I guess that means that we should never create an ActivityWindowAndroid when > targeting an external display (since we only do that in AwContents when our > Context is an instance of Activity) and in that case we only need to worry about > displayContext in WindowAndroid and not in ActivityWindowAndroid? Not including Services, there are only two kinds of context: the global ApplicationContext, and we create WindowAndroid for that, and Activity, and we create ActivityWindowAndroid for that. So shouldn't it possible for an activity to belong to an external display? In that case, we should create an ActivityWindowAndroid, but like I ask above, I don't know what the "display context" should be. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:39: if (context == null) On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 06:38:47, boliu wrote: > > Pass a strong ref in constructor, then this can't ever happen > > Unless the incoming context is null because we retrieved it from WindowAndroid > which only holds a weak reference to its display context (and therefore might > pass null)? You can check for null before passing it into DeviceDisplayInfo then. And if it is null, you'll have to handle that correctly too. Make a dummy DeviceDisplayInfo, or check caller can handle null? https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:163: return context.getResources().getConfiguration().smallestScreenWidthDp; On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 16:03:07, jdduke wrote: > > On 2015/07/15 06:38:47, boliu wrote: > > > I wonder if this can ever change. Maybe it's ok to cache the value in the > > > constructor. > > > > > > Throwing exception is not ok. That's the same as crashing in a > @CalledByNative > > > method, and an object going away under a weak pointer has to be handled > > without > > > crashing. > > > > +1. It might also be worth investigating why this constant is needed, and if > > it's absolutely necessary. Regardless, even if the value does change I don't > > think we'd react to the change properly anyway, and I'd be OK storing it in > the > > constructor. > > Right, going ahead and storing the value in the constructor. > > Checking with skobes@ whether getSmallestDIPWidth can be removed > (it was added in https://codereview.chromium.org/28053002). You no longer need to keep mContextRef in this class then \o/ https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 06:38:47, boliu wrote: > > Looks like this is public only for ResourceManager construction? > > > > Is it possible to hide the java object implementation internal to ui layer, ie > > maybe ResourceManager can take a WindowAndroid pointer instead? > > That should work, I am not sure how we would handle a potential null ref to the > WindowAndroid Context over in the ResourceManager though (I guess with the code > of this PS we aren't handling a potential null reference either...). ResourceManager is created during construction, while we still haven't given up the strong reference to the context, right? Maybe the context should come directly from java ContentViewCore instead then? Context being null can only happen after java webview has been gc-ed, which usually is not during construction. > > Another problem is that resource_manager_impl_unittest.cc constructs a > ResourceManagerImpl which is easy to do now with > base::android::GetApplicationContext() but it might be difficult to pass a > WindowAndroid java object instead? (I guess we could create some empty > WindowAndroid object where only the Context is important...) https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:18: JNIEnv* env = base::android::AttachCurrentThread(); On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 06:38:47, boliu wrote: > > can use c++11 delegated constructor here I think? > > Done. Yeah, call other constructor directly https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/15 19:26:21, gsennton wrote: > On 2015/07/15 16:13:41, jdduke wrote: > > On 2015/07/15 16:03:07, jdduke wrote: > > > Silly question. Do we really need to hold a reference to a Java object here? > > It > > > doesn't look like we really support "live" updating of the object. > > > > > > Would it be any better to do something like: > > > > > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > > > > > And the Java side would do something like: > > > > > > @CalledByNative > > > private void requestUpdate(context, nativePtr) { > > > DeviceDisplayInfo info = new DeviceDisplayInfo(); > > > nativeUpdate(nativePtr, getFoo(), getBar()); > > > } > > > > > > Alternatively, you could make the Update() public and allow live updating, > but > > > it looks like you simply recreate the object when necessary so I don't > really > > > see the advantage of storing the temporary Java object. > > > > Not only will that cut out most of the JNI calls, we could also just use a > > static temporary Java variable to avoid the allocation for native updates > > (assuming we only call this on the UI thread? do we?). > > I am not sure what you mean by using a static temporary java variable here (and > how that would save allocations)? In the code snippet above, instead of "new DeviceDisplayInfo()" each call and creating something to be gc-ed, just use a static variable instead. Of course java DeviceDisplayInfo will need a update method too. This avoids gc churn. > > Oh, I am updating DeviceDisplayInfo in window_android.cc in a stupid way (by > creating a new native DeviceDisplayInfo and therefore also a new java object for > every update) -- we could just have a call on the native side updating all the > values through jni calls using the stored java object (unless we prefer your > suggestion)? I'm not sure how this is different from what is currently coded https://codereview.chromium.org/1144333004/diff/200001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:79: // For WebView to render properly on an external display through a Presentation we need to fetch Avoid mentioning webview. This is needed in chrome too if one day it decides to draw web contents in another display
https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 16:13:41, jdduke wrote: > > > On 2015/07/15 16:03:07, jdduke wrote: > > > > Silly question. Do we really need to hold a reference to a Java object > here? > > > It > > > > doesn't look like we really support "live" updating of the object. > > > > > > > > Would it be any better to do something like: > > > > > > > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > > > > > > > And the Java side would do something like: > > > > > > > > @CalledByNative > > > > private void requestUpdate(context, nativePtr) { > > > > DeviceDisplayInfo info = new DeviceDisplayInfo(); > > > > nativeUpdate(nativePtr, getFoo(), getBar()); > > > > } > > > > > > > > Alternatively, you could make the Update() public and allow live updating, > > but > > > > it looks like you simply recreate the object when necessary so I don't > > really > > > > see the advantage of storing the temporary Java object. > > > > > > Not only will that cut out most of the JNI calls, we could also just use a > > > static temporary Java variable to avoid the allocation for native updates > > > (assuming we only call this on the UI thread? do we?). > > > > I am not sure what you mean by using a static temporary java variable here > (and > > how that would save allocations)? > > In the code snippet above, instead of "new DeviceDisplayInfo()" each call and > creating something to be gc-ed, just use a static variable instead. Of course > java DeviceDisplayInfo will need a update method too. > > This avoids gc churn. > > > > > Oh, I am updating DeviceDisplayInfo in window_android.cc in a stupid way (by > > creating a new native DeviceDisplayInfo and therefore also a new java object > for > > every update) -- we could just have a call on the native side updating all the > > values through jni calls using the stored java object (unless we prefer your > > suggestion)? > > I'm not sure how this is different from what is currently coded Right, something like "private static DeviceDisplayInfo sTemporaryDeviceDisplayInfo". You'd create it on-demand, call an update method on it with the context, feed the updated values to the native object, then possibly "release" it to clear out any references. Would probably want to add a UI thread assert for the method called from native.
https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/16 15:09:03, jdduke wrote: > On 2015/07/16 14:36:56, boliu wrote: > > On 2015/07/15 19:26:21, gsennton wrote: > > > On 2015/07/15 16:13:41, jdduke wrote: > > > > On 2015/07/15 16:03:07, jdduke wrote: > > > > > Silly question. Do we really need to hold a reference to a Java object > > here? > > > > It > > > > > doesn't look like we really support "live" updating of the object. > > > > > > > > > > Would it be any better to do something like: > > > > > > > > > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > > > > > > > > > And the Java side would do something like: > > > > > > > > > > @CalledByNative > > > > > private void requestUpdate(context, nativePtr) { > > > > > DeviceDisplayInfo info = new DeviceDisplayInfo(); > > > > > nativeUpdate(nativePtr, getFoo(), getBar()); > > > > > } > > > > > > > > > > Alternatively, you could make the Update() public and allow live > updating, > > > but > > > > > it looks like you simply recreate the object when necessary so I don't > > > really > > > > > see the advantage of storing the temporary Java object. > > > > > > > > Not only will that cut out most of the JNI calls, we could also just use a > > > > static temporary Java variable to avoid the allocation for native updates > > > > (assuming we only call this on the UI thread? do we?). > > > > > > I am not sure what you mean by using a static temporary java variable here > > (and > > > how that would save allocations)? > > > > In the code snippet above, instead of "new DeviceDisplayInfo()" each call and > > creating something to be gc-ed, just use a static variable instead. Of course > > java DeviceDisplayInfo will need a update method too. > > > > This avoids gc churn. > > > > > > > > Oh, I am updating DeviceDisplayInfo in window_android.cc in a stupid way (by > > > creating a new native DeviceDisplayInfo and therefore also a new java object > > for > > > every update) -- we could just have a call on the native side updating all > the > > > values through jni calls using the stored java object (unless we prefer your > > > suggestion)? > > > > I'm not sure how this is different from what is currently coded > > Right, something like "private static DeviceDisplayInfo > sTemporaryDeviceDisplayInfo". You'd create it on-demand, call an update method > on it with the context, feed the updated values to the native object, then > possibly "release" it to clear out any references. Would probably want to add a > UI thread assert for the method called from native. Actually thinking more about this, having a static DeviceDisplayInfo doesn't work. The whole point of this exercise is have one DeviceDisplayInfo per context, to deal with external displays. So it can't just be static. But update only happen on rotation, which doesn't happen all the time. So a little gc churn isn't that bad.
On 2015/07/16 16:27:03, boliu wrote: > Actually thinking more about this, having a static DeviceDisplayInfo doesn't > work. The whole point of this exercise is have one DeviceDisplayInfo per > context, to deal with external displays. So it can't just be static. > > But update only happen on rotation, which doesn't happen all the time. So a > little gc churn isn't that bad. Maybe I wasn't clear, you'd effectively "obtain" and "recycle" the static DeviceDisplayInfo in the same atomic call from native. So there wouldn't be any kind of sharing, as it's obtained/recycled in the very same call. Somewhat like the various ringbuffer static objects Android uses that can be obtained/recycled (e.g., with Message.obtain()). In the obtain portion you set the context, in the recycle portion you clear the context.
On 2015/07/16 19:24:27, jdduke wrote:
> On 2015/07/16 16:27:03, boliu wrote:
> > Actually thinking more about this, having a static DeviceDisplayInfo doesn't
> > work. The whole point of this exercise is have one DeviceDisplayInfo per
> > context, to deal with external displays. So it can't just be static.
> >
> > But update only happen on rotation, which doesn't happen all the time. So a
> > little gc churn isn't that bad.
>
> Maybe I wasn't clear, you'd effectively "obtain" and "recycle" the static
> DeviceDisplayInfo in the same atomic call from native. So there wouldn't be
any
> kind of sharing, as it's obtained/recycled in the very same call. Somewhat
like
> the various ringbuffer static objects Android uses that can be
obtained/recycled
> (e.g., with Message.obtain()). In the obtain portion you set the context, in
the
> recycle portion you clear the context.
Another alternative that involves more boilerplate is to effectively duplicate
the member getter methods with static getter implementations, where you provided
the necessary args to the static getters. Then in the call from native you only
have to call the static methods. Don't really care which we do, and perhaps this
optimization is premature, but I'd feel more comfortable not allocating new Java
objects when we often stack allocate ("cheaply") the native DeviceDisplayInfo
counterpart.
On 2015/07/16 19:27:34, jdduke wrote:
> Don't really care which we do, and perhaps this
> optimization is premature, but I'd feel more comfortable not allocating new
Java
> objects when we often stack allocate ("cheaply") the native DeviceDisplayInfo
> counterpart.
Ahh that's a different concern. This chain of comments started on the *update*
method, which doesn't happen all that often.
Yeah, I missed the fact that ever native DeviceDisplayInfo creates a new java
object now. It's not premature. Someone deliberately fixed this with
shared_device_display_info before.
For DeviceDisplayInfo that should be tied to an activity, you've already
switched to using window_android->GetDeviceDisplayInfo(), so there are no new
allocations there.
So the only case is all the places that just creates a DeviceDisplayInfo with no
parameters, which just fallback to the global application context. So the best
thing here would be to find the global WindowAndroid and get the
DeviceDisplayInfo from there instead. But alas things are not perfect and fixing
all callsites is hard, and what Jared suggested is probably the next best
option. Basically make native and java DeviceDisplayInfo object lifetimes
completley independent of each other.
content/public/android/java/src/org/chromium/content/browser/ lgtm assuming the tests pass and the Screen Orientation API is still working in JB. https://codereview.chromium.org/1144333004/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java (right): https://codereview.chromium.org/1144333004/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:165: Collections.newSetFromMap(new WeakHashMap<ScreenOrientationListener, Boolean>()); nit: could you rename + add comment. These are not just listeners, these are accurate/eager listeners.
This hasn't been touched in ages :/ sorry about that. Updated the major comments. https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 06:38:46, boliu wrote: > > > This should be per context, instead of per contentviewcore > > > > I don't quite understand this, aren't we only holding one context in > > contentviewcore anyway? (and how do you mean this would look if it was per > > context?) > > Many ContentViewCore can have the same context, in which case they should share > the same ScreenOrientationListener. > > One hacky way to implement it is keep a static WeakHashMap<Context, > ScreenOrientationListener>. Cleaner concept would be to move > ScreenOrientationListener entirely into WindowAndroid. Is there any way for us to guarantee that we can call ScreenOrientationListener.removeObserver from WindowAndroid? E.g. can we do it in WindowAndroid.destroy()? In ContentViewCore we do it not only in destroy() but also in OnDetachedFromWindow to make sure it is always called. AFAICT some (if not all) of the changes I have proposed to ScreenOrientationListener might not be needed because of the way we listen for display updates -- The first way we do it is to call Context.registerComponentCallbacks which for Context.java runs getApplicationContext().registerComponentCallbacks(), see https://cs.corp.google.com/#android/frameworks/base/core/java/android/content... (though Application.java overrides this method to customize this). The second way is to get the DisplayManager for the context and call registerDisplayListener on it: https://cs.corp.google.com/#android/frameworks/base/core/java/android/hardwar... (and that function is implemented through register listeners to a global DisplayManager which should be the same for all different Contexts). So if this won't change in the future we could store application contexts in ScreenOrientationListener (rather than weak references to display/activity contexts) and thus we would be able unregister our listeners even when our weakly referenced contexts have been destructed. Does this seem ok? (I am unsure about relying on the internal implementation of Android here). https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:62: this(activity, true, activity.getApplicationContext()); On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 06:38:46, boliu wrote: > > > Should this just be activity? Why is displayContext the application context > > > here? > > > > > > And if that's the case, activity and displayContext are always equal, so > don't > > > need to add the new param below. > > > > activity and displayContext are not always equal (see AwContents.java) -- > > especially, in the case where we target an external display the two will not > be > > equal since then the displayContext will have been create through > > Context.createDisplayContext(Display). > > Wait. How do you get to that display context from an activity then? I hope it's > not activity.getApplicationContext. It's not the activity itself? > > > I guess that means that we should never create an ActivityWindowAndroid when > > targeting an external display (since we only do that in AwContents when our > > Context is an instance of Activity) and in that case we only need to worry > about > > displayContext in WindowAndroid and not in ActivityWindowAndroid? > > Not including Services, there are only two kinds of context: the global > ApplicationContext, and we create WindowAndroid for that, and Activity, and we > create ActivityWindowAndroid for that. > > So shouldn't it possible for an activity to belong to an external display? In > that case, we should create an ActivityWindowAndroid, but like I ask above, I > don't know what the "display context" should be. In PS11 we pass the activity to the WindowAndroid ctor which means that the activity will be used as the display context. So if an Activity can be made to target a different display it will now do so properly (and if it can't then there was no problem regarding Activities in the first place). ... https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:39: if (context == null) On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 06:38:47, boliu wrote: > > > Pass a strong ref in constructor, then this can't ever happen > > > > Unless the incoming context is null because we retrieved it from WindowAndroid > > which only holds a weak reference to its display context (and therefore might > > pass null)? > > You can check for null before passing it into DeviceDisplayInfo then. And if it > is null, you'll have to handle that correctly too. Make a dummy > DeviceDisplayInfo, or check caller can handle null? Right, will check this in WindowAndroid::UpdateDeviceDisplayInfo and just use the default DeviceDisplayInfo ctor if the context is null. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:163: return context.getResources().getConfiguration().smallestScreenWidthDp; On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 16:03:07, jdduke wrote: > > > On 2015/07/15 06:38:47, boliu wrote: > > > > I wonder if this can ever change. Maybe it's ok to cache the value in the > > > > constructor. > > > > > > > > Throwing exception is not ok. That's the same as crashing in a > > @CalledByNative > > > > method, and an object going away under a weak pointer has to be handled > > > without > > > > crashing. > > > > > > +1. It might also be worth investigating why this constant is needed, and if > > > it's absolutely necessary. Regardless, even if the value does change I don't > > > think we'd react to the change properly anyway, and I'd be OK storing it in > > the > > > constructor. > > > > Right, going ahead and storing the value in the constructor. > > > > Checking with skobes@ whether getSmallestDIPWidth can be removed > > (it was added in https://codereview.chromium.org/28053002). > > You no longer need to keep mContextRef in this class then \o/ will remove mContextRef Regarding getSmallestDIPWidth, according to skobes@ the value is needed but can probably be calculated in the following way int smallestDIPWidth = min(info.GetPhysicalDisplayHeight(), info.GetPhysicalDisplayWidth()) / info.GetDIPScale(); https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/07/16 14:36:56, boliu wrote: > On 2015/07/15 19:26:21, gsennton wrote: > > On 2015/07/15 06:38:47, boliu wrote: > > > Looks like this is public only for ResourceManager construction? > > > > > > Is it possible to hide the java object implementation internal to ui layer, > ie > > > maybe ResourceManager can take a WindowAndroid pointer instead? > > > > That should work, I am not sure how we would handle a potential null ref to > the > > WindowAndroid Context over in the ResourceManager though (I guess with the > code > > of this PS we aren't handling a potential null reference either...). > > ResourceManager is created during construction, while we still haven't given up > the strong reference to the context, right? > > Maybe the context should come directly from java ContentViewCore instead then? > > Context being null can only happen after java webview has been gc-ed, which > usually is not during construction. > > > > > Another problem is that resource_manager_impl_unittest.cc constructs a > > ResourceManagerImpl which is easy to do now with > > base::android::GetApplicationContext() but it might be difficult to pass a > > WindowAndroid java object instead? (I guess we could create some empty > > WindowAndroid object where only the Context is important...) > I'm not sure how we would pass it from the ContentViewCore, this is constructed in compositor_impl_android.cc from where we have access to a NativeWindow. How would you get the context directly from the ContentViewCore? Going back to your first suggestion. Could we just have two constructors in the native ResourceManagerImpl, one of which takes a WindowAndroid and one which doesn't take anything but instead passes the application context to a special java-side createForTests method in ResourceManager.java? So that the create(WindowAndroid, ...) method creates a new ResourceManager through passing the display context of the WindowAndroid while the createForTests(Context, ...) simply passes the given (application)context. I assume that we would make WindowAndroid.getDisplayContext() package private then, though WindowAndroid resides in org.chromium.ui.base while ResourceManager resides in org.chromium.ui.resources. Is there some way to make this private to the whole ui-package rather than only org.chromium.ui.base? https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/1144333004/diff/180001/ui/gfx/android/device_... ui/gfx/android/device_display_info.cc:28: UpdateDisplayInfo( On 2015/07/16 16:27:03, boliu wrote: > On 2015/07/16 15:09:03, jdduke wrote: > > On 2015/07/16 14:36:56, boliu wrote: > > > On 2015/07/15 19:26:21, gsennton wrote: > > > > On 2015/07/15 16:13:41, jdduke wrote: > > > > > On 2015/07/15 16:03:07, jdduke wrote: > > > > > > Silly question. Do we really need to hold a reference to a Java object > > > here? > > > > > It > > > > > > doesn't look like we really support "live" updating of the object. > > > > > > > > > > > > Would it be any better to do something like: > > > > > > > > > > > > Java_DeviceDisplayInfo_requestUpdate(env, context, this); > > > > > > > > > > > > And the Java side would do something like: > > > > > > > > > > > > @CalledByNative > > > > > > private void requestUpdate(context, nativePtr) { > > > > > > DeviceDisplayInfo info = new DeviceDisplayInfo(); > > > > > > nativeUpdate(nativePtr, getFoo(), getBar()); > > > > > > } > > > > > > > > > > > > Alternatively, you could make the Update() public and allow live > > updating, > > > > but > > > > > > it looks like you simply recreate the object when necessary so I don't > > > > really > > > > > > see the advantage of storing the temporary Java object. > > > > > > > > > > Not only will that cut out most of the JNI calls, we could also just use > a > > > > > static temporary Java variable to avoid the allocation for native > updates > > > > > (assuming we only call this on the UI thread? do we?). > > > > > > > > I am not sure what you mean by using a static temporary java variable here > > > (and > > > > how that would save allocations)? > > > > > > In the code snippet above, instead of "new DeviceDisplayInfo()" each call > and > > > creating something to be gc-ed, just use a static variable instead. Of > course > > > java DeviceDisplayInfo will need a update method too. > > > > > > This avoids gc churn. > > > > > > > > > > > Oh, I am updating DeviceDisplayInfo in window_android.cc in a stupid way > (by > > > > creating a new native DeviceDisplayInfo and therefore also a new java > object > > > for > > > > every update) -- we could just have a call on the native side updating all > > the > > > > values through jni calls using the stored java object (unless we prefer > your > > > > suggestion)? > > > > > > I'm not sure how this is different from what is currently coded > > > > Right, something like "private static DeviceDisplayInfo > > sTemporaryDeviceDisplayInfo". You'd create it on-demand, call an update method > > on it with the context, feed the updated values to the native object, then > > possibly "release" it to clear out any references. Would probably want to add > a > > UI thread assert for the method called from native. > > Actually thinking more about this, having a static DeviceDisplayInfo doesn't > work. The whole point of this exercise is have one DeviceDisplayInfo per > context, to deal with external displays. So it can't just be static. > > But update only happen on rotation, which doesn't happen all the time. So a > little gc churn isn't that bad. We do have a DeviceDisplayInfo stored in WindowAndroid so as long as we make sure that it updates itself by reusing its stored java object all the places using WindowAndroid should be fine. What I meant above by me updating DeviceDisplayInfo in a stupid way in WindowAndroid is that I create a new DeviceDisplayInfo (both native and java) object on each update, i.e. I call new DeviceDisplayInfo(Context) instead of device_display_info.FetchDisplayInfo(...). We could have a static DeviceDisplayInfo object (or some other static object used from DeviceDisplayInfo) taking care of the ApplicationContext case (the default constructor case here), but I think we would have to implement this with a special case for screen listening just like before this CL?
No new code to look at, so only replied to comments. Also I don't remember all the stuff I said months ago.. This patch is huge. Can this be broken into smaller patches? https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/10/20 13:27:12, gsennton wrote: > On 2015/07/16 14:36:56, boliu wrote: > > On 2015/07/15 19:26:21, gsennton wrote: > > > On 2015/07/15 06:38:46, boliu wrote: > > > > This should be per context, instead of per contentviewcore > > > > > > I don't quite understand this, aren't we only holding one context in > > > contentviewcore anyway? (and how do you mean this would look if it was per > > > context?) > > > > Many ContentViewCore can have the same context, in which case they should > share > > the same ScreenOrientationListener. > > > > One hacky way to implement it is keep a static WeakHashMap<Context, > > ScreenOrientationListener>. Cleaner concept would be to move > > ScreenOrientationListener entirely into WindowAndroid. > > Is there any way for us to guarantee that we can call > ScreenOrientationListener.removeObserver from WindowAndroid? E.g. can we do it > in WindowAndroid.destroy()? In ContentViewCore we do it not only in destroy() > but also in OnDetachedFromWindow to make sure it is always called. > Those don't have to move? Change ScreenOrientationListener.getInstance().removeObserver(this); to mWindowAndroid.getScreenOrientationListener().removeObserve(this); or something like that? > > AFAICT some (if not all) of the changes I have proposed to > ScreenOrientationListener might not be needed because of the way we listen for > display updates -- > The first way we do it is to call Context.registerComponentCallbacks which for > Context.java runs getApplicationContext().registerComponentCallbacks(), see > https://cs.corp.google.com/#android/frameworks/base/core/java/android/content... > (though Application.java overrides this method to customize this). > The second way is to get the DisplayManager for the context and call > registerDisplayListener on it: > https://cs.corp.google.com/#android/frameworks/base/core/java/android/hardwar... > (and that function is implemented through register listeners to a global > DisplayManager which should be the same for all different Contexts). Interesting. So android has no way to observer orientation change of the external display in any way? Interesting... However, that just mean CVC connected to an external window should *not* observer any orientation change events? No idea how to actually implement that though :/ > So if this won't change in the future we could store application contexts in > ScreenOrientationListener (rather than weak references to display/activity > contexts) and thus we would be able unregister our listeners even when our > weakly referenced contexts have been destructed. Does this seem ok? (I am unsure > about relying on the internal implementation of Android here). https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:62: this(activity, true, activity.getApplicationContext()); On 2015/10/20 13:27:12, gsennton wrote: > On 2015/07/16 14:36:56, boliu wrote: > > On 2015/07/15 19:26:21, gsennton wrote: > > > On 2015/07/15 06:38:46, boliu wrote: > > > > Should this just be activity? Why is displayContext the application > context > > > > here? > > > > > > > > And if that's the case, activity and displayContext are always equal, so > > don't > > > > need to add the new param below. > > > > > > activity and displayContext are not always equal (see AwContents.java) -- > > > especially, in the case where we target an external display the two will not > > be > > > equal since then the displayContext will have been create through > > > Context.createDisplayContext(Display). > > > > Wait. How do you get to that display context from an activity then? I hope > it's > > not activity.getApplicationContext. It's not the activity itself? > > > > > I guess that means that we should never create an ActivityWindowAndroid when > > > targeting an external display (since we only do that in AwContents when our > > > Context is an instance of Activity) and in that case we only need to worry > > about > > > displayContext in WindowAndroid and not in ActivityWindowAndroid? > > > > Not including Services, there are only two kinds of context: the global > > ApplicationContext, and we create WindowAndroid for that, and Activity, and we > > create ActivityWindowAndroid for that. > > > > So shouldn't it possible for an activity to belong to an external display? In > > that case, we should create an ActivityWindowAndroid, but like I ask above, I > > don't know what the "display context" should be. > > In PS11 we pass the activity to the WindowAndroid ctor which means that the > activity will be used as the display context. > > So if an Activity can be made to target a different display it will now do so > properly (and if it can't then there was no problem regarding Activities in the > first place). > > ... Looks good. https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:39: if (context == null) On 2015/10/20 13:27:12, gsennton wrote: > On 2015/07/16 14:36:56, boliu wrote: > > On 2015/07/15 19:26:21, gsennton wrote: > > > On 2015/07/15 06:38:47, boliu wrote: > > > > Pass a strong ref in constructor, then this can't ever happen > > > > > > Unless the incoming context is null because we retrieved it from > WindowAndroid > > > which only holds a weak reference to its display context (and therefore > might > > > pass null)? > > > > You can check for null before passing it into DeviceDisplayInfo then. And if > it > > is null, you'll have to handle that correctly too. Make a dummy > > DeviceDisplayInfo, or check caller can handle null? > > Right, will check this in WindowAndroid::UpdateDeviceDisplayInfo and just use > the default DeviceDisplayInfo ctor if the context is null. "use the default DeviceDisplayInfo" sounds wrong, but then maybe it doesn't matter much since this can only be null momentarily during shutdown. So add a comment if you decide to do that. https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/10/20 13:27:12, gsennton wrote: > On 2015/07/16 14:36:56, boliu wrote: > > On 2015/07/15 19:26:21, gsennton wrote: > > > On 2015/07/15 06:38:47, boliu wrote: > > > > Looks like this is public only for ResourceManager construction? > > > > > > > > Is it possible to hide the java object implementation internal to ui > layer, > > ie > > > > maybe ResourceManager can take a WindowAndroid pointer instead? > > > > > > That should work, I am not sure how we would handle a potential null ref to > > the > > > WindowAndroid Context over in the ResourceManager though (I guess with the > > code > > > of this PS we aren't handling a potential null reference either...). > > > > ResourceManager is created during construction, while we still haven't given > up > > the strong reference to the context, right? > > > > Maybe the context should come directly from java ContentViewCore instead then? > > > > Context being null can only happen after java webview has been gc-ed, which > > usually is not during construction. > > > > > > > > Another problem is that resource_manager_impl_unittest.cc constructs a > > > ResourceManagerImpl which is easy to do now with > > > base::android::GetApplicationContext() but it might be difficult to pass a > > > WindowAndroid java object instead? (I guess we could create some empty > > > WindowAndroid object where only the Context is important...) > > > > I'm not sure how we would pass it from the ContentViewCore, this is constructed > in compositor_impl_android.cc from where we have access to a NativeWindow. How > would you get the context directly from the ContentViewCore? Got lost in the pronoun game. What's "it" and "this"? > > Going back to your first suggestion. Could we just have two constructors in the > native ResourceManagerImpl, one of which takes a WindowAndroid and one which > doesn't take anything but instead passes the application context to a special > java-side createForTests method in ResourceManager.java? So that the > create(WindowAndroid, ...) method creates a new ResourceManager through passing > the display context of the WindowAndroid while the createForTests(Context, ...) > simply passes the given (application)context. > > I assume that we would make WindowAndroid.getDisplayContext() package private > then, though WindowAndroid resides in org.chromium.ui.base while ResourceManager > resides in org.chromium.ui.resources. Is there some way to make this private to > the whole ui-package rather than only org.chromium.ui.base? Nope.. https://codereview.chromium.org/1144333004/diff/200001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1144333004/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:141: mApplicationContext = context.getApplicationContext(); probably at a comment there that context may be the activity context, so can't hold a strong ref to it from here
Yup, the patch is huge... I'll check if I can split it up somehow (though I think most of the stuff depends on each other so smaller patches would also have dependencies on each other). https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/10/20 15:07:22, boliu wrote: > On 2015/10/20 13:27:12, gsennton wrote: > > On 2015/07/16 14:36:56, boliu wrote: > > > On 2015/07/15 19:26:21, gsennton wrote: > > > > On 2015/07/15 06:38:46, boliu wrote: > > > > > This should be per context, instead of per contentviewcore > > > > > > > > I don't quite understand this, aren't we only holding one context in > > > > contentviewcore anyway? (and how do you mean this would look if it was per > > > > context?) > > > > > > Many ContentViewCore can have the same context, in which case they should > > share > > > the same ScreenOrientationListener. > > > > > > One hacky way to implement it is keep a static WeakHashMap<Context, > > > ScreenOrientationListener>. Cleaner concept would be to move > > > ScreenOrientationListener entirely into WindowAndroid. > > > > Is there any way for us to guarantee that we can call > > ScreenOrientationListener.removeObserver from WindowAndroid? E.g. can we do it > > in WindowAndroid.destroy()? In ContentViewCore we do it not only in destroy() > > but also in OnDetachedFromWindow to make sure it is always called. > > > > Those don't have to move? Change > > ScreenOrientationListener.getInstance().removeObserver(this); > > to > > mWindowAndroid.getScreenOrientationListener().removeObserve(this); > > or something like that? > Ah, we could have the WindowAndroids be ScreenOrientationObservers and just add them through calling mScreenOrientationListener().addObserver(mWindowAndroid) .... then if we would happen to add the same WindowAndroid several times we would just hit https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... and thus not add the observer unnecessarily. > > > > AFAICT some (if not all) of the changes I have proposed to > > ScreenOrientationListener might not be needed because of the way we listen for > > display updates -- > > The first way we do it is to call Context.registerComponentCallbacks which for > > Context.java runs getApplicationContext().registerComponentCallbacks(), see > > > https://cs.corp.google.com/#android/frameworks/base/core/java/android/content... > > (though Application.java overrides this method to customize this). > > The second way is to get the DisplayManager for the context and call > > registerDisplayListener on it: > > > https://cs.corp.google.com/#android/frameworks/base/core/java/android/hardwar... > > (and that function is implemented through register listeners to a global > > DisplayManager which should be the same for all different Contexts). > > Interesting. So android has no way to observer orientation change of the > external display in any way? Interesting... If I interpret this correctly there was no way to observe orientation changes of external displays before android version 17 because that is when DisplayManager and Presentation were added. The callback we use from DisplayManager looks like this: void onDisplayChanged(int displayId); so it should support different displays. > > However, that just mean CVC connected to an external window should *not* > observer any orientation change events? No idea how to actually implement that > though :/ > > > So if this won't change in the future we could store application contexts in > > ScreenOrientationListener (rather than weak references to display/activity > > contexts) and thus we would be able unregister our listeners even when our > > weakly referenced contexts have been destructed. Does this seem ok? (I am > unsure > > about relying on the internal implementation of Android here). > https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/10/20 15:07:22, boliu wrote: > On 2015/10/20 13:27:12, gsennton wrote: > > On 2015/07/16 14:36:56, boliu wrote: > > > On 2015/07/15 19:26:21, gsennton wrote: > > > > On 2015/07/15 06:38:47, boliu wrote: > > > > > Looks like this is public only for ResourceManager construction? > > > > > > > > > > Is it possible to hide the java object implementation internal to ui > > layer, > > > ie > > > > > maybe ResourceManager can take a WindowAndroid pointer instead? > > > > > > > > That should work, I am not sure how we would handle a potential null ref > to > > > the > > > > WindowAndroid Context over in the ResourceManager though (I guess with the > > > code > > > > of this PS we aren't handling a potential null reference either...). > > > > > > ResourceManager is created during construction, while we still haven't given > > up > > > the strong reference to the context, right? > > > > > > Maybe the context should come directly from java ContentViewCore instead > then? > > > > > > Context being null can only happen after java webview has been gc-ed, which > > > usually is not during construction. > > > > > > > > > > > Another problem is that resource_manager_impl_unittest.cc constructs a > > > > ResourceManagerImpl which is easy to do now with > > > > base::android::GetApplicationContext() but it might be difficult to pass a > > > > WindowAndroid java object instead? (I guess we could create some empty > > > > WindowAndroid object where only the Context is important...) > > > > > > > I'm not sure how we would pass it from the ContentViewCore, this is > constructed > > in compositor_impl_android.cc from where we have access to a NativeWindow. How > > would you get the context directly from the ContentViewCore? > > Got lost in the pronoun game. What's "it" and "this"? > > > > > Going back to your first suggestion. Could we just have two constructors in > the > > native ResourceManagerImpl, one of which takes a WindowAndroid and one which > > doesn't take anything but instead passes the application context to a special > > java-side createForTests method in ResourceManager.java? So that the > > create(WindowAndroid, ...) method creates a new ResourceManager through > passing > > the display context of the WindowAndroid while the createForTests(Context, > ...) > > simply passes the given (application)context. > > > > I assume that we would make WindowAndroid.getDisplayContext() package private > > then, though WindowAndroid resides in org.chromium.ui.base while > ResourceManager > > resides in org.chromium.ui.resources. Is there some way to make this private > to > > the whole ui-package rather than only org.chromium.ui.base? > > Nope.. Ah, sorry: I'm not sure how we would pass it = 'the display Context' from CVC, this = 'the resource manager' is constructed in compositor_impl_android.cc from where we have access to a NativeWindow... [and the rest of the comment should be understandable :)]
https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; On 2015/10/20 18:46:44, gsennton wrote: > Ah, we could have the WindowAndroids be ScreenOrientationObservers and just add > them through calling > mScreenOrientationListener().addObserver(mWindowAndroid) .... > then if we would happen to add the same WindowAndroid several times we would > just hit The issue to be careful with supporting adding he same observer multiple times is you have to support removing the observer multiple times too. I recommend solving this in WindowAndroid, not the observer pattern. > If I interpret this correctly there was no way to observe orientation changes of > external displays before android version 17 because that is when DisplayManager > and Presentation were added. > The callback we use from DisplayManager looks like this: > > void onDisplayChanged(int displayId); > > so it should support different displays. Hmm, that implies orientation observer need to be re-implemented using DisplayManager.DisplayListener instead to do this? Webivew only need to run on 21 and up, but chrome still supports back to 16 I think. So need to write in a way that gracefully fall back to old implementation on older devices. https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/10/20 18:46:44, gsennton wrote: > > > I'm not sure how we would pass it from the ContentViewCore, this is > > constructed > > > in compositor_impl_android.cc from where we have access to a NativeWindow. > How > > > would you get the context directly from the ContentViewCore? > > > > Got lost in the pronoun game. What's "it" and "this"? > > Ah, sorry: > > I'm not sure how we would pass it = 'the display Context' from CVC, this = 'the > resource manager' is constructed in compositor_impl_android.cc from where we > have access to a NativeWindow... [and the rest of the comment should be > understandable :)] > native WindowAndroid is already passed to CompositorImpl constructor. It's just typedef-ed to gfx::NativeWindow: typedef ui::WindowAndroid* NativeWindow; Yay?
Ok, so I've started to split this up: I have one change in https://codereview.chromium.org/1419843002 that allows the use of any kind of Context in WindowAndroid (rather than just an application context). With that change I also added everything that is fixed by using that context from WindowAndroid. In https://codereview.chromium.org/1409833004 the GetDeviceDisplayInfo() function is added to WindowAndroid (at the moment it still uses the global Shared Device Display Info) and everything that depends on that function is added as well. What I have not uploaded yet is the change to use the current context (not application context) for fetching Device Display Info and caching that in WindowAndroid. So the second CL above (containing GetDeviceDisplayInfo) should not actually change anything visually until the final CL has landed. That final change depends on the two CLs above (more specifically it depends on any changes to WindowAndroid* and DeviceDisplayInfo) but should be a lot simpler than the current one now that the above two CLs have been split out. Sorry for using this huge CL. This work would have been easier/faster if I had broken out the above CLs earlier... https://codereview.chromium.org/1144333004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1144333004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:605: private ScreenOrientationListener mScreenOrientationListener; Right, the removal is problematic... We already use DisplayListener in ScreenOrientationListener :). Or do you mean that we should use it in some other way? https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/1144333004/diff/180001/ui/android/window_andr... ui/android/window_android.h:76: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/10/21 15:54:59, boliu wrote: > On 2015/10/20 18:46:44, gsennton wrote: > > > > I'm not sure how we would pass it from the ContentViewCore, this is > > > constructed > > > > in compositor_impl_android.cc from where we have access to a NativeWindow. > > How > > > > would you get the context directly from the ContentViewCore? > > > > > > Got lost in the pronoun game. What's "it" and "this"? > > > > Ah, sorry: > > > > I'm not sure how we would pass it = 'the display Context' from CVC, this = > 'the > > resource manager' is constructed in compositor_impl_android.cc from where we > > have access to a NativeWindow... [and the rest of the comment should be > > understandable :)] > > > > native WindowAndroid is already passed to CompositorImpl constructor. It's just > typedef-ed to gfx::NativeWindow: > > typedef ui::WindowAndroid* NativeWindow; > > Yay? Yes, but the problem is that GetJavaDisplayContext is public and only used from compositor_impl_android.cc and if we want GetJavaDisplayContext to be private we can't use it in compositor_impl_android.cc. You suggested instead passing a java WindowAndroid to ResourceManager and from the ResourceManager fetch the display context in a way that would be internal to the ui layer. But if we make WindowAndroid.getDisplayContext() package private we still can't access it from ResourceManager since WindowAndroid resides in org.chromium.ui.base and ResourceManager resides in org.chromium.ui.resources. Anyway, I think it is easier / clearer to continue this discussion over in https://codereview.chromium.org/1419843002 :)
Looking at the whole DeviceDisplayInfo logic again, it seems unnecessary to store one DDI object per WindowAndroid - there should only be one DDI per display, right? I am tempted to have a static map [displayID -> DeviceDisplayInfo] that would be initiated the first time we need a DeviceDisplayInfo (using the provided context to fetch the global Displaymanager, and then would be updated by listening to DisplayManager changes (onDisplayAdded/Removed/Changed). Does this sound like something we should do now? I.e. can we have many WindowAndroid per display and is it costly to hold a DeviceDisplayInfo? (The alternative I'm thinking of would be to store one DeviceDisplayInfo per WindowAndroid and keep a SharedDeviceDisplayInfo for all the places where we want to access a DeviceDisplayInfo using the application context. |
