Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416403002:
Reland of Android: support multiple displays on C++ side (Closed)
|
Created:
4 years, 2 months ago by Tima Vaisburd Modified:
4 years, 1 month ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland of Android: support multiple displays on C++ side
Original description:
Starting with API 17 Android can have multiple displays.
CL https://codereview.chromium.org/2361633002 introduced the map
of DisplayAndroid objects on Java side. This CL uses the information
from Java layer to maintain a similar map on the native side.
Similar to other platforms, an individual display is represented
by a Display object, and the collection of all displays in the
system is the Screen object. The Screen object for Android
maintains the mapping between the Android display ID and Display
and receives updates from DisplayAndroidManager.java through JNI.
Therefore the Screen implementation is placed in DisplayAndroidManager
class.
The Screen interface assumes the existence of the primary display.
To support this we always add the primary display on the Java side
(it is propagated to native) during the initialization of
DisplayAndroidManager.java
Native DisplayAndroidManager obtains the display ID from WindowAndroid
and thus depends on /ui/android. This CL places the manager in /ui/android
as well. Because of this we have to explicitly initialize the Screen
singleton by calling SetScreenAndroid() (similar to
other platforms) instead of creating the Screen object on demand.
The explicit initialization of the Screen happens on most platforms,
and required some tests modification.
Note: the explicit initialization of the Android Screen object required
some tests modification. Since the screen used to be created on demand,
we could have missed some tests that run on Android and not
covered by CQ.
BUG=625089
> > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8
> > Cr-Commit-Position: refs/heads/master@{#432310}
> Revert:
> Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823
> Cr-Commit-Position: refs/heads/master@{#433108}
Committed: https://crrev.com/fe53c2c64abb741b886b16b9a8c725ed397be99f
Cr-Commit-Position: refs/heads/master@{#434090}
Patch Set 1 #Patch Set 2 : Fix: set the DIP scale into Display #Patch Set 3 : Rebase only #Patch Set 4 : Update a map of Displays from Java #
Total comments: 34
Patch Set 5 : Ensure primary display. New jni scheme #
Total comments: 26
Patch Set 6 : Rebase only #Patch Set 7 : JNI generated and registered in ui/android; ensure primary DisplayAndroid by going to app context #
Total comments: 40
Patch Set 8 : Split updateFromDisplay() in two parts and update native from DisplayAndroidManager only #
Total comments: 1
Patch Set 9 : Addressed comments #Patch Set 10 : Rebase only #Patch Set 11 : Moved display_android_manager.cc to /ui/android and explicitly initialized the Screen #Patch Set 12 : Added dependency #Patch Set 13 : Update native side only after jni init #
Total comments: 1
Patch Set 14 : Fix compilaton #Patch Set 15 : Restore GetDisplayNearestWindow(0) functionality to return primary display. #
Total comments: 11
Patch Set 16 : Rebase only #Patch Set 17 : An attempt to eliminate the split between Java and native update #Patch Set 18 : Rebased #Patch Set 19 : DisplayAndroid holds native pointer to DisplayAndroidManager, fixed tests #
Total comments: 3
Patch Set 20 : Reverted PS19 #Patch Set 21 : Moved SetScreenInstance call up to browser_main_loop.cc #Patch Set 22 : Rebased #Patch Set 23 : tests #Patch Set 24 : Added dummy Screen for tests #Patch Set 25 : Fixed compilation #
Total comments: 4
Patch Set 26 : Moved SetScreenInstance call to PreMainMessageLoopRun #
Total comments: 10
Patch Set 27 : Addressed comments #
Total comments: 8
Patch Set 28 : Took web_contents_view_android.cc changes away, addressed comments #
Total comments: 2
Patch Set 29 : Renamed screen_android_for_tests.h -> dummy_screen_android.h #Patch Set 30 : Removed now unused ui/display/android/DEPS #Patch Set 31 : Rebased #Patch Set 32 : display_android_manager inherits from screen_base #Patch Set 33 : Rebased and fixed webkit_tests #Dependent Patchsets: Messages
Total messages: 192 (112 generated)
Description was changed from ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes the information about display to the Renderer. When creating ScreenInfo on Android we now take into account the display that the window is attached to. In this CL the native window_android pulls the information from its Java conunterpart and creates display::Display object on demand. BUG=620929, 625089 ========== to ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes the information about display to the Renderer. When creating ScreenInfo on Android we now take into account the display that the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ==========
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
timav@chromium.org changed reviewers: + boliu@chromium.org
PTAL. Not tested yet.
Description was changed from ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes the information about display to the Renderer. When creating ScreenInfo on Android we now take into account the display that the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ========== to ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes information about display to the Renderer. Since Android supports multiple displays, we now create the ScreenInfo using the display the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ==========
Description was changed from ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes information about display to the Renderer. Since Android supports multiple displays, we now create the ScreenInfo using the display the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ========== to ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes information about display to the Renderer. Since Android supports multiple displays, we now create the ScreenInfo using the display the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ==========
On the right track, but not how I want to structure the changes in ui/. So I think java DisplayAndroid with have a 1-to-1 mapping to a native Display in screen_android. Then DisplayAndroid will push updates to native side, including when to add and remove a display. I'm not sure what the native window_android->display mapping will look like exactly. Maybe window_android will only hold a display id so it can look up a display from screen_android. I think having a display inside window_android doesn't make much sense, because then we have to worry about keeping that update to date.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
timav@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@: Ted, this CL is not finished, as you can see from the discussion, but I think the interface for ScreenInfo creation won't change. I'm going to send you another CL which depends on this one, that's why I'm sending this one to you now.
Description was changed from ========== Android: Create ScreenInfo for particular display. The ScreenInfo passes information about display to the Renderer. Since Android supports multiple displays, we now create the ScreenInfo using the display the window is attached to. In this CL the native window_android pulls the information from its Java counterpart and creates display::Display object on demand. BUG=620929, 625089 ========== to ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroid.java. This CL also creates ScreenInfo for the right display. BUG=620929, 625089 ==========
timav@chromium.org changed reviewers: + mthiesse@chromium.org
I updated the CL, now it propagates the display info from the Java side to the native. I haven't tested the code yet, it just compiled and passed the presubmit check. Please take a look. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:57: DCHECK_EQ(displays_.count(display_id), 1U); This DCHECK will succeed because DisplayAndroid is always created in the process of WindowAndroid creation, so when we pass a view, the display is already there. For other methods like GetAllDisplays() or GetPrimaryDisplay() we might not have a window for it yet. Shall we create Displays and DisplayAndroid from these methods?
https://codereview.chromium.org/2416403002/diff/50001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:114: ui::WindowAndroid* window = I think you could simplify this function to: DisplayToScreenInfo(display::Screen::GetScreen()->GetDisplayNearestWindow(GetNativeView()), result); https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:29: @JNINamespace("ui") I think the native code for Displays on android should be in DisplayAndroidManager. I think DisplayAndroid should be equivalent to the native Display class that's usually passed by value and stay as lightweight as possible. https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { If we store a mapping from window to display in both java and native DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window would just call into DisplayAndroidManager to get the display for itself. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:58: return displays_.find(display_id)->second; I think we need to guarantee always returning a display here, even if it's the default, so if for some reason we display_id doesn't match a display in the list, we should return the default. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.h:42: std::map<int, Display> displays_; It looks like other platforms (like ash) keep their display mappings in the display manager, and call into the manager. I think we should probably do the same, and keep the mapping inside native DisplayAndroidManager.
haven't read this line by line yet https://codereview.chromium.org/2416403002/diff/50001/ui/android/display_andr... File ui/android/display_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/display_andr... ui/android/display_android.cc:5: #include "ui/android/display_android.h" ui/display/android pls https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:29: @JNINamespace("ui") On 2016/10/26 14:49:49, mthiesse wrote: > I think the native code for Displays on android should be in > DisplayAndroidManager. I think DisplayAndroid should be equivalent to the native > Display class that's usually passed by value and stay as lightweight as > possible. Pass by value is not really a thing in java, but yeah, java DisplayAndroid should be as light weight as possible. That doesn't mean it can't talk to native things. As long as the common accessors don't do jni, it's just as light weight as before. https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { On 2016/10/26 14:49:49, mthiesse wrote: > If we store a mapping from window to display in both java and native > DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window would > just call into DisplayAndroidManager to get the display for itself. yeah, this is one of those weird things where we really wish to expose this to WindowAndroid only, not the world. But maybe a comment, or getSdkDisplayIdInternal would be good enough here. I think I'd rather not have native side have an explicit window -> display mapping because feels like keeping that mapping up-to-date is a lot of work. Unless you see some advantage? https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.cc:54: return new WindowAndroid(env, java_window.obj(), display_id); I think it's cleaner to have WindowAndroid create the native side, and add a jni method to get that pointer here https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.h:40: int display_id() const { return display_id_; } don't need to be public? can just friend screen_android if screen_android needs this https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.h:52: display::Display GetDisplay() const; probably don't need this either https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:54: DCHECK(window); this may not hold either https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:57: DCHECK_EQ(displays_.count(display_id), 1U); On 2016/10/26 02:02:18, Tima Vaisburd wrote: > This DCHECK will succeed because DisplayAndroid is always created in the process > of WindowAndroid creation, so when we pass a view, the display is already there. It's more subtle than that. In theory, it's possible for WindowAndroid to outlive the "display" it's referring to. (Maybe android has guarantees of when this happens, but I certainly don't want to rely on that). In java, we just keep a reference to the DisplayAndroid so that object doesn't die, but it's holding stale data. But in native code, you won't find a display here, so this DCHECK won't hold in that case > > For other methods like GetAllDisplays() or GetPrimaryDisplay() we might not have > a window for it yet. > > Shall we create Displays and DisplayAndroid from these methods? I don't know what this means https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:58: return displays_.find(display_id)->second; On 2016/10/26 14:49:49, mthiesse wrote: > I think we need to guarantee always returning a display here, even if it's the > default, so if for some reason we display_id doesn't match a display in the > list, we should return the default. +1 looks like most desktop implementations fallback to the default display, so should be ok here https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. remove (c) https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.h:42: std::map<int, Display> displays_; On 2016/10/26 14:49:49, mthiesse wrote: > It looks like other platforms (like ash) keep their display mappings in the > display manager, and call into the manager. I think we should probably do the > same, and keep the mapping inside native DisplayAndroidManager. The point here is we don't want to expose ScreenAndroid in a header file. aura has a display manager, but I think mac and ios do not, so it's not really a pattern. It's probably easier to have DisplayAndroid.java talk directly to screen_android.cc through jni. Nothing technically wrong with that, but that's like unidiomatic jni. So... maybe move everything currently in screen_android.cc to display_android.cc? Or move the jni stuff from java DisplayAndroid to DisplayAndroidManager, dunno which one is easier.
https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { On 2016/10/26 17:24:20, boliu wrote: > On 2016/10/26 14:49:49, mthiesse wrote: > > If we store a mapping from window to display in both java and native > > DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window > would > > just call into DisplayAndroidManager to get the display for itself. > > yeah, this is one of those weird things where we really wish to expose this to > WindowAndroid only, not the world. But maybe a comment, or > getSdkDisplayIdInternal would be good enough here. > > I think I'd rather not have native side have an explicit window -> display > mapping because feels like keeping that mapping up-to-date is a lot of work. > Unless you see some advantage? The main benefit is consistency between native and java sides. It's weird that WindowAndroid java stores a cached DisplayAndroid, but native stores an ID and gets the display from somewhere else. I'm okay with them both just storing an ID and fetching the display from the Manager, but that requires exposing a getDisplayById-style function in DisplayAndroidManager, which you seemed to be against. Also public methods with names like getSdkDisplayIdInternal kind of suck :( I don't see storing both a native and java window->display map as being relevantly more work than the implicit mapping done by passing a display ID around, and it makes things much easier to reason about. Both maps would be updated only by DisplayAndroidManager, DisplayAndroidManager::SetDisplayForWindow would just call nativeSetDisplayForWindow, etc.
https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:57: DCHECK_EQ(displays_.count(display_id), 1U); On 2016/10/26 17:24:20, boliu wrote: > I don't know what this means I was asking whether in GetPrimaryDisplay() and maybe other methods we need to call into DisplayAndroidManager through jni, get() the primary display (or all current displays) there to ensure we added them into both maps, and then return the corresponding Display(s). I think this is the way to support GetPrimaryDisplay(), and maybe GetDisplayCount() and GetAllDisplays() if we need them, and really wanted to hear your opinion.
https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { On 2016/10/26 17:52:49, mthiesse wrote: > On 2016/10/26 17:24:20, boliu wrote: > > On 2016/10/26 14:49:49, mthiesse wrote: > > > If we store a mapping from window to display in both java and native > > > DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window > > would > > > just call into DisplayAndroidManager to get the display for itself. > > > > yeah, this is one of those weird things where we really wish to expose this to > > WindowAndroid only, not the world. But maybe a comment, or > > getSdkDisplayIdInternal would be good enough here. > > > > I think I'd rather not have native side have an explicit window -> display > > mapping because feels like keeping that mapping up-to-date is a lot of work. > > Unless you see some advantage? > > The main benefit is consistency between native and java sides. It's weird that > WindowAndroid java stores a cached DisplayAndroid, but native stores an ID and > gets the display from somewhere else. I'm okay with them both just storing an ID > and fetching the display from the Manager, but that requires exposing a > getDisplayById-style function in DisplayAndroidManager, which you seemed to be > against. Ok I objected to that because the current thing seems cleaner, but the inconsistency does seem worse. How about this. Let's assume we've nailed down the concept of the primary display, which we have to guarantee (at least in code) that it never goes away. Then on both the java and native side, if the display the window was referring to goes away, it just falls back to primary one. That still exposes display id to WindowAndroid though. I don't think it's really necessary to hide it since it's visible from native side already. And we still don't need a window->display map anywhere :p > Also public methods with names like getSdkDisplayIdInternal kind of > suck :( > > I don't see storing both a native and java window->display map as being > relevantly more work than the implicit mapping done by passing a display ID > around, and it makes things much easier to reason about. Both maps would be > updated only by DisplayAndroidManager, > DisplayAndroidManager::SetDisplayForWindow would just call > nativeSetDisplayForWindow, etc. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:57: DCHECK_EQ(displays_.count(display_id), 1U); On 2016/10/26 17:58:10, Tima Vaisburd wrote: > On 2016/10/26 17:24:20, boliu wrote: > > > I don't know what this means > > I was asking whether in GetPrimaryDisplay() and maybe other methods we need to > call into DisplayAndroidManager through jni, get() the primary display (or all > current displays) there to ensure we added them into both maps, and then return > the corresponding Display(s). > > I think this is the way to support GetPrimaryDisplay(), and maybe > GetDisplayCount() and GetAllDisplays() if we need them, and really wanted to > hear your opinion. Primary display should be pushed down as well. And it probably shouldn't change, at least on android. GetAllDisplay should just loop through what's already there. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:72: const int kPrimaryDisplayId = 0; where did we decide 0 is the primary one?
https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { On 2016/10/26 18:24:43, boliu wrote: > On 2016/10/26 17:52:49, mthiesse wrote: > > On 2016/10/26 17:24:20, boliu wrote: > > > On 2016/10/26 14:49:49, mthiesse wrote: > > > > If we store a mapping from window to display in both java and native > > > > DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window > > > would > > > > just call into DisplayAndroidManager to get the display for itself. > > > > > > yeah, this is one of those weird things where we really wish to expose this > to > > > WindowAndroid only, not the world. But maybe a comment, or > > > getSdkDisplayIdInternal would be good enough here. > > > > > > I think I'd rather not have native side have an explicit window -> display > > > mapping because feels like keeping that mapping up-to-date is a lot of work. > > > Unless you see some advantage? > > > > The main benefit is consistency between native and java sides. It's weird that > > WindowAndroid java stores a cached DisplayAndroid, but native stores an ID and > > gets the display from somewhere else. I'm okay with them both just storing an > ID > > and fetching the display from the Manager, but that requires exposing a > > getDisplayById-style function in DisplayAndroidManager, which you seemed to be > > against. > > Ok I objected to that because the current thing seems cleaner, but the > inconsistency does seem worse. How about this. Let's assume we've nailed down > the concept of the primary display, which we have to guarantee (at least in > code) that it never goes away. Then on both the java and native side, if the > display the window was referring to goes away, it just falls back to primary > one. > > That still exposes display id to WindowAndroid though. I don't think it's really > necessary to hide it since it's visible from native side already. And we still > don't need a window->display map anywhere :p > > > Also public methods with names like getSdkDisplayIdInternal kind of > > suck :( > > > > I don't see storing both a native and java window->display map as being > > relevantly more work than the implicit mapping done by passing a display ID > > around, and it makes things much easier to reason about. Both maps would be > > updated only by DisplayAndroidManager, > > DisplayAndroidManager::SetDisplayForWindow would just call > > nativeSetDisplayForWindow, etc. > sgtm
https://codereview.chromium.org/2416403002/diff/50001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:114: ui::WindowAndroid* window = On 2016/10/26 14:49:49, mthiesse wrote: > I think you could simplify this function to: > > DisplayToScreenInfo(display::Screen::GetScreen()->GetDisplayNearestWindow(GetNativeView()), > result); Good point! I rewrote this similarly, keeping primary display in case GetNativeView() returns 0. https://codereview.chromium.org/2416403002/diff/50001/ui/android/display_andr... File ui/android/display_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/display_andr... ui/android/display_android.cc:5: #include "ui/android/display_android.h" On 2016/10/26 17:24:19, boliu wrote: > ui/display/android pls This file is gone, JNI is handled by ui/display/andriod/display_android_manager.cc https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:107: public int getSdkDisplayId() { On 2016/10/26 18:24:43, boliu wrote: > On 2016/10/26 17:52:49, mthiesse wrote: > > On 2016/10/26 17:24:20, boliu wrote: > > > On 2016/10/26 14:49:49, mthiesse wrote: > > > > If we store a mapping from window to display in both java and native > > > > DisplayAndroidManager, we don't need to expose DisplayIds anywhere. window > > > would > > > > just call into DisplayAndroidManager to get the display for itself. > > > > > > yeah, this is one of those weird things where we really wish to expose this > to > > > WindowAndroid only, not the world. But maybe a comment, or > > > getSdkDisplayIdInternal would be good enough here. > > > > > > I think I'd rather not have native side have an explicit window -> display > > > mapping because feels like keeping that mapping up-to-date is a lot of work. > > > Unless you see some advantage? > > > > The main benefit is consistency between native and java sides. It's weird that > > WindowAndroid java stores a cached DisplayAndroid, but native stores an ID and > > gets the display from somewhere else. I'm okay with them both just storing an > ID > > and fetching the display from the Manager, but that requires exposing a > > getDisplayById-style function in DisplayAndroidManager, which you seemed to be > > against. > > Ok I objected to that because the current thing seems cleaner, but the > inconsistency does seem worse. How about this. Let's assume we've nailed down > the concept of the primary display, which we have to guarantee (at least in > code) that it never goes away. Then on both the java and native side, if the > display the window was referring to goes away, it just falls back to primary > one. > > That still exposes display id to WindowAndroid though. I don't think it's really > necessary to hide it since it's visible from native side already. And we still > don't need a window->display map anywhere :p > > > Also public methods with names like getSdkDisplayIdInternal kind of > > suck :( > > > > I don't see storing both a native and java window->display map as being > > relevantly more work than the implicit mapping done by passing a display ID > > around, and it makes things much easier to reason about. Both maps would be > > updated only by DisplayAndroidManager, > > DisplayAndroidManager::SetDisplayForWindow would just call > > nativeSetDisplayForWindow, etc. > Done, except for Java side. https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.cc:54: return new WindowAndroid(env, java_window.obj(), display_id); On 2016/10/26 17:24:20, boliu wrote: > I think it's cleaner to have WindowAndroid create the native side, and add a jni > method to get that pointer here This is not done yet. https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.h:40: int display_id() const { return display_id_; } On 2016/10/26 17:24:20, boliu wrote: > don't need to be public? can just friend screen_android if screen_android needs > this Done. https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.h:52: display::Display GetDisplay() const; On 2016/10/26 17:24:20, boliu wrote: > probably don't need this either Removed. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:58: return displays_.find(display_id)->second; On 2016/10/26 17:24:20, boliu wrote: > On 2016/10/26 14:49:49, mthiesse wrote: > > I think we need to guarantee always returning a display here, even if it's the > > default, so if for some reason we display_id doesn't match a display in the > > list, we should return the default. > > +1 looks like most desktop implementations fallback to the default display, so > should be ok here Following the offline conversation I add the primary display in DisplayAndroidManager upon the creation of DisplayAndroidMAnager.java. The primary display is never removed from our maps. GetDisplayNearestWindow() fall backs to GetPrimaryDisplay(). https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.cc:72: const int kPrimaryDisplayId = 0; On 2016/10/26 18:24:44, boliu wrote: > where did we decide 0 is the primary one? Display.DEFAULT_DISPLAY is defined as 0, I added the explanation. https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... File ui/display/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/10/26 17:24:20, boliu wrote: > remove (c) Gone while moving/renaming https://codereview.chromium.org/2416403002/diff/50001/ui/display/android/scre... ui/display/android/screen_android.h:42: std::map<int, Display> displays_; On 2016/10/26 17:24:20, boliu wrote: > On 2016/10/26 14:49:49, mthiesse wrote: > > It looks like other platforms (like ash) keep their display mappings in the > > display manager, and call into the manager. I think we should probably do the > > same, and keep the mapping inside native DisplayAndroidManager. > > The point here is we don't want to expose ScreenAndroid in a header file. aura > has a display manager, but I think mac and ios do not, so it's not really a > pattern. > > It's probably easier to have DisplayAndroid.java talk directly to > screen_android.cc through jni. Nothing technically wrong with that, but that's > like unidiomatic jni. So... maybe move everything currently in screen_android.cc > to display_android.cc? Or move the jni stuff from java DisplayAndroid to > DisplayAndroidManager, dunno which one is easier. Renamed screen_android.cc into display_android_manager.cc and moved the JNI stuff from DisplayAndroid to DisplayAndroidManager. Now on native side DisplayAndroidManager implements display::Screen interface and receives JNI calls. https://codereview.chromium.org/2416403002/diff/70001/content/app/android/lib... File content/app/android/library_loader_hooks.cc (right): https://codereview.chromium.org/2416403002/diff/70001/content/app/android/lib... content/app/android/library_loader_hooks.cc:101: if (!display::RegisterJni(env)) Put registration separately from ui::RegisterUIAndroidJni(): is this the right approach? https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn#new... ui/display/BUILD.gn:142: "../../ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java", The path that start with ".." is probably bad, shall we move DisplayAndroid.java and DisplayAndroidManager.java under ui/display/android ?
I'll leave any comments on the jni to boliu, as I'm pretty new to chrome jni. https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:286: getManager().updateDisplayOnNativeSide(this); I think this should be moved into the manager, DisplayAndroid shouldn't have to worry about the native side getting updated. (Just call updateDisplayOnNativeSide after updateFromDisplay) https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); This feels scary - should we not just use getDisplayFromContext(getContext()) in this case and use that as though it's the default display? Then in onDisplayRemoved you could reject removing the last display from the list as well, so we always have a display? https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn#new... ui/display/BUILD.gn:142: "../../ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java", On 2016/10/27 07:55:58, Tima Vaisburd wrote: > The path that start with ".." is probably bad, shall we move DisplayAndroid.java > and DisplayAndroidManager.java under ui/display/android ? Do you mean to ui/display/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java? I don't think we can move java source out of a java/src folder. https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, Why is this static? It's only called from a non-static function, right?
I'll leave any comments on the jni to boliu, as I'm pretty new to chrome jni.
Michael, thank you for prompt response! https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:286: getManager().updateDisplayOnNativeSide(this); On 2016/10/27 14:30:30, mthiesse wrote: > I think this should be moved into the manager, DisplayAndroid shouldn't have to > worry about the native side getting updated. (Just call > updateDisplayOnNativeSide after updateFromDisplay) I thought about that, this method could have returned boolean to indicate that there were changes. However, I wanted to update the native side before we notify observers. More observers are supposed to come (e.g. DIP scale observer). https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 14:30:30, mthiesse wrote: > This feels scary - should we not just use getDisplayFromContext(getContext()) in > this case and use that as though it's the default display? > > Then in onDisplayRemoved you could reject removing the last display from the > list as well, so we always have a display? Sorry, I did not quite get your idea. Do you suggest that if there is no primary display (with id 0), but some other display exists, we should consider it primary? I kept primary here according to the Android documentation: the one with display id equal to 0. https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn#new... ui/display/BUILD.gn:142: "../../ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java", On 2016/10/27 14:30:30, mthiesse wrote: > On 2016/10/27 07:55:58, Tima Vaisburd wrote: > > The path that start with ".." is probably bad, shall we move > DisplayAndroid.java > > and DisplayAndroidManager.java under ui/display/android ? > > Do you mean to > ui/display/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java? > > I don't think we can move java source out of a java/src folder. I guess we could repeat the hierarchy under this ui/display/android, but I did not try. https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, On 2016/10/27 14:30:30, mthiesse wrote: > Why is this static? It's only called from a non-static function, right? I did this to avoid propagating the native object pointer to DisplayAndroidManager.java. Maybe I should have.
https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:286: getManager().updateDisplayOnNativeSide(this); On 2016/10/27 17:56:23, Tima Vaisburd wrote: > On 2016/10/27 14:30:30, mthiesse wrote: > > I think this should be moved into the manager, DisplayAndroid shouldn't have > to > > worry about the native side getting updated. (Just call > > updateDisplayOnNativeSide after updateFromDisplay) > > I thought about that, this method could have returned boolean to indicate that > there were changes. > However, I wanted to update the native side before we notify observers. More > observers are supposed to come (e.g. DIP scale observer). On the second thought: yes, I will split this method.
https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 17:56:23, Tima Vaisburd wrote: > On 2016/10/27 14:30:30, mthiesse wrote: > > This feels scary - should we not just use getDisplayFromContext(getContext()) > in > > this case and use that as though it's the default display? > > > > Then in onDisplayRemoved you could reject removing the last display from the > > list as well, so we always have a display? > > Sorry, I did not quite get your idea. Do you suggest that if there is no primary > display (with id 0), but some other display exists, we should consider it > primary? > I kept primary here according to the Android documentation: the one with display > id equal to 0. Yes, that's what I'm suggesting. I'm not sure what we should do if there's no display at all. Is that even possible? https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, On 2016/10/27 17:56:24, Tima Vaisburd wrote: > On 2016/10/27 14:30:30, mthiesse wrote: > > Why is this static? It's only called from a non-static function, right? > > I did this to avoid propagating the native object pointer to > DisplayAndroidManager.java. Maybe I should have. Just my opinion, but I think it would be better to pass the native pointer and not have these static.
https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 19:51:51, mthiesse wrote: > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > On 2016/10/27 14:30:30, mthiesse wrote: > > > This feels scary - should we not just use > getDisplayFromContext(getContext()) > > in > > > this case and use that as though it's the default display? > > > > > > Then in onDisplayRemoved you could reject removing the last display from the > > > list as well, so we always have a display? > > > > Sorry, I did not quite get your idea. Do you suggest that if there is no > primary > > display (with id 0), but some other display exists, we should consider it > > primary? > > I kept primary here according to the Android documentation: the one with > display > > id equal to 0. > > Yes, that's what I'm suggesting. I'm not sure what we should do if there's no > display at all. Is that even possible? I do not know, maybe not possible. Another idea is to come up with default properties (size, density etc.), or have the values corresponding to Display() with 32 bits per pixel (Android now has only RGBA_8888 pixel format). Actually the latter seems reasonable to me, what do you think?
https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 20:22:30, Tima Vaisburd wrote: > On 2016/10/27 19:51:51, mthiesse wrote: > > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > This feels scary - should we not just use > > getDisplayFromContext(getContext()) > > > in > > > > this case and use that as though it's the default display? > > > > > > > > Then in onDisplayRemoved you could reject removing the last display from > the > > > > list as well, so we always have a display? > > > > > > Sorry, I did not quite get your idea. Do you suggest that if there is no > > primary > > > display (with id 0), but some other display exists, we should consider it > > > primary? > > > I kept primary here according to the Android documentation: the one with > > display > > > id equal to 0. > > > > Yes, that's what I'm suggesting. I'm not sure what we should do if there's no > > display at all. Is that even possible? > > I do not know, maybe not possible. > Another idea is to come up with default properties (size, density etc.), or have > the values corresponding to Display() with 32 bits per pixel (Android now has > only RGBA_8888 pixel format). Actually the latter seems reasonable to me, what > do you think? I'll have to defer to Bo here, I don't know enough in this space.
ok ok.. the new jni_registrar might be going too far, at least not until we discuss long term plans of how ui android code should be structured with ui/android owners, and all the owners are ooo :/ I think my proposal would be just have ui/android be the jni layer, so it can include android cc files from every other ui directory (for jni registration), and then cc files living in individual ui directories have to respect the DEPS rules of that directory so in this case.. go back to like the previous patch set, but just move the cc to ui/display, and leave everything else the same? https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:286: getManager().updateDisplayOnNativeSide(this); On 2016/10/27 18:34:26, Tima Vaisburd wrote: > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > On 2016/10/27 14:30:30, mthiesse wrote: > > > I think this should be moved into the manager, DisplayAndroid shouldn't have > > to > > > worry about the native side getting updated. (Just call > > > updateDisplayOnNativeSide after updateFromDisplay) > > > > I thought about that, this method could have returned boolean to indicate that > > there were changes. > > However, I wanted to update the native side before we notify observers. Whatever you end up doing, do keep this property. Personally I think this is ok. > > More > > observers are supposed to come (e.g. DIP scale observer). > > On the second thought: yes, I will split this method. https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 20:29:13, mthiesse wrote: > On 2016/10/27 20:22:30, Tima Vaisburd wrote: > > On 2016/10/27 19:51:51, mthiesse wrote: > > > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > > This feels scary - should we not just use > > > getDisplayFromContext(getContext()) > > > > in > > > > > this case and use that as though it's the default display? > > > > > > > > > > Then in onDisplayRemoved you could reject removing the last display from > > the > > > > > list as well, so we always have a display? > > > > > > > > Sorry, I did not quite get your idea. Do you suggest that if there is no > > > primary > > > > display (with id 0), but some other display exists, we should consider it > > > > primary? > > > > I kept primary here according to the Android documentation: the one with > > > display > > > > id equal to 0. > > > > > > Yes, that's what I'm suggesting. I'm not sure what we should do if there's > no > > > display at all. Is that even possible? > > > > I do not know, maybe not possible. > > Another idea is to come up with default properties (size, density etc.), or > have > > the values corresponding to Display() with 32 bits per pixel (Android now has > > only RGBA_8888 pixel format). Actually the latter seems reasonable to me, what > > do you think? > > I'll have to defer to Bo here, I don't know enough in this space. I don't this that can ever happen. throw an exception and hope for the best, since this is android..? or if you want to be safer, get the default display from the application context, that should always be the default display, I hope.. https://codereview.chromium.org/2416403002/diff/70001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/window_andro... ui/android/window_android.cc:16: #include "ui/display/screen.h" this import seems unnecessary https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn#new... ui/display/BUILD.gn:72: if (is_android && !use_aura) { aura + android isn't a thing anymore, and android directory stuff should already be only be included on android, so don't need this conditional https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, On 2016/10/27 19:51:51, mthiesse wrote: > On 2016/10/27 17:56:24, Tima Vaisburd wrote: > > On 2016/10/27 14:30:30, mthiesse wrote: > > > Why is this static? It's only called from a non-static function, right? > > > > I did this to avoid propagating the native object pointer to > > DisplayAndroidManager.java. Maybe I should have. > > Just my opinion, but I think it would be better to pass the native pointer and > not have these static. I'm fine with static here. I don't think the assumption that screen is global will change any time soon
On 2016/10/27 22:17:49, boliu wrote: > I think my proposal would be just have ui/android be the jni layer, so it can > include android cc files from every other ui directory (for jni registration), > and then cc files living in individual ui directories have to respect the DEPS > rules of that directory > so in this case.. go back to like the previous patch set, but just move the cc > to ui/display, and leave everything else the same? Last time I tried I could not do that, because display_android_manager.cc in ui/display/android/ could not find the generated "jni/DisplayAndroidManager_jni.h" when is was generated from ui/android/. That's why I ended up putting generate_jni() in ui/display/BUILD.gn. But then I had to reference the Java file across the components. Of course I could have been missing something...
On 2016/10/27 22:42:15, Tima Vaisburd wrote: > On 2016/10/27 22:17:49, boliu wrote: > > > I think my proposal would be just have ui/android be the jni layer, so it can > > include android cc files from every other ui directory (for jni registration), > > and then cc files living in individual ui directories have to respect the DEPS > > rules of that directory > > > so in this case.. go back to like the previous patch set, but just move the cc > > to ui/display, and leave everything else the same? > > Last time I tried I could not do that, because display_android_manager.cc in > ui/display/android/ could not find the generated > "jni/DisplayAndroidManager_jni.h" > when is was generated from ui/android/. > That's why I ended up putting generate_jni() in ui/display/BUILD.gn. > But then I had to reference the Java file across the components. > > Of course I could have been missing something... The display_android_manager.cc target needs to depend on jni header target, will that be a problem?
On 2016/10/27 22:48:12, boliu wrote: > On 2016/10/27 22:42:15, Tima Vaisburd wrote: > > On 2016/10/27 22:17:49, boliu wrote: > > > > > I think my proposal would be just have ui/android be the jni layer, so it > can > > > include android cc files from every other ui directory (for jni > registration), > > > and then cc files living in individual ui directories have to respect the > DEPS > > > rules of that directory > > > > > so in this case.. go back to like the previous patch set, but just move the > cc > > > to ui/display, and leave everything else the same? > > > > Last time I tried I could not do that, because display_android_manager.cc in > > ui/display/android/ could not find the generated > > "jni/DisplayAndroidManager_jni.h" > > when is was generated from ui/android/. > > That's why I ended up putting generate_jni() in ui/display/BUILD.gn. > > But then I had to reference the Java file across the components. > > > > Of course I could have been missing something... > > The display_android_manager.cc target needs to depend on jni header target, will > that be a problem? I think I did not have that, thanks!
Back from sheriffing, PTAL again. boliu@> The display_android_manager.cc target needs to depend boliu@> on jni header target, will that be a problem? That worked! So, now all jni is generated by ui/android/BUILD.gn, but one header is used in ui/display/android. The jni registration is consolidated in ui_android_jni_registrar.cc https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2416403002/diff/50001/ui/android/window_andro... ui/android/window_android.cc:54: return new WindowAndroid(env, java_window.obj(), display_id); On 2016/10/27 07:55:58, Tima Vaisburd wrote: > On 2016/10/26 17:24:20, boliu wrote: > > I think it's cleaner to have WindowAndroid create the native side, and add a > jni > > method to get that pointer here > > This is not done yet. Done, but no new method added: Java's createForTesting() returns the native pointer. Please take a look. https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:286: getManager().updateDisplayOnNativeSide(this); On 2016/10/27 22:17:49, boliu wrote: > On 2016/10/27 18:34:26, Tima Vaisburd wrote: > > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > I think this should be moved into the manager, DisplayAndroid shouldn't > have > > > to > > > > worry about the native side getting updated. (Just call > > > > updateDisplayOnNativeSide after updateFromDisplay) > > > > > > I thought about that, this method could have returned boolean to indicate > that > > > there were changes. > > > However, I wanted to update the native side before we notify observers. > > Whatever you end up doing, do keep this property. Personally I think this is ok. > > > > More > > > observers are supposed to come (e.g. DIP scale observer). > > > > On the second thought: yes, I will split this method. > The split is not done, I'll try in the next PS. https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:227: nativeSetNoPrimaryDisplay(); On 2016/10/27 22:17:49, boliu wrote: > On 2016/10/27 20:29:13, mthiesse wrote: > > On 2016/10/27 20:22:30, Tima Vaisburd wrote: > > > On 2016/10/27 19:51:51, mthiesse wrote: > > > > On 2016/10/27 17:56:23, Tima Vaisburd wrote: > > > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > > > This feels scary - should we not just use > > > > getDisplayFromContext(getContext()) > > > > > in > > > > > > this case and use that as though it's the default display? > > > > > > > > > > > > Then in onDisplayRemoved you could reject removing the last display > from > > > the > > > > > > list as well, so we always have a display? > > > > > > > > > > Sorry, I did not quite get your idea. Do you suggest that if there is no > > > > primary > > > > > display (with id 0), but some other display exists, we should consider > it > > > > > primary? > > > > > I kept primary here according to the Android documentation: the one with > > > > display > > > > > id equal to 0. > > > > > > > > Yes, that's what I'm suggesting. I'm not sure what we should do if there's > > no > > > > display at all. Is that even possible? > > > > > > I do not know, maybe not possible. > > > Another idea is to come up with default properties (size, density etc.), or > > have > > > the values corresponding to Display() with 32 bits per pixel (Android now > has > > > only RGBA_8888 pixel format). Actually the latter seems reasonable to me, > what > > > do you think? > > > > I'll have to defer to Bo here, I don't know enough in this space. > > I don't this that can ever happen. throw an exception and hope for the best, > since this is android..? > > or if you want to be safer, get the default display from the application > context, that should always be the default display, I hope.. Got Display from the application context. I had to pass the display ID to the native side since I wasn't sure that the Display in this case will always have is equal to 0. https://codereview.chromium.org/2416403002/diff/70001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/android/window_andro... ui/android/window_android.cc:16: #include "ui/display/screen.h" On 2016/10/27 22:17:49, boliu wrote: > this import seems unnecessary Removed. https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/BUILD.gn#new... ui/display/BUILD.gn:72: if (is_android && !use_aura) { On 2016/10/27 22:17:49, boliu wrote: > aura + android isn't a thing anymore, and android directory stuff should already > be only be included on android, so don't need this conditional display_android_manager.{cc,h} are included unconditionally, jni_registrar.{cc,h} are removed altogether. https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, On 2016/10/27 22:17:49, boliu wrote: > On 2016/10/27 19:51:51, mthiesse wrote: > > On 2016/10/27 17:56:24, Tima Vaisburd wrote: > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > Why is this static? It's only called from a non-static function, right? > > > > > > I did this to avoid propagating the native object pointer to > > > DisplayAndroidManager.java. Maybe I should have. > > > > Just my opinion, but I think it would be better to pass the native pointer and > > not have these static. > > I'm fine with static here. I don't think the assumption that screen is global > will change any time soon Kept static.
one step closer to nuking DeviceDisplayInfo https://codereview.chromium.org/2416403002/diff/110001/ui/android/DEPS File ui/android/DEPS (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/DEPS#newcode15 ui/android/DEPS:15: "+ui/display/android/display_android_manager.h", just replace these 3 with ui/display https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:185: WindowAndroid windowAndroid = new WindowAndroid(context); maybe add a comment that java windowAndroid is owned by native, so is not immediately eligible for gc here https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:680: * The properties of DisplayAndroid used by native side. hmm? comment seems out of place https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:175: PixelFormat info = new PixelFormat(); move this to updateFromDisplay allocating heap objects is definitely a no-no on these getters https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:184: public int getBitsPerComponent() { package https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:252: mPixelFormatId = PixelFormat.RGBA_8888; move this to updateFromDisplay https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... ui/android/window_android.h:19: #include "ui/display/display.h" not necessary? https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... ui/android/window_android.h:99: int display_id_; const? https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/DEPS File ui/display/android/DEPS (right): https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/DEP... ui/display/android/DEPS:2: "+ui/android", add ui/android/window_android.h explicitly, and make a note window_android should really be under ui/gfx https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:24: // Disallow copy and assign. https://cs.chromium.org/chromium/src/base/macros.h?rcl=0&l=25 https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:28: // Screen interface. where are the overrides? https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:30: gfx::Point GetCursorScreenPoint() { return gfx::Point(); } this one is NOTIMPLEMENTED as well https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:69: return Display(); old code returns primary display, so keep doing that but probably should leave some kind of warning like NOTIMPLEMENTED https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:74: return Display(); ditto https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:79: CHECK_EQ(displays_.count(primary_display_id_), 1U); since this is a release check, avoid looking up the map twice in release code by actually using iterators https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:84: void AddObserver(DisplayObserver* observer) {} add NOTIMPLEMENTED https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:94: void RemoveDisplay(int display_id) { displays_.erase(display_id); } DCHECK key already exists https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:99: Display GetFromMap(int id) const { in both places, having this helper is actually forcing the code to do the look up twice, just don't do it? https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:100: DCHECK_EQ(displays_.count(id), 1U); fyi there's base::ContainsKey https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:138: display::Display display = display::Display(0, bounds_in_dip); use id here also I think declaration syntax might be preferred over assignment, though I didn't check the style guide.. it's definitely shorter though
https://codereview.chromium.org/2416403002/diff/110001/ui/android/DEPS File ui/android/DEPS (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/DEPS#newcode15 ui/android/DEPS:15: "+ui/display/android/display_android_manager.h", On 2016/11/01 17:11:04, boliu wrote: > just replace these 3 with ui/display Done. https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:185: WindowAndroid windowAndroid = new WindowAndroid(context); On 2016/11/01 17:11:04, boliu wrote: > maybe add a comment that java windowAndroid is owned by native, so is not > immediately eligible for gc here Done. https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:680: * The properties of DisplayAndroid used by native side. On 2016/11/01 17:11:04, boliu wrote: > hmm? > > comment seems out of place Forgot to delete it on prior cleanup. Removed. https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:175: PixelFormat info = new PixelFormat(); On 2016/11/01 17:11:04, boliu wrote: > move this to updateFromDisplay > > allocating heap objects is definitely a no-no on these getters Done. https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:184: public int getBitsPerComponent() { On 2016/11/01 17:11:04, boliu wrote: > package Done. https://codereview.chromium.org/2416403002/diff/110001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:252: mPixelFormatId = PixelFormat.RGBA_8888; On 2016/11/01 17:11:04, boliu wrote: > move this to updateFromDisplay Done. https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... ui/android/window_android.h:19: #include "ui/display/display.h" On 2016/11/01 17:11:04, boliu wrote: > not necessary? Removed. https://codereview.chromium.org/2416403002/diff/110001/ui/android/window_andr... ui/android/window_android.h:99: int display_id_; On 2016/11/01 17:11:04, boliu wrote: > const? Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/DEPS File ui/display/android/DEPS (right): https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/DEP... ui/display/android/DEPS:2: "+ui/android", On 2016/11/01 17:11:04, boliu wrote: > add ui/android/window_android.h explicitly, and make a note window_android > should really be under ui/gfx Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:24: // Disallow copy and assign. On 2016/11/01 17:11:05, boliu wrote: > https://cs.chromium.org/chromium/src/base/macros.h?rcl=0&l=25 Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:28: // Screen interface. On 2016/11/01 17:11:05, boliu wrote: > where are the overrides? Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:30: gfx::Point GetCursorScreenPoint() { return gfx::Point(); } On 2016/11/01 17:11:05, boliu wrote: > this one is NOTIMPLEMENTED as well Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:69: return Display(); On 2016/11/01 17:11:05, boliu wrote: > old code returns primary display, so keep doing that > > but probably should leave some kind of warning like NOTIMPLEMENTED Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:74: return Display(); On 2016/11/01 17:11:05, boliu wrote: > ditto Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:79: CHECK_EQ(displays_.count(primary_display_id_), 1U); On 2016/11/01 17:11:04, boliu wrote: > since this is a release check, avoid looking up the map twice in release code by > actually using iterators Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:84: void AddObserver(DisplayObserver* observer) {} On 2016/11/01 17:11:05, boliu wrote: > add NOTIMPLEMENTED Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:94: void RemoveDisplay(int display_id) { displays_.erase(display_id); } On 2016/11/01 17:11:04, boliu wrote: > DCHECK key already exists Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:99: Display GetFromMap(int id) const { On 2016/11/01 17:11:05, boliu wrote: > in both places, having this helper is actually forcing the code to do the look > up twice, just don't do it? Done. https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:100: DCHECK_EQ(displays_.count(id), 1U); On 2016/11/01 17:11:05, boliu wrote: > fyi there's base::ContainsKey This method is gone, but using in RemoveDisplay(). https://codereview.chromium.org/2416403002/diff/110001/ui/display/android/dis... ui/display/android/display_android_manager.cc:138: display::Display display = display::Display(0, bounds_in_dip); On 2016/11/01 17:11:05, boliu wrote: > use id here > > also I think declaration syntax might be preferred over assignment, though I > didn't check the style guide.. it's definitely shorter though Done. https://codereview.chromium.org/2416403002/diff/130001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/130001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:268: /* package */ Set<PropertyChange> updateFromDisplay(Display display) { Split original updateFromDisplay() into two parts. The first part returns Set<PropertyChange> to the manager. The manager updates the native side and calls the second part with this set. The second part decides which observers to notify. This way the native side is only updated from the manager and it happens before we notify observers. PTAL.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The analyze() step on many bots failed with the chack that can be reproduced
locally:
> gn gen <out-dir> --check
ERROR at //ui/display/android/display_android_manager.cc:14:11: Include not
allowed.
#include "ui/android/view_android.h"
^------------------------
It is not in any dependency of
//ui/display:display
The include file is in the target(s):
//ui/android:android
which should somehow be reachable.
I'm thinking of making a base class in gfx:: that ui::ViewAndroid will derive
from,
e.g.
gfx { class NativeViewAndroid { virtual int GetDisplayId(); }
ui::ViewAndroid : public gfx::NativeViewAndroid {...}
and make the typedef gfx::NativeView point to this new gfx::NativeViewAndroid.
I hope there is a better way...
From what I understand I introduced circular dependencies because ViewAndroid::SetAncorRect https://cs.chromium.org/chromium/src/ui/android/view_android.cc?rcl=0&l=125 uses Display and now display_android_manager wants to use ViewAndroid. Shall SetAncorRect go to jni instead?
oh deps fun.. Looking at desktop: ui/display depends on ui/gfx and not the other way around. NativeWidget/Window (typedef-ed to View/WindowAndroid pointers on android) are declared in gfx. *But* the actual implementation of NativeWidget doesn't need to be in gfx and can life in a higher layer, and indeed on aura, it uses ui/display stuff as well for android, ui/android is layered above ui/display right now window/view_android are in ui/android, and currently as written, display_android_manager is in ui/display, and they are circularly depending on each other right now The simplest fix would be to move display_android_manager up to ui/android, which is exactly what I said earlier should be avoided. jni stuff generally doesn't care much about layers here I think. sigh sigh sigh, guess there is no way around it :( Another solution would be to not have display_android_manager depend on window/view_android, which means the call for getting the window_android (or the display id I guess) needs to be *declared* in ui/gfx, which works as well I guess
On 2016/11/02 19:19:28, boliu wrote: > The simplest fix would be to move display_android_manager up to ui/android, > which is exactly what I said earlier should be avoided. jni stuff generally > doesn't care much about layers here I think. sigh sigh sigh, guess there is no > way around it :( Right now screen.cc calls CreateNativeScreen(), this will no longer work, I guess? Probably not a very big problem since there exists SetScreenInstance(). > the call for getting the window_android (or the > display id I guess) needs to be *declared* in ui/gfx, which works as well I > guess This is what I wanted to ask about: what portion of ViewAndroid and WindowAndroid should be moved to gfx?
On 2016/11/02 19:56:57, Tima Vaisburd wrote: > On 2016/11/02 19:19:28, boliu wrote: > > The simplest fix would be to move display_android_manager up to ui/android, > > which is exactly what I said earlier should be avoided. jni stuff generally > > doesn't care much about layers here I think. sigh sigh sigh, guess there is no > > way around it :( > > Right now screen.cc calls CreateNativeScreen(), this will no longer work, I > guess? > Probably not a very big problem since there exists SetScreenInstance(). Yep looks like it. So if you go down that route, will need an explicit init > > > the call for getting the window_android (or the > > display id I guess) needs to be *declared* in ui/gfx, which works as well I > > guess > > This is what I wanted to ask about: what portion of ViewAndroid and > WindowAndroid > should be moved to gfx? minimal set needed by ui/display, actually it could be declared in ui/display rather than ui/gfx.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I moved display_android_manager to ui/android/. In my local testing this CL worked for WebView, but did not work for Chrome since in Chrome the Java WindowAndroid is created before the C++ libs are loaded.
Going to signal that the jni is loaded somehow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2016/11/04 01:26:33, Tima Vaisburd wrote: > Going to signal that the jni is loaded somehow. The issue is we can't call nativeInit in the constructor? well that's super brittle... I guess revert the createForTesting change then?
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Signaled the JNI initialization with native pointer in DisplayAndroidManager.java. Now we update the native side only after we have this pointer. Cleaned up the naming in the public interface. Now tested with both Chrome(monochrome) and WebView. Please take another look. https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... File ui/display/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/70001/ui/display/android/disp... ui/display/android/display_android_manager.cc:131: static void UpdateDisplay(JNIEnv* env, On 2016/10/31 23:36:15, Tima Vaisburd wrote: > On 2016/10/27 22:17:49, boliu wrote: > > On 2016/10/27 19:51:51, mthiesse wrote: > > > On 2016/10/27 17:56:24, Tima Vaisburd wrote: > > > > On 2016/10/27 14:30:30, mthiesse wrote: > > > > > Why is this static? It's only called from a non-static function, right? > > > > > > > > I did this to avoid propagating the native object pointer to > > > > DisplayAndroidManager.java. Maybe I should have. > > > > > > Just my opinion, but I think it would be better to pass the native pointer > and > > > not have these static. > > > > I'm fine with static here. I don't think the assumption that screen is global > > will change any time soon > > Kept static. Back to native pointer is PS 13. The native pointer indicates that the JNI is ready, and since we have it we can use it for communication. https://codereview.chromium.org/2416403002/diff/230001/ui/android/display_and... File ui/android/display_android_manager.h (right): https://codereview.chromium.org/2416403002/diff/230001/ui/android/display_and... ui/android/display_android_manager.h:17: class DisplayAndroidManager : public display::Screen { I had to resurrect this header file, otherwise DisplayAndroidManager_jni.h did not compile, now it dereferences DisplayAndroidManager *. I did not put this file in BUILD.gn at all, the interface header file is now ui/android/scheen_android.h.
On 2016/11/04 02:13:26, boliu wrote: > The issue is we can't call nativeInit in the constructor? well that's super > brittle... > > I guess revert the createForTesting change then? M-mmm... As far as I understand, the problem only happens when we try access the jni before the native libraries are loaded. WindowAndroid.createForTesting() is only called from C++, which means that the library exists?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { we don't ever use enums on android because it it's bigger and slower than ints also why is this needed? I think the previous code that have DisplayAndroid.updateFromDisplay call back to DiplayAndroidManager is fine. https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); what if we also create the screen inside RegisterScreenAndroid? a bit unidiomatic, but seems much easier?
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { On 2016/11/07 17:05:33, boliu wrote: > we don't ever use enums on android because it it's bigger and slower than ints > My original intent was to use a bitmask, but then I found this: http://stackoverflow.com/questions/14281827/correct-usage-of-bitmask, and followed it. I would be happy to use the proper bitmask or just the proper way to pass that kind of info between two Java methods (what is it?). > also why is this needed? I think the previous code that have > DisplayAndroid.updateFromDisplay call back to DiplayAndroidManager is fine. I think the elimination of the call back makes the code more straightforward. It also adds some flexibility: I intentionally did not notify observers on addDisplay(). Before this change the code always went to check the changes and to call observers, and it seems that in addDisplay() the change depended on the good guess of initial parameters. If you say that the end result is heavier/uglier than before I'll revert, of course. https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); On 2016/11/07 17:05:33, boliu wrote: > what if we also create the screen inside RegisterScreenAndroid? a bit > unidiomatic, but seems much easier? I thought we wanted to be similar to other platforms... I will try this idea.
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { On 2016/11/07 18:26:04, Tima Vaisburd wrote: > On 2016/11/07 17:05:33, boliu wrote: > > we don't ever use enums on android because it it's bigger and slower than ints > > > My original intent was to use a bitmask, but then I found this: > http://stackoverflow.com/questions/14281827/correct-usage-of-bitmask, > and followed it. I would be happy to use the proper bitmask or just the proper > way to pass that kind of info between two Java methods (what is it?). > > > also why is this needed? I think the previous code that have > > DisplayAndroid.updateFromDisplay call back to DiplayAndroidManager is fine. > > I think the elimination of the call back makes the code more straightforward. It > also adds some flexibility: I intentionally did not notify observers on > addDisplay(). Before this change the code always went to check the changes and > to call observers, and it seems that in addDisplay() the change depended on the > good guess of initial parameters. > > If you say that the end result is heavier/uglier than before I'll revert, of > course. > revert
Patchset #17 (id:310001) has been deleted
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { On 2016/11/07 18:46:29, boliu wrote: > revert I tried, but it did not go well. I did not realize that I never tested the versions before the split. A simple attempt to revert led to recursion: DisplayAndroidManager.getInstance() creates the instance DisplayAndroidManager() creates initial DisplayAndroid and adds it to the map addAndroid() calls updateFromDisplay() which wants to call back DisplayAndroid.updateFromDisplay() calls DisplayAndroidManager.getInstance() but the global variable is not assigned yet so it creates another DisplayManagerObject etc. To cope with this problem I added DisplayManager.initialize() so that the global object would exist at the time of the callback, and I was able to run PS17. However, now I would prefer the split. What do you think?
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { On 2016/11/07 23:52:24, Tima Vaisburd wrote: > On 2016/11/07 18:46:29, boliu wrote: > > revert > > I tried, but it did not go well. I did not realize that I never tested the > versions before the split. A simple attempt to revert led to recursion: > > DisplayAndroidManager.getInstance() creates the instance > DisplayAndroidManager() creates initial DisplayAndroid > and adds it to the map > addAndroid() calls updateFromDisplay() which wants to call back > DisplayAndroid.updateFromDisplay() > calls DisplayAndroidManager.getInstance() > but the global variable is not assigned yet so > it creates another DisplayManagerObject etc. > > To cope with this problem I added DisplayManager.initialize() so that the global > object would exist at the time of the callback, and I was able to run PS17. > However, now I would prefer the split. What do you think? > perhaps that suggests the jni should happen on DisplayAndroid.java, instead of having to pass all these values back and forth between DisplayAndroid and DisplayAndroidManager
On 2016/11/08 00:02:47, boliu wrote: > perhaps that suggests the jni should happen on DisplayAndroid.java, instead of > having to pass all these values back and forth between DisplayAndroid and > DisplayAndroidManager I see. The stuff like remove display and set primary display id seem to belong to the manager though, should we have two jni channels?
On 2016/11/08 01:53:58, Tima Vaisburd wrote: > On 2016/11/08 00:02:47, boliu wrote: > > perhaps that suggests the jni should happen on DisplayAndroid.java, instead of > > having to pass all these values back and forth between DisplayAndroid and > > DisplayAndroidManager > > I see. The stuff like remove display and set primary display id seem to belong > to the manager though, > should we have two jni channels? whatever is easiest..
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:44: public enum PropertyChange { On 2016/11/08 00:02:47, boliu wrote: > On 2016/11/07 23:52:24, Tima Vaisburd wrote: > > On 2016/11/07 18:46:29, boliu wrote: > > > revert > > > > I tried, but it did not go well. I did not realize that I never tested the > > versions before the split. A simple attempt to revert led to recursion: > > > > DisplayAndroidManager.getInstance() creates the instance > > DisplayAndroidManager() creates initial DisplayAndroid > > and adds it to the map > > addAndroid() calls updateFromDisplay() which wants to call back > > DisplayAndroid.updateFromDisplay() > > calls DisplayAndroidManager.getInstance() > > but the global variable is not assigned yet so > > it creates another DisplayManagerObject etc. > > > > To cope with this problem I added DisplayManager.initialize() so that the > global > > object would exist at the time of the callback, and I was able to run PS17. > > However, now I would prefer the split. What do you think? > > > > perhaps that suggests the jni should happen on DisplayAndroid.java, instead of > having to pass all these values back and forth between DisplayAndroid and > DisplayAndroidManager I came up with DisplayAndroid holding the native pointer to display_android_manager and calling it through the static native method in DisplayAndroidManager. This seemed simplest, although I can have another JNI channel DisplayAndroid -> display_android.cc. Please take another look? https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); On 2016/11/07 18:26:04, Tima Vaisburd wrote: > On 2016/11/07 17:05:33, boliu wrote: > > what if we also create the screen inside RegisterScreenAndroid? a bit > > unidiomatic, but seems much easier? > > I thought we wanted to be similar to other platforms... I will try this idea. Couldn't do it: in Chrome the call to Java_DisplayAndroidManager_onNativeSideCreated() failed because it missed the command line object. In WebView, the JNI registration is skipped (btw, how does it work?), so RegisterScreenAndroid is not called at all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); On 2016/11/09 01:17:01, Tima Vaisburd wrote: > On 2016/11/07 18:26:04, Tima Vaisburd wrote: > > On 2016/11/07 17:05:33, boliu wrote: > > > what if we also create the screen inside RegisterScreenAndroid? a bit > > > unidiomatic, but seems much easier? > > > > I thought we wanted to be similar to other platforms... I will try this idea. > > Couldn't do it: in Chrome the call to > Java_DisplayAndroidManager_onNativeSideCreated() failed because it missed the > command line object. meh, init is so complicated how about lazily when the primary display is registered? as you can see, if you want to add an explicit init call, then you need to add that call in *a lot* of places.. > In WebView, the JNI registration is skipped (btw, how does > it work?), so RegisterScreenAndroid is not called at all. Oh yeah.. forgot, jni registration is lazy in webview https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:339: if (mNativeManagerPointer == 0) return; can you explain what the previous problem is? holding the native pointer and calling native methods outside of the actual class is a really really weird pattern
https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/270001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); On 2016/11/09 03:34:25, boliu wrote: > On 2016/11/09 01:17:01, Tima Vaisburd wrote: > > On 2016/11/07 18:26:04, Tima Vaisburd wrote: > > > On 2016/11/07 17:05:33, boliu wrote: > > > > what if we also create the screen inside RegisterScreenAndroid? a bit > > > > unidiomatic, but seems much easier? > > > > > > I thought we wanted to be similar to other platforms... I will try this > idea. > > > > Couldn't do it: in Chrome the call to > > Java_DisplayAndroidManager_onNativeSideCreated() failed because it missed the > > command line object. > > meh, init is so complicated > > how about lazily when the primary display is registered? > > as you can see, if you want to add an explicit init call, then you need to add > that call in *a lot* of places.. > > > In WebView, the JNI registration is skipped (btw, how does > > it work?), so RegisterScreenAndroid is not called at all. > > Oh yeah.. forgot, jni registration is lazy in webview Sigh.. maybe in some common part in the content layer initialization? https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:339: if (mNativeManagerPointer == 0) return; On 2016/11/09 03:34:25, boliu wrote: > can you explain what the previous problem is? holding the native pointer and > calling native methods outside of the actual class is a really really weird > pattern When updateDisplayOnNativeSide() is called for the first time the call is initiated by the DisplayAndroidManager constructor which, in turn, is initiated by DisplayAndroidManager.getInstance(). At this time we cannot reach the manager object from outside, because the singleton pointer is not assigned yet. Last time it worked because there was no callback, all the calls were initiated within DisplayAndroidManager. Now it works because I call a static method of DisplayAndroidManager. I can try to call a native method "within" (as far as I get native methods are just C functions, what make them "belong" to the class are our code generation rules only), there will be static native method that will call into display_android.cc and from there I will call display_android_manager.cc. Does it sound better? Just realized: In c++ the call from display_android can assign the Screen pointer...
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #21 (id:410001) has been deleted
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #24 (id:490001) has been deleted
Patchset #23 (id:470001) has been deleted
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I moved the SetScreenInstance() to the content layer, in BrowserMainLoop::PreCreateThreads() and removed it from all Parts. Some tests do not register JNI, therefore I had to create a dummy screen object for testing, it behaves the same as the existing ScreenAndroid behaved without an update from Java side. The tests bots are green, I cannot believe my eyes! https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/370001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:339: if (mNativeManagerPointer == 0) return; On 2016/11/09 09:01:43, Tima Vaisburd wrote: > On 2016/11/09 03:34:25, boliu wrote: > > can you explain what the previous problem is? holding the native pointer and > > calling native methods outside of the actual class is a really really weird > > pattern > > When updateDisplayOnNativeSide() is called for the first time the call is > initiated by the DisplayAndroidManager constructor which, in turn, is initiated > by DisplayAndroidManager.getInstance(). > At this time we cannot reach the manager object from outside, because the > singleton pointer is not assigned yet. > > Last time it worked because there was no callback, all the calls were initiated > within DisplayAndroidManager. > Now it works because I call a static method of DisplayAndroidManager. > > I can try to call a native method "within" (as far as I get native methods are > just C functions, what make them "belong" to the class are our code generation > rules only), there will be static native method that will call into > display_android.cc and from there I will call display_android_manager.cc. Does > it sound better? > > Just realized: In c++ the call from display_android can assign the Screen > pointer... Returned to the original solution with separate DisplayAndroidManager constructor and initialize(), as discussed offline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looking at the number of linux-only callsites code search can find for Screen::SetScreenInstance, there's a scary number of test suites on that list.. maybe not all of them runs on android, but definitely possible to miss things even if trybots are green, so I guess be prepared to react quickly, or have your CL reverted due to places that it's missed.. https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... content/browser/browser_main_loop.cc:742: display::Screen::SetScreenInstance(ui::CreateScreenAndroid()); linux puts this in PreMainMessageLoopRun
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... content/browser/browser_main_loop.cc:742: display::Screen::SetScreenInstance(ui::CreateScreenAndroid()); On 2016/11/11 19:54:48, boliu wrote: > linux puts this in PreMainMessageLoopRun Sometimes it is in PreCreateThreads, though under USE_AURA: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... https://cs.chromium.org/chromium/src/chromecast/browser/cast_browser_main_par... I moved, assuming USE_AURA and OS_ANDROID are mutually exclusive and I won't override the existing setting. However, doesn't PreCreateThreads feel safer?
https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... content/browser/browser_main_loop.cc:742: display::Screen::SetScreenInstance(ui::CreateScreenAndroid()); On 2016/11/11 21:32:25, Tima Vaisburd wrote: > On 2016/11/11 19:54:48, boliu wrote: > > linux puts this in PreMainMessageLoopRun > > Sometimes it is in PreCreateThreads, though under USE_AURA: > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... > https://cs.chromium.org/chromium/src/chromecast/browser/cast_browser_main_par... > > I moved, assuming USE_AURA and OS_ANDROID are mutually exclusive and I won't > override the existing setting. However, doesn't PreCreateThreads feel safer? it hurts start up perf more
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ah, I did forget to publish! https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2416403002/diff/550001/content/browser/browse... content/browser/browser_main_loop.cc:742: display::Screen::SetScreenInstance(ui::CreateScreenAndroid()); On 2016/11/11 21:36:10, boliu wrote: > On 2016/11/11 21:32:25, Tima Vaisburd wrote: > > On 2016/11/11 19:54:48, boliu wrote: > > > linux puts this in PreMainMessageLoopRun > > > > Sometimes it is in PreCreateThreads, though under USE_AURA: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... > > > https://cs.chromium.org/chromium/src/chromecast/browser/cast_browser_main_par... > > > > I moved, assuming USE_AURA and OS_ANDROID are mutually exclusive and I won't > > override the existing setting. However, doesn't PreCreateThreads feel safer? > > it hurts start up perf more Done. The relevant tests seem to pass again.
https://codereview.chromium.org/2416403002/diff/570001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:20: "dummy_screen_android.cc", if this is test only, then make a test-only target and make only tests depend on it this should definitely not be part of production code https://codereview.chromium.org/2416403002/diff/570001/ui/android/display_and... File ui/android/display_android_manager.h (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/display_and... ui/android/display_android_manager.h:17: class DisplayAndroidManager : public display::Screen { this doesn't need a header file I thought, just put the declaration in the cc file as well? https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:123: public int getSdkDisplayId() { I prefer to keep new methods package visible until something outside of ui actually needs it from java https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:261: mBackend.startListening(); hmm... this can still happen in initialize, right? there is no reason why the java side can't work without the native side? https://codereview.chromium.org/2416403002/diff/570001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); I think it's better for these to be SetScreenAndroid and return void. Anything that wants to get access to this still needs to go through the cross-platform code
https://codereview.chromium.org/2416403002/diff/570001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:20: "dummy_screen_android.cc", On 2016/11/12 00:34:13, boliu wrote: > if this is test only, then make a test-only target and make only tests depend on > it > > this should definitely not be part of production code Done. https://codereview.chromium.org/2416403002/diff/570001/ui/android/display_and... File ui/android/display_android_manager.h (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/display_and... ui/android/display_android_manager.h:17: class DisplayAndroidManager : public display::Screen { On 2016/11/12 00:34:13, boliu wrote: > this doesn't need a header file I thought, just put the declaration in the cc > file as well? display_android_manager.cc did not compile without this separation: #include "jni/DisplayAndroidManager_jni.h" The file DisplayAndroidManager_jni.h references DisplayAndroidManager, e.g. DisplayAndroidManager* native = reinterpret_cast<DisplayAndroidManager*>(nativeDisplayAndroidManager); return native->UpdateDisplay([...]); and complains it does not know what DisplayAndroidManager is. https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:123: public int getSdkDisplayId() { On 2016/11/12 00:34:13, boliu wrote: > I prefer to keep new methods package visible until something outside of ui > actually needs it from java Done. https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java:261: mBackend.startListening(); On 2016/11/12 00:34:13, boliu wrote: > hmm... this can still happen in initialize, right? there is no reason why the > java side can't work without the native side? yes, I think it would work either way. Moved up. https://codereview.chromium.org/2416403002/diff/570001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/570001/ui/android/screen_andr... ui/android/screen_android.h:19: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroid(); On 2016/11/12 00:34:13, boliu wrote: > I think it's better for these to be SetScreenAndroid and return void. Anything > that wants to get access to this still needs to go through the cross-platform > code Done.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2416403002/diff/590001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2416403002/diff/590001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:33: void DisplayToScreenInfo(const display::Display& display, ScreenInfo* results) { fwiw, changes in this file seem independent, could pull it out into separate CL? https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... File ui/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... ui/android/display_android_manager.cc:25: if (display::Screen::GetScreen()) this should be a DCHECK (or if you feel unsafe, a CHECK), it should not be a run time check that just swallows the error https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... File ui/android/display_android_manager.h (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... ui/android/display_android_manager.h:19: DisplayAndroidManager(); can this be private, and then friend SetScreenAndroid? https://codereview.chromium.org/2416403002/diff/590001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/screen_andr... ui/android/screen_android.h:23: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroidForTests(); this should move to a test-only header
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroid.java. This CL also creates ScreenInfo for the right display. BUG=620929, 625089 ========== to ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroid.java. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
https://codereview.chromium.org/2416403002/diff/590001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2416403002/diff/590001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:33: void DisplayToScreenInfo(const display::Display& display, ScreenInfo* results) { On 2016/11/14 15:48:53, boliu wrote: > fwiw, changes in this file seem independent, could pull it out into separate CL? Yes, created https://codereview.chromium.org/2499173002 https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... File ui/android/display_android_manager.cc (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... ui/android/display_android_manager.cc:25: if (display::Screen::GetScreen()) On 2016/11/14 15:48:53, boliu wrote: > this should be a DCHECK (or if you feel unsafe, a CHECK), it should not be a run > time check that just swallows the error I think DCHECK is better. Replaced with DCHECK. https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... File ui/android/display_android_manager.h (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/display_and... ui/android/display_android_manager.h:19: DisplayAndroidManager(); On 2016/11/14 15:48:53, boliu wrote: > can this be private, and then friend SetScreenAndroid? Done. https://codereview.chromium.org/2416403002/diff/590001/ui/android/screen_andr... File ui/android/screen_android.h (right): https://codereview.chromium.org/2416403002/diff/590001/ui/android/screen_andr... ui/android/screen_android.h:23: UI_ANDROID_EXPORT display::Screen* CreateScreenAndroidForTests(); On 2016/11/14 15:48:54, boliu wrote: > this should move to a test-only header Added new file "ui/android/screen_android_for_tests.h"
Description was changed from ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroid.java. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroidManager.java. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
Please write a better CL description. Summarize what's being done here. Point out the design decisions made (need explicit init, layering issues, default display etc). Also make a note that this may miss more test entry points https://codereview.chromium.org/2416403002/diff/610001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/610001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:68: "screen_android_for_tests.h", just match the cc file, so dummy_screen_android.h
Description was changed from ========== Android: support multiple displays on C++ side Maintain a map of integer display ID to Display object in ScreenAndroid. Update this map through DisplayAndroidManager.java. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I rewrote the description and now it might be too long. PTAL again? https://codereview.chromium.org/2416403002/diff/610001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2416403002/diff/610001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:68: "screen_android_for_tests.h", On 2016/11/14 20:49:59, boliu wrote: > just match the cc file, so dummy_screen_android.h Done, I also renamed the creation function to CreateDummyScreenAndroid() that seems more logical now.
I wish all CL descriptions are like this. > This is prerequisite for https://codereview.chromium.org/2499173002 Why is that?
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. This is prerequisite for https://codereview.chromium.org/2499173002 BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ==========
On 2016/11/14 22:23:18, boliu wrote: > I wish all CL descriptions are like this. > > > This is prerequisite for https://codereview.chromium.org/2499173002 > > Why is that? You are right, removed this line. Do you see some other work that needs to be done on this CL?
On 2016/11/14 22:45:58, Tima Vaisburd wrote: > On 2016/11/14 22:23:18, boliu wrote: > > I wish all CL descriptions are like this. > > > > > This is prerequisite for https://codereview.chromium.org/2499173002 > > > > Why is that? > > You are right, removed this line. > > Do you see some other work that needs to be done on this CL? nope, lgtm
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This Cl places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and require some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ==========
timav@chromium.org changed reviewers: + derat@chromium.org, sky@chromium.org
I realized that ui/display/android can be deleted. +sky@: could you, please, provide the owner review for content/public/test changes? +derat@: could you, please, provide the owner review for ui/display changes? Thank you.
lgtm for ui/display
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
During rebase I discovered that in https://codereview.chromium.org/2491563002 ScreenAndroid was made derived from ScreenBase, i.e inheritance became Screen -> ScreenBase -> ScreenAndroid I kept DisplayAndroidManager to inherit directly from Screen, i.e. display::Screen -> ui::DisplayAndroidManager because, as far as I understand, the collection of displays does not form a single virtual screen on Android and therefore the requests for point/rect do not make sense on this platform, and because we do not support native DisplayObservers on Android. If these assumptions are wrong, we might need to inherit from ScreenBase in a subsequent CL.
On 2016/11/15 01:57:29, Tima Vaisburd wrote: > During rebase I discovered that in https://codereview.chromium.org/2491563002 > ScreenAndroid was made derived from ScreenBase, i.e inheritance became > Screen -> ScreenBase -> ScreenAndroid > > I kept DisplayAndroidManager to inherit directly from Screen, i.e. > display::Screen -> ui::DisplayAndroidManager > because, as far as I understand, the collection of displays does not form a > single virtual screen on Android and therefore the requests for point/rect do > not make sense on this platform, and because we do not support native > DisplayObservers on Android. > > If these assumptions are wrong, we might need to inherit from ScreenBase in a > subsequent CL. Doesn't look like ScreenBase adds any new APIs. So I don't see any reason why android shouldn't keep using it. Anything that doesn't make sense on android can just be overridden? Although which ones in particular do you thin do not work?
On 2016/11/15 02:12:30, boliu wrote: > On 2016/11/15 01:57:29, Tima Vaisburd wrote: > > During rebase I discovered that in https://codereview.chromium.org/2491563002 > > ScreenAndroid was made derived from ScreenBase, i.e inheritance became > > Screen -> ScreenBase -> ScreenAndroid > > > > I kept DisplayAndroidManager to inherit directly from Screen, i.e. > > display::Screen -> ui::DisplayAndroidManager > > because, as far as I understand, the collection of displays does not form a > > single virtual screen on Android and therefore the requests for point/rect do > > not make sense on this platform, and because we do not support native > > DisplayObservers on Android. > > > > If these assumptions are wrong, we might need to inherit from ScreenBase in a > > subsequent CL. > > Doesn't look like ScreenBase adds any new APIs. So I don't see any reason why > android shouldn't keep using it. Anything that doesn't make sense on android can > just be overridden? Although which ones in particular do you thin do not work? GetDisplayNearestPoint(gfx::Point) https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=46 GetDisplayMatching(gfx::Rect) https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=58 They use bounds(), but IIUC all bounds on android will start with (0,0). GetDisplayMatching() does not take into account the difference between rects, so (it seems) I cannot use it to ask question like "which display is closest to this size". And, if we use the ScreenBase we should probably use its |display_list_| instead of |displays_| and I'd love to do this in another CL.
On 2016/11/15 02:52:38, Tima Vaisburd wrote: > On 2016/11/15 02:12:30, boliu wrote: > > On 2016/11/15 01:57:29, Tima Vaisburd wrote: > > > During rebase I discovered that in > https://codereview.chromium.org/2491563002 > > > ScreenAndroid was made derived from ScreenBase, i.e inheritance became > > > Screen -> ScreenBase -> ScreenAndroid > > > > > > I kept DisplayAndroidManager to inherit directly from Screen, i.e. > > > display::Screen -> ui::DisplayAndroidManager > > > because, as far as I understand, the collection of displays does not form a > > > single virtual screen on Android and therefore the requests for point/rect > do > > > not make sense on this platform, and because we do not support native > > > DisplayObservers on Android. > > > > > > If these assumptions are wrong, we might need to inherit from ScreenBase in > a > > > subsequent CL. > > > > Doesn't look like ScreenBase adds any new APIs. So I don't see any reason why > > android shouldn't keep using it. Anything that doesn't make sense on android > can > > just be overridden? Although which ones in particular do you thin do not work? > > GetDisplayNearestPoint(gfx::Point) > https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=46 > > GetDisplayMatching(gfx::Rect) > https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=58 > > They use bounds(), but IIUC all bounds on android will start with (0,0). > GetDisplayMatching() does not take into account the difference between rects, > so (it seems) I cannot use it to ask question like "which display is closest to > this size". Yeah, so override those to be NOTIMPLEMENTED > > And, if we use the ScreenBase we should probably use its |display_list_| instead > of |displays_| > and I'd love to do this in another CL. Seems like a bad idea. Doesn't seem that hard to switch to ScreenBase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2016/11/15 03:01:23, boliu wrote: > On 2016/11/15 02:52:38, Tima Vaisburd wrote: > > > > GetDisplayNearestPoint(gfx::Point) > > https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=46 > > > > GetDisplayMatching(gfx::Rect) > > https://cs.chromium.org/chromium/src/ui/display/screen_base.cc?rcl=0&l=58 > > > > They use bounds(), but IIUC all bounds on android will start with (0,0). > > GetDisplayMatching() does not take into account the difference between rects, > > so (it seems) I cannot use it to ask question like "which display is closest > to > > this size". > > Yeah, so override those to be NOTIMPLEMENTED > > > > > And, if we use the ScreenBase we should probably use its |display_list_| > instead > > of |displays_| > > and I'd love to do this in another CL. > > Seems like a bad idea. Doesn't seem that hard to switch to ScreenBase. Switched. Do you want to take a look at it?
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2416403002/#ps690001 (title: "display_android_manager inherits from screen_base")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
stop spamming the trybot.. after it fails twice on the exact same thing, probably should look into why: crbug.com/665595
Message was sent while issue was closed.
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ==========
Message was sent while issue was closed.
Committed patchset #32 (id:690001)
Message was sent while issue was closed.
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ==========
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310}
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:690001) has been created in https://codereview.chromium.org/2514633002/ by timav@chromium.org. The reason for reverting is: Revert to see whether it fixes failures in webkit_tests, e.g. https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr... see http://crbug.com/666420 unexpected_failures: svg/batik/text/textAnchor3.svg images/paletted-png-with-color-profile.html svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg svg/as-background-image/svg-width-100p-as-background.html.
Message was sent while issue was closed.
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ========== to ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ==========
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: support multiple displays on C++ side Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ========== to ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ==========
Description was changed from ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 Cr-Commit-Position: refs/heads/master@{#432310} ========== to ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ==========
Description was changed from ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ========== to ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ==========
The CQ bit was unchecked by timav@chromium.org
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, boliu@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2416403002/#ps710001 (title: "Rebased and fixed webkit_tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 710001, "attempt_start_ts": 1479862270352700,
"parent_rev": "36e13e2b425e43e8955d4d26be20f090b1ad8cb7", "commit_rev":
"28a3a4662965309deb2983f64a89551d6c5461c4"}
Message was sent while issue was closed.
Description was changed from ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ========== to ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ==========
Message was sent while issue was closed.
Committed patchset #33 (id:710001)
Message was sent while issue was closed.
Description was changed from ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} ========== to ========== Reland of Android: support multiple displays on C++ side Original description: Starting with API 17 Android can have multiple displays. CL https://codereview.chromium.org/2361633002 introduced the map of DisplayAndroid objects on Java side. This CL uses the information from Java layer to maintain a similar map on the native side. Similar to other platforms, an individual display is represented by a Display object, and the collection of all displays in the system is the Screen object. The Screen object for Android maintains the mapping between the Android display ID and Display and receives updates from DisplayAndroidManager.java through JNI. Therefore the Screen implementation is placed in DisplayAndroidManager class. The Screen interface assumes the existence of the primary display. To support this we always add the primary display on the Java side (it is propagated to native) during the initialization of DisplayAndroidManager.java Native DisplayAndroidManager obtains the display ID from WindowAndroid and thus depends on /ui/android. This CL places the manager in /ui/android as well. Because of this we have to explicitly initialize the Screen singleton by calling SetScreenAndroid() (similar to other platforms) instead of creating the Screen object on demand. The explicit initialization of the Screen happens on most platforms, and required some tests modification. Note: the explicit initialization of the Android Screen object required some tests modification. Since the screen used to be created on demand, we could have missed some tests that run on Android and not covered by CQ. BUG=625089 > > Committed: https://crrev.com/3da850c3bfadcf3d83407bb4aa9b1e047cbd44a8 > > Cr-Commit-Position: refs/heads/master@{#432310} > Revert: > Committed: https://crrev.com/ece04082b79fd30261b48ad895e7f05beeda7823 > Cr-Commit-Position: refs/heads/master@{#433108} Committed: https://crrev.com/fe53c2c64abb741b886b16b9a8c725ed397be99f Cr-Commit-Position: refs/heads/master@{#434090} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/fe53c2c64abb741b886b16b9a8c725ed397be99f Cr-Commit-Position: refs/heads/master@{#434090} |
