Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2414173002:
Moved FORCE_DEVICE_SCALE_FACTOR to DisplayAndroid (Closed)
|
Created:
4 years, 2 months ago by Tima Vaisburd Modified:
4 years, 2 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoved FORCE_DEVICE_SCALE_FACTOR to DisplayAndroid
On Java side the commend line switch --force-device_scale-factor
used to be processed by RenderCoordinates (a part of ContentViewCore),
move it from content/ fo ui/ layer.
This is a prerequisite to https://codereview.chromium.org/2300463002
BUG=620929
Committed: https://crrev.com/c4a49d706ecb5230be690ae4558368e170986eb2
Cr-Commit-Position: refs/heads/master@{#425539}
Patch Set 1 #Patch Set 2 : getDIPScale returns float #
Total comments: 6
Patch Set 3 : Addressed comments #
Total comments: 14
Patch Set 4 : Used Float instead of float for command line value #Patch Set 5 : Added log in case of invalid input #
Dependent Patchsets: Messages
Total messages: 38 (22 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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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.
timav@chromium.org changed reviewers: + boliu@chromium.org, tedchoc@chromium.org
PTAL.
https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { you don't have to add the parameter, you can get the DisplayAndroid from a context but this method name is just confusing, should be something like updateDeviceScaleFactorFromContext https://codereview.chromium.org/2414173002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:126: return (float) mDisplayMetrics.density; do the cast in the client?
https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { On 2016/10/14 00:37:17, boliu wrote: > you don't have to add the parameter, you can get the DisplayAndroid from a > context How to do that? I only see how to get Display, but not DipslayAndroid. On the other hand, I think it is possible to pass WindowAndroid as the parameter.
https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { On 2016/10/14 00:58:10, Tima Vaisburd wrote: > On 2016/10/14 00:37:17, boliu wrote: > > you don't have to add the parameter, you can get the DisplayAndroid from a > > context > > How to do that? I only see how to get Display, but not DipslayAndroid. > On the other hand, I think it is possible to pass WindowAndroid as the > parameter. https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/disp...
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/2414173002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:63: void setDeviceScaleFactor(float dipScale, Context context) { On 2016/10/14 01:05:30, boliu wrote: > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/disp... Got it, thanks. Still, passing WindowAndroid seems more straightforward, I've done this, let me know what you think. Renamed the method to updateDeviceScaleFactorFromWindow https://codereview.chromium.org/2414173002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:126: return (float) mDisplayMetrics.density; On 2016/10/14 00:37:17, boliu wrote: > do the cast in the client? The density is actually defined as float: https://developer.android.com/reference/android/util/DisplayMetrics.html#density Changed the return type.
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:64: void updateDeviceScaleFactorFromWindow(WindowAndroid windowAndroid) { you dropped the null windowAndroid check, in which case old code update from the application context arguably the existing behavior is wrong too, and we should just wait until we get a WindowAndroid instead, but maybe not do that in this CL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:64: void updateDeviceScaleFactorFromWindow(WindowAndroid windowAndroid) { On 2016/10/14 02:17:17, boliu wrote: > you dropped the null windowAndroid check, in which case old code update from the > application context > > arguably the existing behavior is wrong too, and we should just wait until we > get a WindowAndroid instead, but maybe not do that in this CL As far as I understand if windowAndroid == null this method won't be called, and should not be. In that respect I think this version is better? I also changed the behavior when windowAndroid exists, but the Context is null. Before, we used application context, and now we use a "reasonable default". I assumed since both are incorrect, it is ok to go the simplest way.
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:64: void updateDeviceScaleFactorFromWindow(WindowAndroid windowAndroid) { On 2016/10/14 02:17:17, boliu wrote: > we should just wait until we get a WindowAndroid instead Wait, isn't it what this CL is doing? I meant it to be called with valid windowAndroid...
lgtm https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:64: void updateDeviceScaleFactorFromWindow(WindowAndroid windowAndroid) { On 2016/10/14 17:55:41, Tima Vaisburd wrote: > On 2016/10/14 02:17:17, boliu wrote: > > you dropped the null windowAndroid check, in which case old code update from > the > > application context > > > > arguably the existing behavior is wrong too, and we should just wait until we > > get a WindowAndroid instead, but maybe not do that in this CL > > As far as I understand if windowAndroid == null this method won't be called, and > should not be. In that respect I think this version is better? > I also changed the behavior when windowAndroid exists, but the Context is null. > Before, we used application context, and now we use a "reasonable default". I > assumed since both are incorrect, it is ok to go the simplest way. Oh I missed the calls moved in CVC. Ok yeah this should be ok.
lgtm w/ some small comments https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:76: && context.getTheme().resolveAttribute( I would move this line up to the previous one. https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:53: private static float sForcedDIPScale = -1.0f; I would just use a capital F Float here and rely on null to determine unset-ness instead of -1.0f. https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:63: sForcedDIPScale = Math.abs(Float.valueOf(forcedScaleAsString)); What happens if the command line contains a negative value? Should we do any validation here? Throw an exception?
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:76: && context.getTheme().resolveAttribute( On 2016/10/14 18:32:44, Ted C wrote: > I would move this line up to the previous one. Yes, I did this initially. This is from git cl format... https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:63: sForcedDIPScale = Math.abs(Float.valueOf(forcedScaleAsString)); On 2016/10/14 18:32:45, Ted C wrote: > What happens if the command line contains a negative value? Should we do any > validation here? Throw an exception? Math.abs() was introduced exactly for that purpose. So negative values are silently multiplied by -1. Is this ok?
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:76: && context.getTheme().resolveAttribute( On 2016/10/14 18:52:59, Tima Vaisburd wrote: > On 2016/10/14 18:32:44, Ted C wrote: > > I would move this line up to the previous one. > > Yes, I did this initially. This is from git cl format... git cl format for java isn't something you should trust. https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:63: sForcedDIPScale = Math.abs(Float.valueOf(forcedScaleAsString)); On 2016/10/14 18:52:59, Tima Vaisburd wrote: > On 2016/10/14 18:32:45, Ted C wrote: > > What happens if the command line contains a negative value? Should we do any > > validation here? Throw an exception? > > Math.abs() was introduced exactly for that purpose. So negative values are > silently multiplied by -1. Is this ok? I would actually ignore the value if it is negative. I think making it positive is a bit weird because they gave us invalid info. I would treat it the same as you treat with invalid ones in the NumberFormatException case. Also in either case, I would also add a log statement stating that we received an invalid value.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/2414173002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:76: && context.getTheme().resolveAttribute( On 2016/10/14 20:33:52, Ted C wrote: > On 2016/10/14 18:52:59, Tima Vaisburd wrote: > > On 2016/10/14 18:32:44, Ted C wrote: > > > I would move this line up to the previous one. > > > > Yes, I did this initially. This is from git cl format... > > git cl format for java isn't something you should trust. Done. https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java (right): https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:53: private static float sForcedDIPScale = -1.0f; On 2016/10/14 18:32:44, Ted C wrote: > I would just use a capital F Float here and rely on null to determine unset-ness > instead of -1.0f. Done. https://codereview.chromium.org/2414173002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java:63: sForcedDIPScale = Math.abs(Float.valueOf(forcedScaleAsString)); On 2016/10/14 20:33:52, Ted C wrote: > On 2016/10/14 18:52:59, Tima Vaisburd wrote: > > On 2016/10/14 18:32:45, Ted C wrote: > > > What happens if the command line contains a negative value? Should we do > any > > > validation here? Throw an exception? > > > > Math.abs() was introduced exactly for that purpose. So negative values are > > silently multiplied by -1. Is this ok? > > I would actually ignore the value if it is negative. I think making it positive > is a bit weird because they gave us invalid info. I would treat it the same as > you treat with invalid ones in the NumberFormatException case. > > Also in either case, I would also add a log statement stating that we received > an invalid value. Done.
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 tedchoc@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2414173002/#ps80001 (title: "Added log in case of invalid input")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Moved FORCE_DEVICE_SCALE_FACTOR to DisplayAndroid On Java side the commend line switch --force-device_scale-factor used to be processed by RenderCoordinates (a part of ContentViewCore), move it from content/ fo ui/ layer. This is a prerequisite to https://codereview.chromium.org/2300463002 BUG=620929 ========== to ========== Moved FORCE_DEVICE_SCALE_FACTOR to DisplayAndroid On Java side the commend line switch --force-device_scale-factor used to be processed by RenderCoordinates (a part of ContentViewCore), move it from content/ fo ui/ layer. This is a prerequisite to https://codereview.chromium.org/2300463002 BUG=620929 Committed: https://crrev.com/c4a49d706ecb5230be690ae4558368e170986eb2 Cr-Commit-Position: refs/heads/master@{#425539} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c4a49d706ecb5230be690ae4558368e170986eb2 Cr-Commit-Position: refs/heads/master@{#425539} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
