Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300463002:
Add observers for DIP scale change. (Closed)
|
Created:
4 years, 3 months ago by Tima Vaisburd Modified:
4 years, 1 month ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, darin-cc_chromium.org, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd observers for DIP scale change.
Extended Java DisplayAndroidObserver interface to receive
the DIP scale change notifications.
Added two such observers: ContentViewCore and AwContents.
Upon receiving the notification ContentViewCore now sets the DIP
scale both on Java and native side.
It happens
1. During initialization,
2. When ContentViewCore is attached to a window, either for
the first time or due to reparenting,
3. Due to notification from observer.
In cases (2) and (3) we force layout.
BUG=620929
Committed: https://crrev.com/9483b39990200554ba76602455869962005ace0e
Cr-Commit-Position: refs/heads/master@{#430536}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase only #Patch Set 3 : DIPScaleListener listen itself, add observer early in AwContents #
Total comments: 15
Patch Set 4 : Moved the listener to WindowAndroid, some minor changes #
Total comments: 8
Patch Set 5 : Made observer interface top level class and renamed to listener #
Total comments: 11
Patch Set 6 : Start listening on first attached observer and stop on last removed one. #Patch Set 7 : An attempt to fix tests compilation #
Total comments: 61
Patch Set 8 : Addressed comments, more questions. #
Total comments: 4
Patch Set 9 : Made DisplayObserver private to ContentViewCore #Patch Set 10 : Added getDIPScale(context) and removed logs in DIPScaleMonitor #Patch Set 11 : Rebase only #Patch Set 12 : Rebase #Patch Set 13 : Reworked to use DisplayAndroid #Patch Set 14 : Fix test compilation #
Total comments: 1
Patch Set 15 : Do not force layout during initialization. #
Total comments: 18
Patch Set 16 : Addressed comments #
Total comments: 14
Patch Set 17 : Pass DIP scale during CVC c++ Init(), force layout only if DIP scale changed #
Total comments: 2
Patch Set 18 : Attempt to fix unit test #
Total comments: 1
Patch Set 19 : Update the fix #
Total comments: 13
Patch Set 20 : Rebase only #Patch Set 21 : Rebased to master #Messages
Total messages: 119 (65 generated)
Bo, finally I came up with something, could you please verify the approach? Thank you. https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1370: WindowAndroid windowAndroid = getWindowAndroid(); Now I think that if we attach the observers in onAttachedToWindow() and detach in onDetachedFromWindow() then they will be called automatically during window reparenting. Does it make sense? https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1371: if (windowAndroid != null) { Is this possible that there would be no window here? https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:42: private ObserverList<DIPScaleObserver> mObservers = new ObserverList<DIPScaleObserver>(); I forgot to change the comments. Originally I had ArrayList< ObserverList > : observers and the scale per attached Display. I can restore this version, but I think DeviceDisplayInfo does not support this anyway.
boliu@chromium.org changed reviewers: + boliu@chromium.org, yusufo@chromium.org
+yusufo to answer reparent question you need to actually add me to reviewers... I only looked your comments, didn't read through the code. Looking at the description though, config change signal should come from ui layer, not from chrome or android_webview https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1370: WindowAndroid windowAndroid = getWindowAndroid(); On 2016/08/31 01:04:24, Tima Vaisburd wrote: > Now I think that if we attach the observers in onAttachedToWindow() and detach > in onDetachedFromWindow() then they will be called automatically during window > reparenting. Does it make sense? This should work in *most* cases, but what if webview is never attached to window and app just keeps calling onDraw and whatnot? Actually I think we should just break that edge case. Yeah, adding/removing in attach/detach is fine with me https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1371: if (windowAndroid != null) { On 2016/08/31 01:04:24, Tima Vaisburd wrote: > Is this possible that there would be no window here? I dunno. +yusufo about reparenting ContentViewCore: can CVC ever have a null WindowAndroid even during reparenting? https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:42: private ObserverList<DIPScaleObserver> mObservers = new ObserverList<DIPScaleObserver>(); On 2016/08/31 01:04:24, Tima Vaisburd wrote: > I forgot to change the comments. Originally I had ArrayList< ObserverList > : > observers and the scale per attached Display. I can restore this version, but I > think DeviceDisplayInfo does not support this anyway. I don't see a question..
Bo Liu> config change signal should come from ui layer, not from chrome or android_webview I did it because I wanted to turn the listener on android_webview only. https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:42: private ObserverList<DIPScaleObserver> mObservers = new ObserverList<DIPScaleObserver>(); On 2016/08/31 01:58:15, boliu wrote: > On 2016/08/31 01:04:24, Tima Vaisburd wrote: > > I forgot to change the comments. Originally I had ArrayList< ObserverList > : > > observers and the scale per attached Display. I can restore this version, but > I > > think DeviceDisplayInfo does not support this anyway. > > I don't see a question.. never mind, I will fix the comments.
On 2016/08/31 17:48:04, Tima Vaisburd wrote: > Bo Liu> config change signal should come from ui layer, not from chrome or > android_webview > > I did it because I wanted to turn the listener on android_webview only. Build in a separate mechanism in the ui layer. > > https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... > File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): > > https://codereview.chromium.org/2300463002/diff/1/ui/android/java/src/org/chr... > ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:42: private > ObserverList<DIPScaleObserver> mObservers = new > ObserverList<DIPScaleObserver>(); > On 2016/08/31 01:58:15, boliu wrote: > > On 2016/08/31 01:04:24, Tima Vaisburd wrote: > > > I forgot to change the comments. Originally I had ArrayList< ObserverList > > : > > > observers and the scale per attached Display. I can restore this version, > but > > I > > > think DeviceDisplayInfo does not support this anyway. > > > > I don't see a question.. > > never mind, I will fix the comments.
https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1371: if (windowAndroid != null) { On 2016/08/31 01:58:15, boliu wrote: > On 2016/08/31 01:04:24, Tima Vaisburd wrote: > > Is this possible that there would be no window here? > > I dunno. +yusufo about reparenting ContentViewCore: can CVC ever have a null > WindowAndroid even during reparenting? Yes, during tab reparenting where we have detached from the previous activity and have not attached to the new one windowAndroid is null See Tab#detachAndStartReparenting
On 2016/09/01 18:51:23, Yusuf wrote: > https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/2300463002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1371: > if (windowAndroid != null) { > On 2016/08/31 01:58:15, boliu wrote: > > On 2016/08/31 01:04:24, Tima Vaisburd wrote: > > > Is this possible that there would be no window here? > > > > I dunno. +yusufo about reparenting ContentViewCore: can CVC ever have a null > > WindowAndroid even during reparenting? > > Yes, during tab reparenting where we have detached from the previous activity > and have not attached to the new one windowAndroid is null > > See Tab#detachAndStartReparenting Yusuf, thank you. Does it mean that during reparenting the view does not receive View.onDetachedFromWindow()/onAttachedToWindow() calls?
Description was changed from ========== Add listener and observers for DIP scale change. The listener and observers are implemented on Java side. There are currently two observers: ContentViewCore and AwContents. They are added in onAttachedToWindow() and removed in onDetachedFromWindow() methods. The listener assumes there is only one physical display attached to the device. The listener is passive: it receives configuration change signal from AwContents. BUG=620929 ========== to ========== Add listener and observers for DIP scale change. The listener and observers are implemented on Java side. There are currently two observers: ContentViewCore and AwContents. They are added in onAttachedToWindow() and removed in onDetachedFromWindow() methods. Besides, AwContents add itself as an observer during initialization to make DIP scale available early. The listener registers itself to the DisplayManager and gets new DIP scale upon receiving onDisplayChanged(). However, in this implementation the listener assumes there is only one physical display attached to the device. BUG=620929 ==========
Bo> config change signal should come from ui layer, not from chrome or android_webview Done, PTAL. https://codereview.chromium.org/2300463002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2300463002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1065: DIPScaleListener.getInstance().addObserver(this, mWindowAndroid.getWindowAndroid()); This was needed for loadUrl() which happened before onAttachedToWindow() and indirectly called BrowserViewRenderer::UpdateRootLayerState() which required a valid DIP scale.
only glanced at ui https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:81: public static DIPScaleListener getInstance() { this assumes one display, not ok.. if there is no good abstraction for a display in java side yet, maybe just add this to WindowAndroid instead? https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); also don't use the application context https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:163: (DisplayManager) mAppContext.getSystemService(Context.DISPLAY_SERVICE); yuk, this is going to be a big source of potential leaks in webview again :/ although I don't have any advice here, unless android has better APIs https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:206: return displayContext != null ? displayContext : windowAndroid.getApplicationContext(); hmm... this is ok, since it doesn't really matter what happens if the weakref is null (as long as it doesn't crash), but this code suggest using application context is ok, and that's not true. add a comment maybe? https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:213: return (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); this creates way too many java objects to be gc-ed later
Description was changed from ========== Add listener and observers for DIP scale change. The listener and observers are implemented on Java side. There are currently two observers: ContentViewCore and AwContents. They are added in onAttachedToWindow() and removed in onDetachedFromWindow() methods. Besides, AwContents add itself as an observer during initialization to make DIP scale available early. The listener registers itself to the DisplayManager and gets new DIP scale upon receiving onDisplayChanged(). However, in this implementation the listener assumes there is only one physical display attached to the device. BUG=620929 ========== to ========== Add listener and observers for DIP scale change. The listener and observers are implemented on Java side. There are currently two observers: ContentViewCore and AwContents. They are added in onAttachedToWindow() and removed in onDetachedFromWindow() methods. These classes propagate DIP scale to the native side. The listenet is a part of WindowAndroid. Listeners register themselves to the DisplayManager and get new DIP scale upon receiving onDisplayChanged(). BUG=620929 ==========
https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:81: public static DIPScaleListener getInstance() { On 2016/09/08 02:38:35, boliu wrote: > this assumes one display, not ok.. if there is no good abstraction for a display > in java side yet, maybe just add this to WindowAndroid instead? I tried to do this, as we talked. The listener now retrieves the DIP scale and starts listening right in the constructor. It can also provide DIP scale on demand by getDIPScale(). https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); On 2016/09/08 02:38:35, boliu wrote: > also don't use the application context I still use the application context in startListening() and stopListening() to get the DisplayManager, I see no other way around? https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:206: return displayContext != null ? displayContext : windowAndroid.getApplicationContext(); On 2016/09/08 02:38:35, boliu wrote: > hmm... this is ok, since it doesn't really matter what happens if the weakref is > null (as long as it doesn't crash), but this code suggest using application > context is ok, and that's not true. add a comment maybe? Done. https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:213: return (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); On 2016/09/08 02:38:35, boliu wrote: > this creates way too many java objects to be gc-ed later Removed this method. https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:70: public DIPScaleListener(WindowAndroid windowAndroid) { Wasn't sure whether it is better to pass WindowAndroid or WeakReference<Context>
boliu@chromium.org changed reviewers: + gsennton@chromium.org
skimming again.. +gsennton fyi https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); On 2016/09/08 23:37:35, Tima Vaisburd wrote: > On 2016/09/08 02:38:35, boliu wrote: > > also don't use the application context > > I still use the application context in startListening() and stopListening() to > get the DisplayManager, I see no other way around? Why? Do context.getSystemService() instead of context.getApplicationContext().getSystemService()? https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:70: public DIPScaleListener(WindowAndroid windowAndroid) { On 2016/09/08 23:37:36, Tima Vaisburd wrote: > Wasn't sure whether it is better to pass WindowAndroid or WeakReference<Context> Does this need to be public at all? WindowAndroid can just expose the interface to add/remove observer, and everything else is implementation detail. https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:178: // If displayContext does not exist the window is probably detached from view "probably detached from view" part is not correct, it means the activity is activity/context has gone away
A couple of general questions: 1. Do we care how many listeners we register to the Android DisplayManager? (with the current PS it looks like we are registering one per WindowAndroid, while e.g. ScreenOrientationListener only registers one for all of Chrome/WebView). 2. (potentially out of scope of this CL) Should this + ScreenOrientationListener be merged? (I think I was intending to replace ScreenOrientationListener with a new class that would work for several displays - but if we don't care about question 1., maybe what you're doing here (having one listener per WindowAndroid) would be a better/[easier to follow] implementation for multi-display as well. https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); Is there actually a problem with using the application context here? IIRC that was what I was gonna to do register/unregister a listener to the Android DisplayManager since my listener might outlive the "display context". Will you be able to unregister your listener if you are only using the display context? (what if it goes away before you try to unregister?) https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:178: // If displayContext does not exist the window is probably detached from view On 2016/09/09 00:43:20, boliu wrote: > "probably detached from view" part is not correct, it means the activity is > activity/context has gone away Do we have a general idea of when this happens / what state we are in? Are we usually close to shut-down or could it be that we just continue running indefinitely? (doesn't AwContents hold a strong reference to the Context passed to WebView?)
On 2016/09/09 11:27:30, gsennton wrote: > A couple of general questions: > 1. Do we care how many listeners we register to the Android DisplayManager? > (with the current PS it looks like we are registering one per WindowAndroid, > while e.g. ScreenOrientationListener only registers one for all of > Chrome/WebView). Correct. What concern do you have here? > 2. (potentially out of scope of this CL) Should this + ScreenOrientationListener > be merged? Yes! > (I think I was intending to replace ScreenOrientationListener with a > new class that would work for several displays Hmm, what use case is there to be notified of several/all display changes? > - but if we don't care about > question 1., maybe what you're doing here (having one listener per > WindowAndroid) would be a better/[easier to follow] implementation for > multi-display as well. fwiw it should really be one per display, but since we don't have an abstraction for display in java ui/, the best next thing was one per windowandroid https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); On 2016/09/09 11:27:29, gsennton wrote: > Is there actually a problem with using the application context here? IIRC that > was what I was gonna to do register/unregister a listener to the Android > DisplayManager since my listener might outlive the "display context". Will you > be able to unregister your listener if you are only using the display context? > (what if it goes away before you try to unregister?) """ Note: System services obtained via this API may be closely associated with the Context in which they are obtained from. In general, do not share the service objects between various different contexts (Activities, Applications, Services, Providers, etc.) """ It's probably ok given the API, but it's still not great to assume there is one global DisplayManager instance. I mean that's the whole reason we are in the dip mess in the first place, if only chrome had followed this rule everywhere... https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:178: // If displayContext does not exist the window is probably detached from view On 2016/09/09 11:27:29, gsennton wrote: > On 2016/09/09 00:43:20, boliu wrote: > > "probably detached from view" part is not correct, it means the activity is > > activity/context has gone away > > Do we have a general idea of when this happens / what state we are in? Are we > usually close to shut-down or could it be that we just continue running > indefinitely? (doesn't AwContents hold a strong reference to the Context passed > to WebView?) Yeah, it means all AwContents are gone already. Usually it's around/after activity shutdown if it's an activity context, not necessarily the whole app though.
https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:178: // If displayContext does not exist the window is probably detached from view On 2016/09/09 16:21:12, boliu wrote: > On 2016/09/09 11:27:29, gsennton wrote: > > On 2016/09/09 00:43:20, boliu wrote: > > > "probably detached from view" part is not correct, it means the activity is > > > activity/context has gone away > > > > Do we have a general idea of when this happens / what state we are in? Are we > > usually close to shut-down or could it be that we just continue running > > indefinitely? (doesn't AwContents hold a strong reference to the Context > passed > > to WebView?) > > Yeah, it means all AwContents are gone already. Usually it's around/after > activity shutdown if it's an activity context, not necessarily the whole app > though. Well, all AwContents that uses this WindowAndroid
https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); On 2016/09/09 11:27:29, gsennton wrote: > Will you > be able to unregister your listener if you are only using the display context? > (what if it goes away before you try to unregister?) Thank you for pointing this out. I would have to keep the DisplayManager member var to ensure that we unregister with the same object. And thank you for reviewing in general.
https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:104: if (mAppContext == null) mAppContext = displayContext.getApplicationContext(); On 2016/09/09 17:52:26, Tima Vaisburd wrote: > On 2016/09/09 11:27:29, gsennton wrote: > > Will you > > be able to unregister your listener if you are only using the display context? > > (what if it goes away before you try to unregister?) > > Thank you for pointing this out. I would have to keep the DisplayManager member > var to ensure that we unregister with the same object. > > And thank you for reviewing in general. Did in PS5 https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:70: public DIPScaleListener(WindowAndroid windowAndroid) { On 2016/09/09 00:43:20, boliu wrote: > On 2016/09/08 23:37:36, Tima Vaisburd wrote: > > Wasn't sure whether it is better to pass WindowAndroid or > WeakReference<Context> > > Does this need to be public at all? WindowAndroid can just expose the interface > to add/remove observer, and everything else is implementation detail. Renamed DIPScaleListener into DIPScaleMonitor and made it package scope instead of public, renamed DIPScaleListener.Observer to just DIPScaleListener. https://codereview.chromium.org/2300463002/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:178: // If displayContext does not exist the window is probably detached from view On 2016/09/09 16:21:49, boliu wrote: > On 2016/09/09 16:21:12, boliu wrote: > > On 2016/09/09 11:27:29, gsennton wrote: > > > On 2016/09/09 00:43:20, boliu wrote: > > > > "probably detached from view" part is not correct, it means the activity > is > > > > activity/context has gone away > > > > > > Do we have a general idea of when this happens / what state we are in? Are > we > > > usually close to shut-down or could it be that we just continue running > > > indefinitely? (doesn't AwContents hold a strong reference to the Context > > passed > > > to WebView?) > > > > Yeah, it means all AwContents are gone already. Usually it's around/after > > activity shutdown if it's an activity context, not necessarily the whole app > > though. > > Well, all AwContents that uses this WindowAndroid Changed the comment.
Given that the we run into a lot of problems just because we don't want to leak the given Context, is there any way to register some kind of listener/reference to the Context so that we can run some code just *before* the Context is GC'd? (I'm guessing 'no' but it's better to ask than assume :P) https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:56: private static final float EPS = 0.01f; For clarity, could you justify why this value is small enough here? (e.g just an example of different DPI values). https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); It is not ok to store the display manager that was fetched through the display context - the display manager holds on to the context so this would be considered a leak. This kinda sucks, but I guess this whole situation is what it is just because we (WebView) can't put any demands on apps to not leak WebView (since old apps already do that) - and these framework APIs aren't created with our current situation in mind. As Bo pointed out earlier, given the way the DisplayManager API looks (passing a display ID), using the application context here *should* be ok. But, on the other hand this might change in the future :/
> is there any way to register some kind of listener/reference > to the Context so that we can run some code just *before* > the Context is GC'd? Not sure how to do that, can you, please, elaborate? https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:56: private static final float EPS = 0.01f; On 2016/09/12 07:36:35, gsennton wrote: > For clarity, could you justify why this value is small enough here? (e.g just an > example of different DPI values). https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/12 07:36:35, gsennton wrote: > It is not ok to store the display manager that was fetched through the display > context - the display manager holds on to the context so this would be > considered a leak. > > This kinda sucks, but I guess this whole situation is what it is just because we > (WebView) can't put any demands on apps to not leak WebView (since old apps > already do that) - and these framework APIs aren't created with our current > situation in mind. > > As Bo pointed out earlier, given the way the DisplayManager API looks (passing a > display ID), using the application context here *should* be ok. But, on the > other hand this might change in the future :/ This is unfortunate... Before I change the code back, let me ask why can't we rely on the WindowAndroid being eventually destroyed? Since the monitor is not part of WindowAndroid, wouldn't it be properly be destroyed when the activity is gone?
On 2016/09/12 07:36:36, gsennton wrote: > Given that the we run into a lot of problems just because we don't want to leak > the given Context, is there any way to register some kind of listener/reference > to the Context so that we can run some code just *before* the Context is GC'd? > (I'm guessing 'no' but it's better to ask than assume :P) Sure. CleanupReference (or something similar). But that's not what you really what. You really want to make sure that Context is eligible for gc in the first place, and nothing we do prevents that. And that's not possible. https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/12 07:36:35, gsennton wrote: > It is not ok to store the display manager that was fetched through the display > context - the display manager holds on to the context so this would be > considered a leak. > > This kinda sucks, but I guess this whole situation is what it is just because we > (WebView) can't put any demands on apps to not leak WebView (since old apps > already do that) - and these framework APIs aren't created with our current > situation in mind. > > As Bo pointed out earlier, given the way the DisplayManager API looks (passing a > display ID), using the application context here *should* be ok. But, on the > other hand this might change in the future :/ I think the whole gc thing might need some deeper thought.. At this point, all listeners are (or should) be removed in onDetachedFromWindow, and if we remove the DisplayManager ref when all listeners are removed, then that won't cause a leak. Webview has gc tests so you'll know if you break this. I am contemplating if there are more bulletproof ways to prevent leaks (that doesn't involve WeakReference). For Activitiys, can we listen to activity lifetime callback and force cleanup there..?
"Sure. CleanupReference (or something similar). But that's not what you really what. You really want to make sure that Context is eligible for gc in the first place, and nothing we do prevents that. And that's not possible." If we have a way (CleanupReference?) to use the Context just before it becomes GC'd we can just fetch its DisplayManager again, and unregister our DisplayListener? If the Context won't be dropped before we get a call to onDetachedFromWindow then we are fine, right? https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/12 17:06:18, Tima Vaisburd wrote: > On 2016/09/12 07:36:35, gsennton wrote: > > It is not ok to store the display manager that was fetched through the display > > context - the display manager holds on to the context so this would be > > considered a leak. > > > > This kinda sucks, but I guess this whole situation is what it is just because > we > > (WebView) can't put any demands on apps to not leak WebView (since old apps > > already do that) - and these framework APIs aren't created with our current > > situation in mind. > > > > As Bo pointed out earlier, given the way the DisplayManager API looks (passing > a > > display ID), using the application context here *should* be ok. But, on the > > other hand this might change in the future :/ > > This is unfortunate... Before I change the code back, let me ask why can't we > rely on the WindowAndroid being eventually destroyed? Since the monitor is not > part of WindowAndroid, wouldn't it be properly be destroyed when the activity is > gone? Are you talking about the DisplayListener here? I thought that was registered to the DisplayManager so that it doesn't get unregistered unless you do so manually (but I haven't looked at the actual code).
On 2016/09/12 19:03:04, gsennton wrote: > "Sure. CleanupReference (or something similar). But that's not what you really > what. You really want to make sure that Context is eligible for gc in the first > place, and nothing we do prevents that. And that's not possible." > > If we have a way (CleanupReference?) to use the Context just before it becomes > GC'd we can just fetch its DisplayManager again, and unregister our > DisplayListener? > > If the Context won't be dropped before we get a call to onDetachedFromWindow > then we are fine, right? I think you are not understanding how gc works. gc won't just magically clean up your java object for you. An object has to become eligible for gc first, ie not reachable from a gc root. You are assuming here that the DisplayManager is a gc root (probably correct) and the strong ref from it to the Context/WindowAndroid/Listener/whatever needs to dropped. But gc won't help you here at all, because whatever is referenced from DisplayManager won't be eligible for gc in the first place.
https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:56: private static final float EPS = 0.01f; On 2016/09/12 07:36:35, gsennton wrote: > For clarity, could you justify why this value is small enough here? (e.g just an > example of different DPI values). I agree that 0.01 looks too coarse. Changed to 0.0001 and added examples. https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/12 19:03:04, gsennton wrote: > On 2016/09/12 17:06:18, Tima Vaisburd wrote: > > On 2016/09/12 07:36:35, gsennton wrote: > > > It is not ok to store the display manager that was fetched through the > display > > > context - the display manager holds on to the context so this would be > > > considered a leak. > > > > > > This kinda sucks, but I guess this whole situation is what it is just > because > > we > > > (WebView) can't put any demands on apps to not leak WebView (since old apps > > > already do that) - and these framework APIs aren't created with our current > > > situation in mind. > > > > > > As Bo pointed out earlier, given the way the DisplayManager API looks > (passing > > a > > > display ID), using the application context here *should* be ok. But, on the > > > other hand this might change in the future :/ > > > > This is unfortunate... Before I change the code back, let me ask why can't we > > rely on the WindowAndroid being eventually destroyed? Since the monitor is not > > part of WindowAndroid, wouldn't it be properly be destroyed when the activity > is > > gone? > > Are you talking about the DisplayListener here? I thought that was registered to > the DisplayManager so that it doesn't get unregistered unless you do so manually > (but I haven't looked at the actual code) It was about DIPScaleMonitor. I was thinking in terms of C++ destructors which was apparently wrong. In better terms the question was why can't we rely on the automatic cleanup when WindowAndroid object is garbage collected. When WindowAndroid is gc-ed DIPScaleMonitor should be too and if it holds a link to DisplayMonitor service it should release it. I followed Bo's advise and now release the DisplayMonitor when last observer is removed, i.e. when a view is detached from window (I presume window and WindowAndroid are still exist at this moment). https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/12 17:07:11, boliu wrote: > At this point, all listeners are (or should) be removed in onDetachedFromWindow, > and if we remove the DisplayManager ref when all listeners are removed, then > that won't cause a leak. Now I stopListening() when the last observer is removed, and startListening() after the first observer is added.
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...)
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...)
https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/13 00:09:56, Tima Vaisburd wrote: > On 2016/09/12 19:03:04, gsennton wrote: > > On 2016/09/12 17:06:18, Tima Vaisburd wrote: > > > On 2016/09/12 07:36:35, gsennton wrote: > > > > It is not ok to store the display manager that was fetched through the > > display > > > > context - the display manager holds on to the context so this would be > > > > considered a leak. > > > > > > > > This kinda sucks, but I guess this whole situation is what it is just > > because > > > we > > > > (WebView) can't put any demands on apps to not leak WebView (since old > apps > > > > already do that) - and these framework APIs aren't created with our > current > > > > situation in mind. > > > > > > > > As Bo pointed out earlier, given the way the DisplayManager API looks > > (passing > > > a > > > > display ID), using the application context here *should* be ok. But, on > the > > > > other hand this might change in the future :/ > > > > > > This is unfortunate... Before I change the code back, let me ask why can't > we > > > rely on the WindowAndroid being eventually destroyed? Since the monitor is > not > > > part of WindowAndroid, wouldn't it be properly be destroyed when the > activity > > is > > > gone? > > > > Are you talking about the DisplayListener here? I thought that was registered > to > > the DisplayManager so that it doesn't get unregistered unless you do so > manually > > (but I haven't looked at the actual code) > > It was about DIPScaleMonitor. > I was thinking in terms of C++ destructors which was apparently wrong. In better > terms the question was why can't we rely on the automatic cleanup when > WindowAndroid object is garbage collected. When WindowAndroid is gc-ed > DIPScaleMonitor should be too and if it holds a link to DisplayMonitor service > it should release it. > > I followed Bo's advise and now release the DisplayMonitor when last observer is > removed, i.e. when a view is detached from window (I presume window and > WindowAndroid are still exist at this moment). Alright, that means we will prevent the Display Context to be gc'ed until onDetachFromWindow is called, if that is fine I'm ok with this I think :P For clarification: is there a relation between how long the display context lives and the timing for when we will be detached from a window? I assume normally we should be detached before the display context goes away - since the display context will most probably just be the Activity we are attaching to (or rather the Activity that owns the window we attach to), but do we care about any other cases? (such as WebView holding a reference to a context that has nothing to do with the window we attach to).
On 2016/09/12 19:58:01, boliu wrote: > On 2016/09/12 19:03:04, gsennton wrote: > > "Sure. CleanupReference (or something similar). But that's not what you really > > what. You really want to make sure that Context is eligible for gc in the > first > > place, and nothing we do prevents that. And that's not possible." > > > > If we have a way (CleanupReference?) to use the Context just before it becomes > > GC'd we can just fetch its DisplayManager again, and unregister our > > DisplayListener? > > > > If the Context won't be dropped before we get a call to onDetachedFromWindow > > then we are fine, right? > > I think you are not understanding how gc works. gc won't just magically clean up > your java object for you. An object has to become eligible for gc first, ie not > reachable from a gc root. You are assuming here that the DisplayManager is a gc > root (probably correct) and the strong ref from it to the > Context/WindowAndroid/Listener/whatever needs to dropped. But gc won't help you > here at all, because whatever is referenced from DisplayManager won't be > eligible for gc in the first place. I probably didn't make myself clear, what I meant was for us to NOT store a reference to the DisplayManager, and instead re-fetch it using displayContext.getSystemService(..) when we get the callback indicating that the displayContext is eligible for gc - and then unregister the displayListener. So by NOT storing the DisplayManager we are not holding an implicit strong reference to the context and it can be gc'ed. If it is ok to just hold on to the context until onDetachedFromWindow is called that is probably a lot easier to argue about :)
On 2016/09/13 11:42:40, gsennton wrote: > On 2016/09/12 19:58:01, boliu wrote: > > On 2016/09/12 19:03:04, gsennton wrote: > > > "Sure. CleanupReference (or something similar). But that's not what you > really > > > what. You really want to make sure that Context is eligible for gc in the > > first > > > place, and nothing we do prevents that. And that's not possible." > > > > > > If we have a way (CleanupReference?) to use the Context just before it > becomes > > > GC'd we can just fetch its DisplayManager again, and unregister our > > > DisplayListener? > > > > > > If the Context won't be dropped before we get a call to onDetachedFromWindow > > > then we are fine, right? > > > > I think you are not understanding how gc works. gc won't just magically clean > up > > your java object for you. An object has to become eligible for gc first, ie > not > > reachable from a gc root. You are assuming here that the DisplayManager is a > gc > > root (probably correct) and the strong ref from it to the > > Context/WindowAndroid/Listener/whatever needs to dropped. But gc won't help > you > > here at all, because whatever is referenced from DisplayManager won't be > > eligible for gc in the first place. > > I probably didn't make myself clear, what I meant was for us to NOT store a > reference to the DisplayManager, and instead re-fetch it using > displayContext.getSystemService(..) when we get the callback indicating that the > displayContext is eligible for gc - and then unregister the displayListener. That's still the circular I think. displayContext won't become eligible for gc until all observers are removed (assuming that the DisplayManager is actually global and so is a gc root itself, or many other possibilities..) The general pattern of "let's wait for gc and do some clean up on *java*" side is never going work well I think.. > So > by NOT storing the DisplayManager we are not holding an implicit strong > reference to the context and it can be gc'ed. > > If it is ok to just hold on to the context until onDetachedFromWindow is called > that is probably a lot easier to argue about :)
https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:141: getDisplayContext(mWindowAndroid).getSystemService(Context.DISPLAY_SERVICE); On 2016/09/13 11:36:39, gsennton wrote: > On 2016/09/13 00:09:56, Tima Vaisburd wrote: > > On 2016/09/12 19:03:04, gsennton wrote: > > > On 2016/09/12 17:06:18, Tima Vaisburd wrote: > > > > On 2016/09/12 07:36:35, gsennton wrote: > > > > > It is not ok to store the display manager that was fetched through the > > > display > > > > > context - the display manager holds on to the context so this would be > > > > > considered a leak. > > > > > > > > > > This kinda sucks, but I guess this whole situation is what it is just > > > because > > > > we > > > > > (WebView) can't put any demands on apps to not leak WebView (since old > > apps > > > > > already do that) - and these framework APIs aren't created with our > > current > > > > > situation in mind. > > > > > > > > > > As Bo pointed out earlier, given the way the DisplayManager API looks > > > (passing > > > > a > > > > > display ID), using the application context here *should* be ok. But, on > > the > > > > > other hand this might change in the future :/ > > > > > > > > This is unfortunate... Before I change the code back, let me ask why can't > > we > > > > rely on the WindowAndroid being eventually destroyed? Since the monitor is > > not > > > > part of WindowAndroid, wouldn't it be properly be destroyed when the > > activity > > > is > > > > gone? > > > > > > Are you talking about the DisplayListener here? I thought that was > registered > > to > > > the DisplayManager so that it doesn't get unregistered unless you do so > > manually > > > (but I haven't looked at the actual code) > > > > It was about DIPScaleMonitor. > > I was thinking in terms of C++ destructors which was apparently wrong. In > better > > terms the question was why can't we rely on the automatic cleanup when > > WindowAndroid object is garbage collected. When WindowAndroid is gc-ed > > DIPScaleMonitor should be too and if it holds a link to DisplayMonitor service > > it should release it. > > > > I followed Bo's advise and now release the DisplayMonitor when last observer > is > > removed, i.e. when a view is detached from window (I presume window and > > WindowAndroid are still exist at this moment). > > Alright, that means we will prevent the Display Context to be gc'ed until > onDetachFromWindow is called, if that is fine I'm ok with this I think :P That's probably the case with either implementation, since the DisplayManager itself is probably a gc root. If we forget to clean up stuff in onDetachedFromWindow, then things will definitely leak. > > For clarification: is there a relation between how long the display context > lives and the timing for when we will be detached from a window? I assume > normally we should be detached before the display context goes away - since the > display context will most probably just be the Activity we are attaching to (or > rather the Activity that owns the window we attach to), Yep > but do we care about any > other cases? (such as WebView holding a reference to a context that has nothing > to do with the window we attach to). Application/Service are considered to be global. So more of a problem for things like having the Context refer back to webview (rarer), but less of a problem leaking the context (more common) But whatever the case, the assumption with onDetachedFromWindow is that if the webview is attached, then the window/view tree is going to keep the webview alive and not be gc-ed, so then webview keeping the context alive is fine.
On 2016/09/13 15:07:37, boliu wrote: > On 2016/09/13 11:42:40, gsennton wrote: > > On 2016/09/12 19:58:01, boliu wrote: > > > On 2016/09/12 19:03:04, gsennton wrote: > > > > "Sure. CleanupReference (or something similar). But that's not what you > > really > > > > what. You really want to make sure that Context is eligible for gc in the > > > first > > > > place, and nothing we do prevents that. And that's not possible." > > > > > > > > If we have a way (CleanupReference?) to use the Context just before it > > becomes > > > > GC'd we can just fetch its DisplayManager again, and unregister our > > > > DisplayListener? > > > > > > > > If the Context won't be dropped before we get a call to > onDetachedFromWindow > > > > then we are fine, right? > > > > > > I think you are not understanding how gc works. gc won't just magically > clean > > up > > > your java object for you. An object has to become eligible for gc first, ie > > not > > > reachable from a gc root. You are assuming here that the DisplayManager is a > > gc > > > root (probably correct) and the strong ref from it to the > > > Context/WindowAndroid/Listener/whatever needs to dropped. But gc won't help > > you > > > here at all, because whatever is referenced from DisplayManager won't be > > > eligible for gc in the first place. > > > > I probably didn't make myself clear, what I meant was for us to NOT store a > > reference to the DisplayManager, and instead re-fetch it using > > displayContext.getSystemService(..) when we get the callback indicating that > the > > displayContext is eligible for gc - and then unregister the displayListener. > > That's still the circular I think. displayContext won't become eligible for gc > until all observers are removed (assuming that the DisplayManager is actually > global and so is a gc root itself, or many other possibilities..) > > The general pattern of "let's wait for gc and do some clean up on *java*" side > is never going work well I think.. > > > So > > by NOT storing the DisplayManager we are not holding an implicit strong > > reference to the context and it can be gc'ed. > > > > If it is ok to just hold on to the context until onDetachedFromWindow is > called > > that is probably a lot easier to argue about :) Right, Torne explained that we have had problems when adding listeners for different system services before (and the object dealing with the binder connection then being a gc root). I didn't know that :)
https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... File components/external_video_surface/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... components/external_video_surface/BUILD.gn:33: "//ui/android:ui_java", what does this have to do with this CL? https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:890: dpi_scale_ = dipScale; I remember making this comment before. What's responsible for updating all the places where this is used? https://codereview.chromium.org/2300463002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678: if (windowAndroid == null) { this check seems wrong https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:695: if (windowAndroid != null) windowAndroid.addDIPScaleListener(this); do this above the observer call https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3278: nativeWasResized(mNativeContentViewCore); why? if that's needed, then it should just be handled by nativeSetDIPScale https://codereview.chromium.org/2300463002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale) { why separate these if you just end up calling these two things together? https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... content/public/test/android/BUILD.gn:29: "//ui/android:ui_java", hmm, why is this needed suddenly? https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:20: public interface DIPScaleListener { Just call it DisplayObserver. We might hook up other things like orientation later, and it's the observer pattern https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:40: private class DisplayListener implements DisplayManager.DisplayListener { you could merge this with the Monitor class, and then maybe rename it to Listener https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:64: private DisplayListener mDisplayListener = null; remove = null; https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:67: private float mDIPScale = 0; remove = 0 https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:76: ThreadUtils.assertOnUiThread(); if you want to add thread asserts, then add them to all public methods, not just a few random ones https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:84: public void onDestroy() { this method is totally useless.. remove it https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:100: DeviceDisplayInfo.create(getDisplayContext(mWindowAndroid)); I guess this is ok for now.. but it would be better if we got rid of DeviceDisplayInfo and replace it with just Display first :/ https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:119: if (!mObservers.addObserver(listener)) return; don't bother with this check, seems the client shouldn't be doing this in the first place https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:123: assert mObservers.size() == 1; asserts are useless https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:127: mDIPScale = (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); just call getDIPScale https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:156: Log.v(TAG, "startListening"); these seem like debug logs that should just be removed https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:189: float newDIPScale = (float) displayInfo.getDIPScale(); call getDIPScale https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:191: if (Math.abs(newDIPScale - mDIPScale) > EPS) { I think actually equality is fine. It's not like these things are computed and contain rounding errors.. unless the android api says to do this? (and then get rid of EPS) https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:202: // If displayContext does not exist the activity got destroyed and we do not care about s/and/, / https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:44: : Math.abs(Float.valueOf(forcedScaleAsString)); what if there are exceptions? and document the sentinel values
https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:181: private void updateDIPScale(int displayId) { is it possible to save the display id of the current window, and just skip updates that belongs to other displays?
https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:181: private void updateDIPScale(int displayId) { On 2016/09/14 17:28:21, boliu wrote: > is it possible to save the display id of the current window, and just skip > updates that belongs to other displays? The only way I found was window -> context -> windowManager -> getDefaultDisplay() -> getDisplayId(). I think I can do then when I call getDisplayContext() for the first time.
https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... File components/external_video_surface/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... components/external_video_surface/BUILD.gn:33: "//ui/android:ui_java", On 2016/09/14 04:35:32, boliu wrote: > what does this have to do with this CL? I do not know myself. I have a build problem that this dependency does not propagate. It cannot see DisplayObserver. Which makes me think: maybe ContentViewCore should not inherit DisplayObserver, and instead contain it? https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:890: dpi_scale_ = dipScale; On 2016/09/14 04:35:32, boliu wrote: > I remember making this comment before. What's responsible for updating all the > places where this is used? I probably never understood completely, so let me ask a more detailed question: are you talking about the member variables that store the result which includes DIP scale and thus depend on it indirectly? For instance, OnSelectionEvent() scales the selection rect up with DIP scale and passes it to Java side where it is stored in mSelectionRect. mSelectionRect should be recalculated unless the selection goes away during DIP scale changing process. This job is not done. Better yet, as much as possible should be in DIP pixels. https://codereview.chromium.org/2300463002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678: if (windowAndroid == null) { On 2016/09/14 04:35:32, boliu wrote: > this check seems wrong Thank you, I see now. Removed this check. https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:695: if (windowAndroid != null) windowAndroid.addDIPScaleListener(this); On 2016/09/14 04:35:32, boliu wrote: > do this above the observer call Done. https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3278: nativeWasResized(mNativeContentViewCore); On 2016/09/14 04:35:32, boliu wrote: > why? if that's needed, then it should just be handled by nativeSetDIPScale I thought we need to force the new layout without relying on the onSizeChanged() that comes later. That was one of your earlier considerations. I we do not have to do that, the code actually works! But now I believe we have to... Moved to nativeSetDIPScale. https://codereview.chromium.org/2300463002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale) { On 2016/09/14 04:35:32, boliu wrote: > why separate these if you just end up calling these two things together? Combined. https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... content/public/test/android/BUILD.gn:29: "//ui/android:ui_java", On 2016/09/14 04:35:32, boliu wrote: > hmm, why is this needed suddenly? Same unsolved dependency. Actually, here is a chance to ask a question: //content/public/android:content_java already depends on //base:base_java, but the latter is included here as well. Maybe dependencies do not propagate? https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:20: public interface DIPScaleListener { On 2016/09/14 04:35:33, boliu wrote: > Just call it DisplayObserver. We might hook up other things like orientation > later, and it's the observer pattern Done. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:40: private class DisplayListener implements DisplayManager.DisplayListener { On 2016/09/14 04:35:33, boliu wrote: > you could merge this with the Monitor class, and then maybe rename it to > Listener kept here because of "NewApi". Because of this I did not rename DIPScaleMonitor but can you this if you have a better name. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:64: private DisplayListener mDisplayListener = null; On 2016/09/14 04:35:33, boliu wrote: > remove = null; Done. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:67: private float mDIPScale = 0; On 2016/09/14 04:35:33, boliu wrote: > remove = 0 Done. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:76: ThreadUtils.assertOnUiThread(); On 2016/09/14 04:35:33, boliu wrote: > if you want to add thread asserts, then add them to all public methods, not just > a few random ones I think the only public method I missed was getDIPScale(), added there. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:84: public void onDestroy() { On 2016/09/14 04:35:33, boliu wrote: > this method is totally useless.. remove it Done. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:100: DeviceDisplayInfo.create(getDisplayContext(mWindowAndroid)); On 2016/09/14 04:35:33, boliu wrote: > I guess this is ok for now.. but it would be better if we got rid of > DeviceDisplayInfo and replace it with just Display first :/ Do you mean just renaming DeviceDisplayInfo -> Display or a deeper surgery? Besides, there exists android.view.Display, we might want another name. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:119: if (!mObservers.addObserver(listener)) return; On 2016/09/14 04:35:33, boliu wrote: > don't bother with this check, seems the client shouldn't be doing this in the > first place Why to remove this functionality? Right now you can add and remove the same observer several times. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:123: assert mObservers.size() == 1; On 2016/09/14 04:35:33, boliu wrote: > asserts are useless Removed in this file, I saw lots of them in WebView code however. What is our position on asserts? https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:127: mDIPScale = (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); On 2016/09/14 04:35:33, boliu wrote: > just call getDIPScale I wanted to refresh. getDIPScale() could have been called first, then DIP scale changed, then we added the first observer. In this case |mDIPScale| would have the old value and getDIPScale() would return it. Unless getDIPScale() should retrieve the value every time? https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:156: Log.v(TAG, "startListening"); On 2016/09/14 04:35:33, boliu wrote: > these seem like debug logs that should just be removed Is DEBUG acceptable? I put |if (DEBUG) Log.v();| https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:181: private void updateDIPScale(int displayId) { On 2016/09/14 17:38:19, Tima Vaisburd wrote: > On 2016/09/14 17:28:21, boliu wrote: > > is it possible to save the display id of the current window, and just skip > > updates that belongs to other displays? > > The only way I found was > window -> context -> windowManager -> getDefaultDisplay() -> getDisplayId(). > > I think I can do then when I call getDisplayContext() for the first time. Did in startListening(). https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:189: float newDIPScale = (float) displayInfo.getDIPScale(); On 2016/09/14 04:35:32, boliu wrote: > call getDIPScale Same consideration that I need to get the new value. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:191: if (Math.abs(newDIPScale - mDIPScale) > EPS) { On 2016/09/14 04:35:33, boliu wrote: > I think actually equality is fine. It's not like these things are computed and > contain rounding errors.. unless the android api says to do this? > > (and then get rid of EPS) Yes. Silly me. Added a comment why floating point numbers comparison should be ok here. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:202: // If displayContext does not exist the activity got destroyed and we do not care about On 2016/09/14 04:35:33, boliu wrote: > s/and/, / Done. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:44: : Math.abs(Float.valueOf(forcedScaleAsString)); On 2016/09/14 04:35:33, boliu wrote: > what if there are exceptions? Thanks, I missed that. Added try/catch for valueOf(). > and document the sentinel values Done.
https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... File components/external_video_surface/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... components/external_video_surface/BUILD.gn:33: "//ui/android:ui_java", On 2016/09/15 00:05:03, Tima Vaisburd wrote: > On 2016/09/14 04:35:32, boliu wrote: > > what does this have to do with this CL? > > I do not know myself. I have a build problem that this dependency does not > propagate. It cannot see DisplayObserver. That's not ok. Understand the issue, not patch a random fix in that just makes the problem worse. > > Which makes me think: maybe ContentViewCore should not inherit DisplayObserver, > and instead contain it? Shouldn't matter either way I think https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.cc:890: dpi_scale_ = dipScale; On 2016/09/15 00:05:03, Tima Vaisburd wrote: > On 2016/09/14 04:35:32, boliu wrote: > > I remember making this comment before. What's responsible for updating all the > > places where this is used? > > I probably never understood completely, so let me ask a more detailed question: > are you talking about the member variables that store the result which includes > DIP scale and thus depend on it indirectly? > > For instance, OnSelectionEvent() scales the selection rect up with DIP scale and > passes it to Java side where it is stored in mSelectionRect. mSelectionRect > should be recalculated unless the selection goes away during DIP scale changing > process. > > This job is not done. Yep > > Better yet, as much as possible should be in DIP pixels. Yeah, moving all that conversion to java would be one super big help. I guess you can either leave that as a TODO in this CL, or maybe do that refactor first. https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... content/public/test/android/BUILD.gn:29: "//ui/android:ui_java", On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:32, boliu wrote: > > hmm, why is this needed suddenly? > > Same unsolved dependency. > Actually, here is a chance to ask a question: > //content/public/android:content_java already depends on //base:base_java, but > the latter is included here as well. Maybe dependencies do not propagate? That's right, dependencies do *not* propagate. The rule is you explicitly depend on what you directly use. So if this code uses any base stuff directly, then base should be included here. But if this code only uses base transitively through some dependency, then having a transitive dependency should be fine. I don't see any test changes though, so why is this needed? https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:100: DeviceDisplayInfo.create(getDisplayContext(mWindowAndroid)); On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:33, boliu wrote: > > I guess this is ok for now.. but it would be better if we got rid of > > DeviceDisplayInfo and replace it with just Display first :/ > > Do you mean just renaming DeviceDisplayInfo -> Display or a deeper surgery? Haven't looked into detail yet. But that whole DisplayInfo/ shared instance thing needs to be nuked from orbit. > Besides, there exists android.view.Display, we might want another name. I think namespace will be good enough to distinguish the two. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:119: if (!mObservers.addObserver(listener)) return; On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:33, boliu wrote: > > don't bother with this check, seems the client shouldn't be doing this in the > > first place > > Why to remove this functionality? Right now you can add and remove the same > observer several times. You can remove this check and everything will still work the same way. Seems odd that ObserverList decided to not treat repeated add as an error. I wouldn't have just made it an error. Actually maybe you should throw in this case. repeated observer makes no sense at all https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:123: assert mObservers.size() == 1; On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:33, boliu wrote: > > asserts are useless > > Removed in this file, I saw lots of them in WebView code however. What is our > position on asserts? asserts don't do anything after some android version I can't remember.. K or L there's a lot of old code that had asserts. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:127: mDIPScale = (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:33, boliu wrote: > > just call getDIPScale > > I wanted to refresh. getDIPScale() could have been called first, then DIP scale > changed, then we added the first observer. In this case |mDIPScale| would have > the old value and getDIPScale() would return it. Unless getDIPScale() should > retrieve the value every time? Sounds like you need an updateDIPScale in addition to get then. Basically don't duplicate code https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:156: Log.v(TAG, "startListening"); On 2016/09/15 00:05:04, Tima Vaisburd wrote: > On 2016/09/14 04:35:33, boliu wrote: > > these seem like debug logs that should just be removed > > Is DEBUG acceptable? I put |if (DEBUG) Log.v();| how often is someone going to want to turn those on? seems like never.. https://codereview.chromium.org/2300463002/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:895: view->WasResized(); don't duplicate code, factor out the current CVCI::WasResized method into a helper method and call the helper instead https://codereview.chromium.org/2300463002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2300463002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:656: public void addDisplayObserver(DisplayObserver observer) { probably should add a note that this observer is on Window rather than "Display" because we don't have an abstraction for display in java yet
https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... content/public/test/android/BUILD.gn:29: "//ui/android:ui_java", On 2016/09/15 05:27:41, boliu wrote: > On 2016/09/15 00:05:04, Tima Vaisburd wrote: > > On 2016/09/14 04:35:32, boliu wrote: > > > hmm, why is this needed suddenly? > > > > Same unsolved dependency. > > Actually, here is a chance to ask a question: > > //content/public/android:content_java already depends on //base:base_java, but > > the latter is included here as well. Maybe dependencies do not propagate? > > That's right, dependencies do *not* propagate. The rule is you explicitly depend > on what you directly use. So if this code uses any base stuff directly, then > base should be included here. But if this code only uses base transitively > through some dependency, then having a transitive dependency should be fine. > > I don't see any test changes though, so why is this needed? My *guess* is that now |ContentViewCore implement ui::DisplayObserver|, and so whenever system sees CVC it also thinks ui::DisplayObserver is used directly. Before this change CVC subclassed content stuff mostly (i.e. not ui). If this is true, I see two ways: add dependency in many BUILD.gn files, or make ui::DisplayObserver an implementation detail of CVC.
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_chromium_compile_dbg_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: 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...
https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... File components/external_video_surface/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/components/external_vi... components/external_video_surface/BUILD.gn:33: "//ui/android:ui_java", On 2016/09/15 05:27:41, boliu wrote: > On 2016/09/15 00:05:03, Tima Vaisburd wrote: > > On 2016/09/14 04:35:32, boliu wrote: > > > what does this have to do with this CL? > > > > I do not know myself. I have a build problem that this dependency does not > > propagate. It cannot see DisplayObserver. > > That's not ok. Understand the issue, not patch a random fix in that just makes > the problem worse. > > > > > Which makes me think: maybe ContentViewCore should not inherit > DisplayObserver, > > and instead contain it? > > Shouldn't matter either way I think Acknowledged. https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2300463002/diff/120001/content/public/test/an... content/public/test/android/BUILD.gn:29: "//ui/android:ui_java", On 2016/09/15 18:11:42, Tima Vaisburd wrote: > On 2016/09/15 05:27:41, boliu wrote: > > On 2016/09/15 00:05:04, Tima Vaisburd wrote: > > > On 2016/09/14 04:35:32, boliu wrote: > > > > hmm, why is this needed suddenly? > > > > > > Same unsolved dependency. > > > Actually, here is a chance to ask a question: > > > //content/public/android:content_java already depends on //base:base_java, > but > > > the latter is included here as well. Maybe dependencies do not propagate? > > > > That's right, dependencies do *not* propagate. The rule is you explicitly > depend > > on what you directly use. So if this code uses any base stuff directly, then > > base should be included here. But if this code only uses base transitively > > through some dependency, then having a transitive dependency should be fine. > > > > I don't see any test changes though, so why is this needed? > > My *guess* is that now |ContentViewCore implement ui::DisplayObserver|, and so > whenever system sees CVC it also thinks ui::DisplayObserver is used directly. > Before this change CVC subclassed content stuff mostly (i.e. not ui). > If this is true, I see two ways: add dependency in many BUILD.gn files, or make > ui::DisplayObserver an implementation detail of CVC. I moved DisplayObserver from public interface the CVC inherits from into CVC member (i.e. containment) and it fixed the compilation issue. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java (right): https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:119: if (!mObservers.addObserver(listener)) return; On 2016/09/15 05:27:41, boliu wrote: > On 2016/09/15 00:05:04, Tima Vaisburd wrote: > > On 2016/09/14 04:35:33, boliu wrote: > > > don't bother with this check, seems the client shouldn't be doing this in > the > > > first place > > > > Why to remove this functionality? Right now you can add and remove the same > > observer several times. > > You can remove this check and everything will still work the same way. > > Seems odd that ObserverList decided to not treat repeated add as an error. I > wouldn't have just made it an error. Actually maybe you should throw in this > case. repeated observer makes no sense at all Thew IllegalArgumentException. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:127: mDIPScale = (float) DeviceDisplayInfo.create(displayContext).getDIPScale(); On 2016/09/15 05:27:41, boliu wrote: > On 2016/09/15 00:05:04, Tima Vaisburd wrote: > > On 2016/09/14 04:35:33, boliu wrote: > > > just call getDIPScale > > > > I wanted to refresh. getDIPScale() could have been called first, then DIP > scale > > changed, then we added the first observer. In this case |mDIPScale| would have > > the old value and getDIPScale() would return it. Unless getDIPScale() should > > retrieve the value every time? > > Sounds like you need an updateDIPScale in addition to get then. Basically don't > duplicate code Added private getDIPScale(Context) which removed the code duplication, but at the expense of retrieving the Context from mWindowAndroid more than necessary. https://codereview.chromium.org/2300463002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/DIPScaleListener.java:156: Log.v(TAG, "startListening"); On 2016/09/15 05:27:41, boliu wrote: > On 2016/09/15 00:05:04, Tima Vaisburd wrote: > > On 2016/09/14 04:35:33, boliu wrote: > > > these seem like debug logs that should just be removed > > > > Is DEBUG acceptable? I put |if (DEBUG) Log.v();| > > how often is someone going to want to turn those on? seems like never.. Just removed the logs. https://codereview.chromium.org/2300463002/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:895: view->WasResized(); On 2016/09/15 05:27:41, boliu wrote: > don't duplicate code, factor out the current CVCI::WasResized method into a > helper method and call the helper instead Done. https://codereview.chromium.org/2300463002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2300463002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:656: public void addDisplayObserver(DisplayObserver observer) { On 2016/09/15 05:27:41, boliu wrote: > probably should add a note that this observer is on Window rather than "Display" > because we don't have an abstraction for display in java yet Said something in the // comment above..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 ========== Add listener and observers for DIP scale change. The listener and observers are implemented on Java side. There are currently two observers: ContentViewCore and AwContents. They are added in onAttachedToWindow() and removed in onDetachedFromWindow() methods. These classes propagate DIP scale to the native side. The listenet is a part of WindowAndroid. Listeners register themselves to the DisplayManager and get new DIP scale upon receiving onDisplayChanged(). BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore sets the DIP scale in tow places: 1. On Java side in RenderCoordinates 2. On native side in content_view_core_impl.cc Upon setting the DIP scale in the native side ContentViewCore now forces layout by sendinf Resize message to the render widget. BUG=620929 ==========
Description was changed from ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore sets the DIP scale in tow places: 1. On Java side in RenderCoordinates 2. On native side in content_view_core_impl.cc Upon setting the DIP scale in the native side ContentViewCore now forces layout by sendinf Resize message to the render widget. BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore sets the DIP scale in two places: 1. On Java side in RenderCoordinates 2. On native side in content_view_core_impl.cc Upon setting the DIP scale in the native side ContentViewCore now forces layout by sendinf Resize message to the render widget. BUG=620929 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
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 rewrote this CL for DisplayAndroid, please take another look. Now I'm worried that I call ForceLayout() unnecessary often. https://codereview.chromium.org/2300463002/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:64: void updateDeviceScaleFactor(float dipScale, WindowAndroid windowAndroid) { Not sure about two parameters. In one case we get dipScale from the listener.
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...)
Description was changed from ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore sets the DIP scale in two places: 1. On Java side in RenderCoordinates 2. On native side in content_view_core_impl.cc Upon setting the DIP scale in the native side ContentViewCore now forces layout by sendinf Resize message to the render widget. BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During iniitialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ==========
Description was changed from ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During iniitialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During initialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ==========
Slight refactoring to avoid ForceLayout() during initialization. Updated the bug description.
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...)
https://codereview.chromium.org/2300463002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2300463002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:104: DisplayAndroidObserver { use an inner class https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.cc:218: dpi_scale_(1), why is this change needed? https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.cc:806: void ContentViewCoreImpl::ForceLayout() { just call this "WasResized"? https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.h:391: void ForceLayout(); private? https://codereview.chromium.org/2300463002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3276: getWindowAndroid() == null ? null : getWindowAndroid().getContext().get(); should this be an assert? https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3280: nativeSetDIPScale(mNativeContentViewCore, dipScale, true); // force layout format it like (..., true /* force layout */); https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3283: private void updateDeviceScaleFactorFromWindow(WindowAndroid window) { there is only one caller, just inline it? https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3289: nativeSetDIPScale(mNativeContentViewCore, dipScale, false); // do not force layout ditto about formatting https://codereview.chromium.org/2300463002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { maybe should pass in WeakReference<Context> to make it clear it might be null..?
The CQ bit was checked by timav@chromium.org to run a CQ dry run
https://codereview.chromium.org/2300463002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2300463002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:104: DisplayAndroidObserver { On 2016/10/18 00:26:25, boliu wrote: > use an inner class Done. https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.cc:218: dpi_scale_(1), On 2016/10/18 00:26:25, boliu wrote: > why is this change needed? It is actually not needed, but I wanted to make it consistent with the Java side and make it clear that DIP scale will be explicitly set. I reverted this change. I also changed the initial DIP scale in RenderCoordinates.java from 0 to 1. https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.cc:806: void ContentViewCoreImpl::ForceLayout() { On 2016/10/18 00:26:25, boliu wrote: > just call this "WasResized"? WasResized() already existed, now it sets the physical size and then calls this ForceLayout(). See below. https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2300463002/diff/280001/content/browser/androi... content/browser/android/content_view_core_impl.h:391: void ForceLayout(); On 2016/10/18 00:26:25, boliu wrote: > private? Thank you. Done. https://codereview.chromium.org/2300463002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3276: getWindowAndroid() == null ? null : getWindowAndroid().getContext().get(); On 2016/10/18 00:26:25, boliu wrote: > should this be an assert? This method might be called directly by DisplayAndroid and thus not protected by condition windowAndroid != null. I added this condition (early return) instead of assert. https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3280: nativeSetDIPScale(mNativeContentViewCore, dipScale, true); // force layout On 2016/10/18 00:26:25, boliu wrote: > format it like (..., true /* force layout */); Done. https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3283: private void updateDeviceScaleFactorFromWindow(WindowAndroid window) { On 2016/10/18 00:26:25, boliu wrote: > there is only one caller, just inline it? Done. https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3289: nativeSetDIPScale(mNativeContentViewCore, dipScale, false); // do not force layout On 2016/10/18 00:26:25, boliu wrote: > ditto about formatting Done. https://codereview.chromium.org/2300463002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { On 2016/10/18 00:26:25, boliu wrote: > maybe should pass in WeakReference<Context> to make it clear it might be null..? Done.
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/2300463002/diff/300001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:218: dpi_scale_(ui::GetScaleFactorForNativeView(&view_)), with the java side suggestion, you can just ge the dip in the constructor https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:806: void ContentViewCoreImpl::ForceLayout() { naming is hard, but definitely avoid overloading terms like "Layout". How about SendScreenRects then? https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:886: ForceLayout(); only if changed? https://codereview.chromium.org/2300463002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:666: nativeSetDIPScale(mNativeContentViewCore, dipScale, false /* do not force layout */); dipScale should just be part of nativeInit. Then you can avoid this awkward forceLayout bool https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3278: if (getWindowAndroid() == null) return; avoid calling getWindowAndroid twice. it involves jni so not very cheap https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3281: if (mNativeContentViewCore == 0) return; move this check up as well? https://codereview.chromium.org/2300463002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:75: if (context.get() != null && context.get().getTheme().resolveAttribute( you need to call get() first and save it in a local var first, because in theory, it's possible the context gets collected between this check and the next clause when it's used
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/2300463002/diff/300001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:218: dpi_scale_(ui::GetScaleFactorForNativeView(&view_)), On 2016/10/19 04:19:18, boliu wrote: > with the java side suggestion, you can just ge the dip in the constructor Done. https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:806: void ContentViewCoreImpl::ForceLayout() { On 2016/10/19 04:19:18, boliu wrote: > naming is hard, but definitely avoid overloading terms like "Layout". How about > SendScreenRects then? The modestly looking |view->WasResized();| eventually calls RenderWidgetHostImpl::WasResized() https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... which obtains resize parameters, checks whether they are changed and sends ViewMsg_Resize to the renderer process. I renamed this method into SendScreenRectsAndresizeWidget() for now, maybe you can come up with a better name. My intention was to initiate the layout process though. https://codereview.chromium.org/2300463002/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:886: ForceLayout(); On 2016/10/19 04:19:18, boliu wrote: > only if changed? Yes, I added that. Now I realized that during reparenting we might send resize twice... https://codereview.chromium.org/2300463002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:666: nativeSetDIPScale(mNativeContentViewCore, dipScale, false /* do not force layout */); On 2016/10/19 04:19:19, boliu wrote: > dipScale should just be part of nativeInit. Then you can avoid this awkward > forceLayout bool Done. https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3278: if (getWindowAndroid() == null) return; On 2016/10/19 04:19:18, boliu wrote: > avoid calling getWindowAndroid twice. it involves jni so not very cheap Done. https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3281: if (mNativeContentViewCore == 0) return; On 2016/10/19 04:19:18, boliu wrote: > move this check up as well? Done. https://codereview.chromium.org/2300463002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:75: if (context.get() != null && context.get().getTheme().resolveAttribute( On 2016/10/19 04:19:19, boliu wrote: > you need to call get() first and save it in a local var first, because in > theory, it's possible the context gets collected between this check and the next > clause when it's used Thank you! Done.
lgtm % nit https://codereview.chromium.org/2300463002/diff/320001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/320001/content/browser/androi... content/browser/android/content_view_core_impl.cc:884: if (dpi_scale_ != dpi_scale) { nit: general pattern is write it as an early out if (old == new) return; so the rest of the function doesn't need to be indented
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...
timav@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@: Ted, please take a look. https://codereview.chromium.org/2300463002/diff/320001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/320001/content/browser/androi... content/browser/android/content_view_core_impl.cc:884: if (dpi_scale_ != dpi_scale) { On 2016/10/19 20:49:07, boliu wrote: > nit: general pattern is write it as an early out > if (old == new) > return; > > so the rest of the function doesn't need to be indented Done. https://codereview.chromium.org/2300463002/diff/340001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2300463002/diff/340001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:629: // On Android forcing device scale factor does not work for tests, therefore Actually I'm not sure about this, maybe we need to properly force DisplayAndroid update for tests. The WindowAndroid is created early, before the test object.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... content/browser/android/content_view_core_impl.cc:212: float dpi_scale, since we're here, is this supposed to be dpi or dip? https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:657: final float dipScale = windowAndroid.getDisplay().getDIPScale(); in java, the method name should be getDipScale() https://source.android.com/source/code-style.html#treat-acronyms-as-words https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { any reason changes to this file are needed? I see that onDIPScaleChanged is called on the observer, but would windowAndroid.getDisplay().getDIPScale(); not be correct at that point?
https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... content/browser/android/content_view_core_impl.cc:212: float dpi_scale, On 2016/10/31 20:02:40, Ted C wrote: > since we're here, is this supposed to be dpi or dip? I wanted it to be dip (for Device Independent Pixels) but hesitated to change dpi to dip everywhere. Maybe I should actually do it in this CL? https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/10/31 20:02:40, Ted C wrote: > any reason changes to this file are needed? > > I see that onDIPScaleChanged is called on the observer, but would > windowAndroid.getDisplay().getDIPScale(); not be correct at that point? Strictly speaking, this change it not necessary. I think it adds more clarity in the caller though. Now ContentViewCore can receive the DIP scale from observer. I wanted to make the explicit use of it, so in ContentViewCore we have void onDIPScaleChanged(float dipScale) { mRenderCoordinates.setDeviceScaleFactor(dipScale, getWindowAndroid().getContext()); nativeSetDIPScale(mNativeVontentViewCore, dipScale); } I think passing just getWindowAndroid() would obscure the fact that we are using the parameter on the Java side as well.
https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... content/browser/android/content_view_core_impl.cc:212: float dpi_scale, On 2016/10/31 20:37:59, Tima Vaisburd wrote: > On 2016/10/31 20:02:40, Ted C wrote: > > since we're here, is this supposed to be dpi or dip? > > I wanted it to be dip (for Device Independent Pixels) but hesitated to change > dpi to dip everywhere. Maybe I should actually do it in this CL? Hmm...it does seem to be used all over this class and would bloat this CL. Let's do it in a followup. https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/10/31 20:37:59, Tima Vaisburd wrote: > On 2016/10/31 20:02:40, Ted C wrote: > > any reason changes to this file are needed? > > > > I see that onDIPScaleChanged is called on the observer, but would > > windowAndroid.getDisplay().getDIPScale(); not be correct at that point? > > Strictly speaking, this change it not necessary. I think it adds more clarity in > the caller though. > Now ContentViewCore can receive the DIP scale from observer. I wanted to make > the explicit use of it, > so in ContentViewCore we have > > void onDIPScaleChanged(float dipScale) { > mRenderCoordinates.setDeviceScaleFactor(dipScale, > getWindowAndroid().getContext()); > nativeSetDIPScale(mNativeVontentViewCore, dipScale); > } > > I think passing just getWindowAndroid() would obscure the fact that we are using > the parameter on the Java side as well. > To continue down this rabbit hole, why does nativeSetDIPScale need to be called at all? Why does CVC even keep that value? Why doesn't it always pull it from WindowAndroid. I'd like to get to a place where the value is stored in a single place instead of having to do keep all of these in sync. I'm thinking that onDipScaleChanged would just call WasResized and force that through the normal resize path. I guess I'm wondering why we need to do the nativeSetDIPScale at all then. Why does CVC keep that value instead of pulling it from WindowAndroid? I'd rather have WindowAndroid be the only source of truth instead of trying to keep these in sync. onDipScaleChanged in that case would just update
https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/11/01 17:07:16, Ted C wrote: > On 2016/10/31 20:37:59, Tima Vaisburd wrote: > > On 2016/10/31 20:02:40, Ted C wrote: > > > any reason changes to this file are needed? > > > > > > I see that onDIPScaleChanged is called on the observer, but would > > > windowAndroid.getDisplay().getDIPScale(); not be correct at that point? > > > > Strictly speaking, this change it not necessary. I think it adds more clarity > in > > the caller though. > > Now ContentViewCore can receive the DIP scale from observer. I wanted to make > > the explicit use of it, > > so in ContentViewCore we have > > > > void onDIPScaleChanged(float dipScale) { > > mRenderCoordinates.setDeviceScaleFactor(dipScale, > > getWindowAndroid().getContext()); > > nativeSetDIPScale(mNativeVontentViewCore, dipScale); > > } > > > > I think passing just getWindowAndroid() would obscure the fact that we are > using > > the parameter on the Java side as well. > > > > To continue down this rabbit hole, why does nativeSetDIPScale need > to be called at all? Why does CVC even keep that value? Why doesn't > it always pull it from WindowAndroid. > > I'd like to get to a place where the value is stored in a single place > instead of having to do keep all of these in sync. > > I'm thinking that onDipScaleChanged would just call WasResized and > force that through the normal resize path. > I guess I'm wondering why we need to do the nativeSetDIPScale at all > then. Why does CVC keep that value instead of pulling it from > WindowAndroid? > > I'd rather have WindowAndroid be the only source of truth instead of > trying to keep these in sync. > > onDipScaleChanged in that case would just update Reminder WindowAndroid isn't always available, which may or may not cause problems for this single source of truth plan.
On 2016/11/01 17:13:03, boliu wrote: > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java > (right): > > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: > void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) > { > On 2016/11/01 17:07:16, Ted C wrote: > > On 2016/10/31 20:37:59, Tima Vaisburd wrote: > > > On 2016/10/31 20:02:40, Ted C wrote: > > > > any reason changes to this file are needed? > > > > > > > > I see that onDIPScaleChanged is called on the observer, but would > > > > windowAndroid.getDisplay().getDIPScale(); not be correct at that point? > > > > > > Strictly speaking, this change it not necessary. I think it adds more > clarity > > in > > > the caller though. > > > Now ContentViewCore can receive the DIP scale from observer. I wanted to > make > > > the explicit use of it, > > > so in ContentViewCore we have > > > > > > void onDIPScaleChanged(float dipScale) { > > > mRenderCoordinates.setDeviceScaleFactor(dipScale, > > > getWindowAndroid().getContext()); > > > nativeSetDIPScale(mNativeVontentViewCore, dipScale); > > > } > > > > > > I think passing just getWindowAndroid() would obscure the fact that we are > > using > > > the parameter on the Java side as well. > > > > > > > To continue down this rabbit hole, why does nativeSetDIPScale need > > to be called at all? Why does CVC even keep that value? Why doesn't > > it always pull it from WindowAndroid. > > > > I'd like to get to a place where the value is stored in a single place > > instead of having to do keep all of these in sync. > > > > I'm thinking that onDipScaleChanged would just call WasResized and > > force that through the normal resize path. > > I guess I'm wondering why we need to do the nativeSetDIPScale at all > > then. Why does CVC keep that value instead of pulling it from > > WindowAndroid? > > > > I'd rather have WindowAndroid be the only source of truth instead of > > trying to keep these in sync. > > > > onDipScaleChanged in that case would just update > > Reminder WindowAndroid isn't always available, which may or may not cause > problems for this single source of truth plan. If we don't have a WindowAndroid, then we don't know what display we are drawing into and then we can't trust the density anyway. It's true that we'd need to figure out a sane non-crashy thing to do here, but that could just be pull a display out a hat and pick it's density since that is the best we could conceivably do. Or we could rely on the fact that activities can't move across displays yet, so we could just keep them. But if anything, I would rather everyone access WindowAndroid. If they absolutely need a density when they are gone, then it should explicitly cache it then and discard it when the window is reattached.
On 2016/11/01 17:40:22, Ted C wrote: > On 2016/11/01 17:13:03, boliu wrote: > > > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > > File > > > content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java > > (right): > > > > > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: > > void setDeviceScaleFactor(float dipScale, WeakReference<Context> > displayContext) > > { > > On 2016/11/01 17:07:16, Ted C wrote: > > > On 2016/10/31 20:37:59, Tima Vaisburd wrote: > > > > On 2016/10/31 20:02:40, Ted C wrote: > > > > > any reason changes to this file are needed? > > > > > > > > > > I see that onDIPScaleChanged is called on the observer, but would > > > > > windowAndroid.getDisplay().getDIPScale(); not be correct at that point? > > > > > > > > Strictly speaking, this change it not necessary. I think it adds more > > clarity > > > in > > > > the caller though. > > > > Now ContentViewCore can receive the DIP scale from observer. I wanted to > > make > > > > the explicit use of it, > > > > so in ContentViewCore we have > > > > > > > > void onDIPScaleChanged(float dipScale) { > > > > mRenderCoordinates.setDeviceScaleFactor(dipScale, > > > > getWindowAndroid().getContext()); > > > > nativeSetDIPScale(mNativeVontentViewCore, dipScale); > > > > } > > > > > > > > I think passing just getWindowAndroid() would obscure the fact that we are > > > using > > > > the parameter on the Java side as well. > > > > > > > > > > To continue down this rabbit hole, why does nativeSetDIPScale need > > > to be called at all? Why does CVC even keep that value? Why doesn't > > > it always pull it from WindowAndroid. > > > > > > I'd like to get to a place where the value is stored in a single place > > > instead of having to do keep all of these in sync. > > > > > > I'm thinking that onDipScaleChanged would just call WasResized and > > > force that through the normal resize path. > > > I guess I'm wondering why we need to do the nativeSetDIPScale at all > > > then. Why does CVC keep that value instead of pulling it from > > > WindowAndroid? > > > > > > I'd rather have WindowAndroid be the only source of truth instead of > > > trying to keep these in sync. > > > > > > onDipScaleChanged in that case would just update > > > > Reminder WindowAndroid isn't always available, which may or may not cause > > problems for this single source of truth plan. > > If we don't have a WindowAndroid, then we don't know what display we are > drawing into and then we can't trust the density anyway. It's true that > we'd need to figure out a sane non-crashy thing to do here, but that could > just be pull a display out a hat and pick it's density since that is the > best we could conceivably do. Or we could rely on the fact that activities > can't move across displays yet, so we could just keep them. But ContentViewCore can move between activities, which in theory can be on different displays. > But if anything, > I would rather everyone access WindowAndroid. If they absolutely need a density > when they are gone, then it should explicitly cache it then and discard it > when the window is reattached. out of scope for this CL?
https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2300463002/diff/360001/content/browser/androi... content/browser/android/content_view_core_impl.cc:212: float dpi_scale, On 2016/11/01 17:07:16, Ted C wrote: > On 2016/10/31 20:37:59, Tima Vaisburd wrote: > > On 2016/10/31 20:02:40, Ted C wrote: > > > since we're here, is this supposed to be dpi or dip? > > > > I wanted it to be dip (for Device Independent Pixels) but hesitated to change > > dpi to dip everywhere. Maybe I should actually do it in this CL? > > Hmm...it does seem to be used all over this class and would bloat this CL. > Let's do it in a followup. Let's have a followup! https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/11/01 17:07:16, Ted C wrote: > I guess I'm wondering why we need to do the nativeSetDIPScale at all > then. Why does CVC keep that value instead of pulling it from > WindowAndroid? My considerations were pretty simple here. Right now dpi_scale() is used in many places to convert from pixels to dip on the way from Android API to Chrome. This includes UI gestures that I think have to be fast. I wanted to eliminate dip scale duplication between CVC native and Java when I did the first series of refactoring, but stopped at the current state, thinking that the inlined dpi_scale() was the necessary cache. Also, I did not see the good way to pull the info from WindowAndroid before https://codereview.chromium.org/2416403002/. Even with this CL the connection between the windowAndroid to its Display is by id, not with direct link. Would you consider a possibility to address this question in the subsequent CLs? > > I'd rather have WindowAndroid be the only source of truth instead of > trying to keep these in sync. > > onDipScaleChanged in that case would just update Please continue your thought.
lgtm w/ method renamed https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:657: final float dipScale = windowAndroid.getDisplay().getDIPScale(); On 2016/10/31 20:02:40, Ted C wrote: > in java, the method name should be getDipScale() > > https://source.android.com/source/code-style.html#treat-acronyms-as-words Still need to do this renaming though https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:65: void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/11/01 18:23:26, Tima Vaisburd wrote: > On 2016/11/01 17:07:16, Ted C wrote: > > I guess I'm wondering why we need to do the nativeSetDIPScale at all > > then. Why does CVC keep that value instead of pulling it from > > WindowAndroid? > > My considerations were pretty simple here. Right now dpi_scale() is used in many > places to convert from > pixels to dip on the way from Android API to Chrome. This includes UI gestures > that I think have to be fast. I wanted to eliminate dip scale duplication > between CVC native and Java when I did the first series of refactoring, but > stopped at the current state, thinking that the inlined dpi_scale() was the > necessary cache. > > Also, I did not see the good way to pull the info from WindowAndroid before > https://codereview.chromium.org/2416403002/. Even with this CL the connection > between the windowAndroid to its Display is by id, not with direct link. > > Would you consider a possibility to address this question in the subsequent CLs? I think it is fine to do in follow ups, but I would like to know what the end goal is before we go in too many circles. But the discussion doesn't need to be on this change. Getting back to my original comment about why not passing WindowAndroid, I think maybe my main issue is that we still need to get the context from the window android to call this method. But I think that is actually because this method is doing things that I don't think belong here. I find it unfortunate that we need to do the wheel scroll stuff here. I think if we were to store the value as DP then we could calculate it w/o the context. Or we could pass in the DisplayAndroid (and thus having access to the display metrics from the display itself instead of the context). But I guess this is also outside the scope of this change. > > > > > I'd rather have WindowAndroid be the only source of truth instead of > > trying to keep these in sync. > > > > onDipScaleChanged in that case would just update > > Please continue your thought. I think that's just a copy paste error on my part.
https://codereview.chromium.org/2300463002/diff/360001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2300463002/diff/360001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:657: final float dipScale = windowAndroid.getDisplay().getDIPScale(); On 2016/11/01 19:04:59, Ted C wrote: > On 2016/10/31 20:02:40, Ted C wrote: > > in java, the method name should be getDipScale() > > > > https://source.android.com/source/code-style.html#treat-acronyms-as-words > > Still need to do this renaming though I started doing this, but in view of https://codereview.chromium.org/2394773006 it seems it would be better to have a separate renaming CL. Please reply if you think this is a bad idea.
On 2016/11/01 22:45:34, Tima Vaisburd wrote: > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/2300463002/diff/360001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:657: > final float dipScale = windowAndroid.getDisplay().getDIPScale(); > On 2016/11/01 19:04:59, Ted C wrote: > > On 2016/10/31 20:02:40, Ted C wrote: > > > in java, the method name should be getDipScale() > > > > > > https://source.android.com/source/code-style.html#treat-acronyms-as-words > > > > Still need to do this renaming though > > I started doing this, but in view of https://codereview.chromium.org/2394773006 > it seems it would be better to have a separate renaming CL. Please reply if you > think this is a bad idea. sgtm
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.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2300463002/#ps400001 (title: "Rebased to master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During initialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During initialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During initialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 ========== to ========== Add observers for DIP scale change. Extended Java DisplayAndroidObserver interface to receive the DIP scale change notifications. Added two such observers: ContentViewCore and AwContents. Upon receiving the notification ContentViewCore now sets the DIP scale both on Java and native side. It happens 1. During initialization, 2. When ContentViewCore is attached to a window, either for the first time or due to reparenting, 3. Due to notification from observer. In cases (2) and (3) we force layout. BUG=620929 Committed: https://crrev.com/9483b39990200554ba76602455869962005ace0e Cr-Commit-Position: refs/heads/master@{#430536} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/9483b39990200554ba76602455869962005ace0e Cr-Commit-Position: refs/heads/master@{#430536} |
