Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268103002:
Use WindowAndroid to get display metrics in ContentViewCore (Closed)
|
Created:
4 years, 4 months ago by Tima Vaisburd Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse WindowAndroid to get display metrics in ContentViewCore
Since Chrome can do window reparenting, DisplayMetrics should
be extracted from the Context that is associated with the window.
In this CL we pass the context to RenderCoordinates that obtains
the display density (device scale factor) and wheel scroll factor
from it.
BUG=620929
Committed: https://crrev.com/8521fda07ea8d04953d856873a6e08afb96701d2
Cr-Commit-Position: refs/heads/master@{#415158}
Patch Set 1 #Patch Set 2 : Fixed context existence condition #
Total comments: 11
Patch Set 3 : Rewrote getDisplayMetrics(), set dip scale in updateWindowAndroid() #
Total comments: 6
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Rebase only #Patch Set 6 : Moved DisplayMetrics logic and wheel scroll factor to RenderCoordinates #
Dependent Patchsets: Messages
Total messages: 42 (25 generated)
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...
timav@chromium.org changed reviewers: + boliu@chromium.org, gsennton@chromium.org, sievers@chromium.org
PTAL. https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:670: float deviceScaleFactor = getDisplayMetrics().density; Since we postponed this call till after nativeInit(), we might now get the device scale factor from the native side. WDYT? Do you know why SharedDeviceDisplayInfo https://cs.chromium.org/chromium/src/ui/gfx/android/shared_device_display_inf... does not honor Display::GetForcedDeviceScaleFactor() https://cs.chromium.org/chromium/src/ui/display/display.cc?rcl=1471951080&l=57 ?
https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:632: (getWindowAndroid() != null && getWindowAndroid().getContext().get() != null) avoid double jni calls here https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:633: ? getWindowAndroid().getContext().get() this can be null, just make sure it doesn't crash https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:670: float deviceScaleFactor = getDisplayMetrics().density; On 2016/08/23 17:24:21, Tima Vaisburd wrote: > Since we postponed this call till after nativeInit(), we might now get the > device scale factor from the native side. WDYT? I don't have a good picture in my head if that's a worthwhile change.. > Do you know why SharedDeviceDisplayInfo > https://cs.chromium.org/chromium/src/ui/gfx/android/shared_device_display_inf... > does not honor Display::GetForcedDeviceScaleFactor() > https://cs.chromium.org/chromium/src/ui/display/display.cc?rcl=1471951080&l=57 ? it probably should https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: public void updateWindowAndroid(WindowAndroid windowAndroid) { need to redo that work here, when WindowAndroid changes
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...
https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:632: (getWindowAndroid() != null && getWindowAndroid().getContext().get() != null) On 2016/08/23 18:16:14, boliu wrote: > avoid double jni calls here Done. https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:633: ? getWindowAndroid().getContext().get() On 2016/08/23 18:16:14, boliu wrote: > this can be null, just make sure it doesn't crash I rewrote this method to avoid calling getWindowAndroid().getContext().get() twice. https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:670: float deviceScaleFactor = getDisplayMetrics().density; On 2016/08/23 18:16:14, boliu wrote: > On 2016/08/23 17:24:21, Tima Vaisburd wrote: > > Since we postponed this call till after nativeInit(), we might now get the > > device scale factor from the native side. WDYT? > > I don't have a good picture in my head if that's a worthwhile change.. The idea was to have just one source of truth for dip scale. Not doing here then. https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: public void updateWindowAndroid(WindowAndroid windowAndroid) { On 2016/08/23 18:16:14, boliu wrote: > need to redo that work here, when WindowAndroid changes I put a line that sets device scale factor, but maybe we need a broader update. Maybe we shall postpone this until reparenting to another screen works? Right now it would be hard to test.
https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: public void updateWindowAndroid(WindowAndroid windowAndroid) { On 2016/08/23 21:25:11, Tima Vaisburd wrote: > On 2016/08/23 18:16:14, boliu wrote: > > need to redo that work here, when WindowAndroid changes > > I put a line that sets device scale factor, but maybe we need a broader update. What "broader update"? > Maybe we shall postpone this until reparenting to another screen works? Right > now it would be hard to test. https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:707: if (windowAndroid != null) mRenderCoordinates.setDeviceScaleFactor(getDisplayDensity()); do you need the null check?
https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: public void updateWindowAndroid(WindowAndroid windowAndroid) { On 2016/08/23 21:33:00, boliu wrote: > What "broader update"? Maybe I'm missing something. As far as I got you've suggested an observer pattern (I thought similar to ScreenOrienationObserver , https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...) that would notify objects that cashed dip-dependent values on dip scale change. This part is not ready yet and I do not know yet what are these components. https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:707: if (windowAndroid != null) mRenderCoordinates.setDeviceScaleFactor(getDisplayDensity()); On 2016/08/23 21:33:00, boliu wrote: > do you need the null check? The search for updateWindowAndroid() gave me two calls: at detaching https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... and reattaching https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... The first call passes null for windowAndroid. Without the check we'd set device scale factor from the mContext (probably wrong one) but the window is in detached state. I wanted to avoid that.
lg2m % comments https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: // TODO(timav, gsennton): Update all required elements if device scale factor remove this todo, wording makes no sense.. https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:707: if (windowAndroid != null) mRenderCoordinates.setDeviceScaleFactor(getDisplayDensity()); On 2016/08/23 21:57:37, Tima Vaisburd wrote: > On 2016/08/23 21:33:00, boliu wrote: > > do you need the null check? > > The search for updateWindowAndroid() gave me two calls: > at detaching > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > and reattaching > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I think it's pretty clear from this method that windowAndroid param can be null. That's not the question. > > The first call passes null for windowAndroid. Without the check we'd set device > scale factor from the mContext (probably wrong one) but the window is in > detached state. so either way is ok, but no null check is simpler > I wanted to avoid that.
https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: // TODO(timav, gsennton): Update all required elements if device scale factor On 2016/08/23 22:12:17, boliu wrote: > remove this todo, wording makes no sense.. Done. https://codereview.chromium.org/2268103002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:707: if (windowAndroid != null) mRenderCoordinates.setDeviceScaleFactor(getDisplayDensity()); On 2016/08/23 22:12:17, boliu wrote: > On 2016/08/23 21:57:37, Tima Vaisburd wrote: > > On 2016/08/23 21:33:00, boliu wrote: > > > do you need the null check? > > > > The search for updateWindowAndroid() gave me two calls: > > at detaching > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > and reattaching > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > I think it's pretty clear from this method that windowAndroid param can be null. > That's not the question. > > > > > The first call passes null for windowAndroid. Without the check we'd set > device > > scale factor from the mContext (probably wrong one) but the window is in > > detached state. > > so either way is ok, but no null check is simpler > > > I wanted to avoid that. Removed null check.
timav@chromium.org changed reviewers: + tedchoc@chromium.org
Adding Ted as OWNERS reviewer.
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.
On 2016/08/23 22:56:37, Tima Vaisburd wrote: > Adding Ted as OWNERS reviewer. A gentle ping?
lgtm w/ nits sievers@ do you have any thoughts here? Also, my comment about removing getContext() is way outside the scope of this change and just some ramblings for followups in my mind. https://codereview.chromium.org/2268103002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2268103002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:627: * Gets DisplayMetrics from WindowAndroid's context if WindowAndroid exists, this just describes the code...I don't think this comment adds enough value to justify it https://codereview.chromium.org/2268103002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:634: if (displayContext == null) displayContext = getContext(); should we be getting rid of getContext() and simply using window.getContext() everywhere? https://codereview.chromium.org/2268103002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:640: * line switch Same here...the comment should just describe that it is returning the display density (potentially describing what that is). But we don't need to describe the code here. https://codereview.chromium.org/2268103002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:645: return (forceScaleFactor != null) ? Float.valueOf(forceScaleFactor) the ()s aren't necessary around the null check. And I would put the two conditions of the ternary on the second line (indented 8 from the start of the line vs the indenting that I suspect clang format thrust upon you)
maybe the code can then actually move from CVC into RenderCoordinates? and pass the WindowAndroid instead.
On 2016/08/24 21:38:24, sievers wrote: > maybe the code can then actually move from CVC into RenderCoordinates? and pass > the WindowAndroid instead. I moved setting logic to RenderCoordinates, but passed Context instead of WindowAndroid to ensure the context is not null. Also moved wheel scroll factor there. Please take another look.
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...
A gentle ping. Daniel, would you please take another look?
On 2016/08/30 00:01:19, Tima Vaisburd wrote: > A gentle ping. Daniel, would you please take another look? oops sorry, lgtm. but you might want to udpate the description since it uses the context and doesn't pass the WindowAndroid to calculate it.
Description was changed from ========== Get DisplayMetrics from window's context in ContentViewCore For Chrome DisplayMetrics should be extracted from WindowAndroid. Since WindowAndroid is obtained on the native size, postpone device scale factor settng till ContentViewCore::initialize(). BUG=620929 ========== to ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display dencity (device scale factor) and whee scroll factor from it. BUG=620929 ==========
Description was changed from ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display dencity (device scale factor) and whee scroll factor from it. BUG=620929 ========== to ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display density (device scale factor) and wheel scroll factor from it. BUG=620929 ==========
The CQ bit was unchecked by timav@chromium.org
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2268103002/#ps100001 (title: "Moved DisplayMetrics logic and wheel scroll factor to RenderCoordinates")
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 ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display density (device scale factor) and wheel scroll factor from it. BUG=620929 ========== to ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display density (device scale factor) and wheel scroll factor from it. BUG=620929 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display density (device scale factor) and wheel scroll factor from it. BUG=620929 ========== to ========== Use WindowAndroid to get display metrics in ContentViewCore Since Chrome can do window reparenting, DisplayMetrics should be extracted from the Context that is associated with the window. In this CL we pass the context to RenderCoordinates that obtains the display density (device scale factor) and wheel scroll factor from it. BUG=620929 Committed: https://crrev.com/8521fda07ea8d04953d856873a6e08afb96701d2 Cr-Commit-Position: refs/heads/master@{#415158} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8521fda07ea8d04953d856873a6e08afb96701d2 Cr-Commit-Position: refs/heads/master@{#415158} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
