Add DeviceDisplayInfo getter in WindowAndroid.

ui/android/window_android.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/android/window_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
 #include "jni/WindowAndroid_jni.h"
 #include "ui/android/window_android_compositor.h"
 #include "ui/android/window_android_observer.h"
+#include "ui/gfx/android/device_display_info.h"
 
 namespace ui {
 
 using base::android::AttachCurrentThread;
 using base::android::ScopedJavaLocalRef;
 
 WindowAndroid::WindowAndroid(JNIEnv* env, jobject obj) : compositor_(NULL) {
   java_window_.Reset(env, obj);
+  // TODO(gsennton) have our device display info depend on our (java) display
+  // context
+  device_display_info_ = make_scoped_ptr(new gfx::DeviceDisplayInfo());

[Ted C, 2015/11/05 23:15:58]

DeviceDisplayInfo seems very tied to the application context.  If we change it
to support holding onto a Context, it has the same memory leak possibilities of
WindowAndroid.

[gsennton, 2015/11/06 16:52:14]

Right, so the idea would be to no longer hold on to any Context in
DeviceDisplayInfo.

AFAICT there are two ways we could do this:

1. Pass the (presentation) Context to DeviceDisplayInfo so that it can fetch a
WindowManager which DeviceDisplayInfo can hold on to. The WindowManager is tied
to a certain display so we can fetch display metrics from that.

2. Let DeviceDisplayInfo hold on to a DisplayManager (fetched through the
presentation context or the application context, shouldn't matter) and pass the
display ID (associated with the presentation context) to DeviceDisplayInfo as a
reference to the display we are interested in - and then fetch display info
through the DisplayManager.getDisplay(displayID).

[Ted C, 2015/11/25 19:35:52]

Again, I don't know what the lifecycles of either WindowManager or
DisplayManager are to know whether they are actually better than
Context.  Do they transitively hold onto a Context ref by any means
that we don't expect?

If it does, then it could potentially causes leaks on webview if we
aren't clearing the references where appropriate.

If the java side of WindowAndroid was in charge of creating display
infos, then it could just pass a weak ref to the java class and it
could use the same leaky behavior as WindowAndroid.

If sievers@ and maybe boliu@ don't think this is an issue than I can
be convinced, but as this code is in areas I'm not familiar with I 
have to operate on my standard level of skepticism.

[gsennton, 2015/12/02 15:37:05]

Looking at this again, I don't think we should hold references to any
non-trivial Objects (fetched through our Context) beyond the Context lifetime
(since it is unsustainable to keep track of which Objects are OK to leak and we
might even leak the Context itself through some chain of references). This also
means that we should probably not hold a reference to the display/activity
context Resources in ResourceManager as we do since
https://codereview.chromium.org/1419843002/.  
We should probably fetch non-display resources from the application context
rather than the current context in that class.

Since we shouldn't hold strong references to objects fetched through our Context
this limits our alternatives in DeviceDisplayInfo. We could possibly use the
DisplayManager fetched from the application context (not the
activity/presentation context!) to fetch info about different displays. And then
store a displayID in each WindowAndroid (fetched through its display/activity
context). This display ID would be used to fetch the correct display for our
context.

[Ted C, 2015/12/03 04:23:56]

I think the best approach in many of these cases would to just hold a reference
to the WindowAndroid.  Then fetch the context as needed (handling the case
where it has gone away, which in non-webview cases indicates a coding error
BTW) and update what is necessary.

But if we can't always get a WindowAndroid because of the screen apis, then
I would try to keep a weak ref to the context in the devicedisplayinfo directly
and handle it nicely if it goes away.  If you keep a weak ref to only the
context,
then you'd need to refetch any dependent variables (services, resources, etc..)
but hopefully we could cache the final values and discard the intermediary
bits.

[boliu, 2015/12/08 08:21:54]

+1
I think you mean DeviceDisplayInfo instead of gfx::Screen here?
DeviceDisplayInfo is an android type, so we probably pass in whatever we need.
I think the only time the display values can change is on orientation change. So
we can cache everything as long as we listen orientation change events.

All this is getting a bit head of this patch :)

[gsennton, 2015/12/10 12:29:54]

Yeah, the idea with this patch is to only add the interface for getting
DeviceDisplayInfo from WindowAndroid, so that we can then focus on the
DeviceDisplayInfo implementation changes in the next patch (that implementation
includes changing the values cached in DeviceDisplayInfo on orientation
changes).

 }
 
 void WindowAndroid::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }
 
 ScopedJavaLocalRef<jobject> WindowAndroid::GetJavaObject() {
   return base::android::ScopedJavaLocalRef<jobject>(java_window_);
 }
 
@@ skipping 92 matching lines @@
 }
 
 bool WindowAndroid::CanRequestPermission(const std::string& permission) {
   JNIEnv* env = AttachCurrentThread();
   return Java_WindowAndroid_canRequestPermission(
       env,
       GetJavaObject().obj(),
       base::android::ConvertUTF8ToJavaString(env, permission).obj());
 }
 
+const gfx::DeviceDisplayInfo& WindowAndroid::GetDeviceDisplayInfo() {

[AKV, 2015/11/04 14:20:48]

DCHECK(device_display_info_) ?

[boliu, 2015/12/08 08:21:54]

or just make device_display_info_ const

[gsennton, 2015/12/10 12:29:54]

I don't think we want to make it const since it should be updated on orientation
changes.

[boliu, 2015/12/10 16:38:20]

Pointer can be const, pointee (?) don't need to be const (unless
DeviceDisplayInfo is immutable)

[gsennton, 2015/12/14 18:10:32]

Done.

+  return *device_display_info_;
+}
+
 // ----------------------------------------------------------------------------
 // Native JNI methods
 // ----------------------------------------------------------------------------
 
 jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   WindowAndroid* window = new WindowAndroid(env, obj);
   return reinterpret_cast<intptr_t>(window);
 }
 
 }  // namespace ui