Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2462603003:
Convert browser controls animation to use ratio (Closed)
DescriptionConvert browser controls animation to use ratio
This change will make it possible to use one code path for both top
and bottom controls animation. The general controls offset is now
also tracked as a ratio.
BUG=652892
Committed: https://crrev.com/5bfae06913f963b1b396323fd9774f0c278ff93d
Cr-Commit-Position: refs/heads/master@{#431477}
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : doc #Patch Set 4 : store ratio as instance #Patch Set 5 : offset stored as ratio #Patch Set 6 : nit #Patch Set 7 : amend ratio function name and doc #
Total comments: 2
Patch Set 8 : address comment #Patch Set 9 : use Math.abs for zero checks in tests #Patch Set 10 : fix float comparisons in tests #Patch Set 11 : add check in getContentOffset instead #Messages
Total messages: 31 (17 generated)
Description was changed from ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. BUG=652892 ========== to ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. The general controls offset is now also tracked as a ratio. BUG=652892 ==========
mdjones@chromium.org changed reviewers: + ianwen@chromium.org, tedchoc@chromium.org
ptal
mdjones@chromium.org changed reviewers: + aelias@chromium.org
+aelias
lgtm
Talked to mdjones@ with more context of the CL and I only have one optional nit to say. lgtm https://codereview.chromium.org/2462603003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2462603003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:429: @SuppressWarnings("SelfEquality") Instead of suppressing warnings, should we use Float.isNan()?
https://codereview.chromium.org/2462603003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2462603003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:429: @SuppressWarnings("SelfEquality") On 2016/11/10 02:14:23, Ian Wen wrote: > Instead of suppressing warnings, should we use Float.isNan()? Done.
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, ianwen@chromium.org Link to the patchset: https://codereview.chromium.org/2462603003/#ps140001 (title: "address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, ianwen@chromium.org Link to the patchset: https://codereview.chromium.org/2462603003/#ps160001 (title: "use Math.abs for zero checks in tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 mdjones@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...
Added criteria for control offset and changed other float comparisons to use epsilon. ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
reverted to simply add check in getContentOffset
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, ianwen@chromium.org Link to the patchset: https://codereview.chromium.org/2462603003/#ps200001 (title: "add check in getContentOffset instead")
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 ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. The general controls offset is now also tracked as a ratio. BUG=652892 ========== to ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. The general controls offset is now also tracked as a ratio. BUG=652892 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. The general controls offset is now also tracked as a ratio. BUG=652892 ========== to ========== Convert browser controls animation to use ratio This change will make it possible to use one code path for both top and bottom controls animation. The general controls offset is now also tracked as a ratio. BUG=652892 Committed: https://crrev.com/5bfae06913f963b1b396323fd9774f0c278ff93d Cr-Commit-Position: refs/heads/master@{#431477} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5bfae06913f963b1b396323fd9774f0c278ff93d Cr-Commit-Position: refs/heads/master@{#431477} |
