Add DeviceDisplayInfo getter in WindowAndroid.

ui/android/screen_android.cc

 // Copyright (c) 2012 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/gfx/screen.h"
 
 #include "base/logging.h"
+#include "ui/android/view_android.h"
+#include "ui/android/window_android.h"
 #include "ui/gfx/android/device_display_info.h"
 #include "ui/gfx/display.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
 namespace gfx {

[sadrul, 2015/11/03 18:14:27]

namespace ui

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

screen_android.h header is still under gfx. This is not completely wrong since
things like gfx::NativeView/NativeWindow are defined to
ViewAndroid/WindowAndroid under ui/android. And they are under ui/android
presumably because they talk through to java classes in jni.

I guess ViewAndroid/WindowAndroid should live in like ui/gfx/android, but
WindowAndroid.cc has more dependencies on ui/android/window_android_compositor.h
and ui/android/window_android_observer.h, so it's the same problem.

Given the current state of things, this is not that much worse. Not sure if
owners actually agree with this.

[Ted C, 2015/12/13 00:29:38]

I would follow what other files of similar nature due.  I don't honestly care.

[no sievers, 2016/02/29 18:16:03]

Note that ui/android/java containts java code for all of ui/...

 
+gfx::Display DisplayFromDeviceDisplayInfo(

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

should this be in an anonymous namespace?

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

Yup, will update in next PS.

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

Done.

+    const gfx::DeviceDisplayInfo& device_info) {
+  const float device_scale_factor = device_info.GetDIPScale();
+  // Note: GetPhysicalDisplayWidth/Height() does not subtract window
+  // decorations etc. Use it instead of GetDisplayWidth/Height() when
+  // available.
+  const gfx::Rect bounds_in_pixels =

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

existing code but a bit weird, all display on android are at the origin, so they
are like on top of each other?

I guess nothing really cares about that fact

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

Up until N we haven't been able to move applications around anyway so the
location of the 'Display' shouldn't matter? (if that is what you are referring
to).

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

Think of a desktop with two monitors, each 1000x1000 (for simplicity), placed
side-by-side, forming one virtual desktop.
gfx::Display here corresponds to the physical monitors, so you'd have two of
them, one at 0,0 1000x1000, and one at 1000,0 1000x1000

FWIW gfx::Screen corresponds to the "virtual desktop" part, at least on aura
desktop.

So if android doesn't supporting moving windows between displays, then that's
like having one Screen per Display? Then 0,0 is actually correct. Either way,
don't need to worry about it too much until it becomes a problem I think.

+      gfx::Rect(device_info.GetPhysicalDisplayWidth()
+                    ? device_info.GetPhysicalDisplayWidth()
+                    : device_info.GetDisplayWidth(),
+                device_info.GetPhysicalDisplayHeight()
+                    ? device_info.GetPhysicalDisplayHeight()
+                    : device_info.GetDisplayHeight());
+  const gfx::Rect bounds_in_dip = gfx::Rect(gfx::ScaleToCeiledSize(
+      bounds_in_pixels.size(), 1.0f / device_scale_factor));
+  gfx::Display display(0, bounds_in_dip);
+  if (!gfx::Display::HasForceDeviceScaleFactor())
+    display.set_device_scale_factor(device_scale_factor);
+  display.SetRotationAsDegree(device_info.GetRotationDegrees());
+  return display;
+}
+
 class ScreenAndroid : public Screen {
  public:
   ScreenAndroid() {}
 
   gfx::Point GetCursorScreenPoint() override { return gfx::Point(); }
 
   gfx::NativeWindow GetWindowUnderCursor() override {
     NOTIMPLEMENTED();
     return NULL;
   }
 
   gfx::NativeWindow GetWindowAtScreenPoint(const gfx::Point& point) override {
     NOTIMPLEMENTED();
     return NULL;
   }
 
   gfx::Display GetPrimaryDisplay() const override {
     gfx::DeviceDisplayInfo device_info;
-    const float device_scale_factor = device_info.GetDIPScale();
-    // Note: GetPhysicalDisplayWidth/Height() does not subtract window
-    // decorations etc. Use it instead of GetDisplayWidth/Height() when
-    // available.
-    const gfx::Rect bounds_in_pixels =
-        gfx::Rect(device_info.GetPhysicalDisplayWidth()
-                      ? device_info.GetPhysicalDisplayWidth()
-                      : device_info.GetDisplayWidth(),
-                  device_info.GetPhysicalDisplayHeight()
-                      ? device_info.GetPhysicalDisplayHeight()
-                      : device_info.GetDisplayHeight());
-    const gfx::Rect bounds_in_dip = gfx::Rect(gfx::ScaleToCeiledSize(
-        bounds_in_pixels.size(), 1.0f / device_scale_factor));
-    gfx::Display display(0, bounds_in_dip);
-    if (!gfx::Display::HasForceDeviceScaleFactor())
-      display.set_device_scale_factor(device_scale_factor);
-    display.SetRotationAsDegree(device_info.GetRotationDegrees());
-    return display;
+    return DisplayFromDeviceDisplayInfo(device_info);

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

What is gfx::Display used for?  How do they get notified of display changes?  In
particular, rotation seems to very quickly become inaccurate if it is held onto
and not updated.

Is Display supposed to be a snapshot in time or is it support to always reflect
the current state?  If the latter, then it looks like it was wrong before and
likely to be "more wrong" if we are dealing with an activity?

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

In all of the cases I have seen gfx::Display in use we simply grab some piece of
info from it (especially device scale factor / DPI), i.e. we use the Display as
a snapshot (though, I guess that observation is not a guarantee that we are/will
always use it in that way..).

I don't quite follow how it would be more wrong (if we hold on to Displays) when
dealing with an activity? 

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

I must say much of my skepticism is around my lack of knowledge of
what a Display represents in Android.

Does it truly represent the screen dimensions and not the window
bounds/available draw area?  In samsung multi window, the draw area
!= the screen size and I don't know what this is supposed to be.

Also, the fact that this is a snapshot in time is really really bad
for things like orientation.  To me, we should not set orientation
via this at all or we need to make this actually track updates from
the display (outside the scope of this I suspect).

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

So there are two different sizes that can be fetched from Display - the screen
size (getRealSize) and the size of the application window (getSize).
I might be misunderstanding your comment here about orientation changes but the
idea would be to update the DeviceDisplayInfo by using ScreenOrientationListener
which listens to changes in orientation. Right now that class assumes there is
only one display, so it needs to be changed to listen to several displays (the
biggest problem with this is making sure that the listeners are not dependent on
our activity/display context in any way, because then we can't unregister them
if our context goes away).

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

I think the "easiest" thing would be to actually not create a snapshot of the
display
values at all but simply pull them as needed.  But that requires owning a
context ref
and could potentially slow everything down as we are now generating these once
and reading the values are cheap.

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

Maybe as an aspirational end goal after all the other stuff like this stupid
SharedDeviceDisplayInfo is removed?

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

@Bo now I'm a bit lost here, what would be the aspirational goal?
If I'm not mistaken, the whole idea here is to make as few jni calls / java
object constructions/destructions as possible and I guess we wouldn't have the
SharedDeviceDisplayInfo class if this wasn't a problem.

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

Tradeoffs.. perf comes after correctness and having no leaks, and probably
on-par with code simplicity. I doubt we actually poll this enough times for the
jni to matter, but we'll have to measure that I guess

Either way, this part is out of scope for this CL, no?

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

Alright
Yeah this needs to be considered in the follow-up. The worrying part is that I
haven't touched any places under chrome/ using the default DeviceDisplayInfo
ctor so just removing the SharedDeviceDisplayInfo would make each of those calls
create a new DeviceDisplayInfo (while the call-sites I have changed would at
least cache the DeviceDisplayInfo in WindowAndroid).

   }
 
   gfx::Display GetDisplayNearestWindow(gfx::NativeView view) const override {
-    return GetPrimaryDisplay();
+    if (view) {
+      return DisplayFromDeviceDisplayInfo(
+          view->GetWindowAndroid()->GetDeviceDisplayInfo());
+    } else {
+      return GetPrimaryDisplay();
+    }
   }
 
   gfx::Display GetDisplayNearestPoint(const gfx::Point& point) const override {
     return GetPrimaryDisplay();
   }
 
   int GetNumDisplays() const override { return 1; }
 
   std::vector<gfx::Display> GetAllDisplays() const override {
     return std::vector<gfx::Display>(1, GetPrimaryDisplay());
@@ skipping 13 matching lines @@
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ScreenAndroid);
 };
 
 Screen* CreateNativeScreen() {
   return new ScreenAndroid;
 }
 
 }  // namespace gfx