Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1936253002:
Fix indeterminate progress bar animation jank (Closed)
DescriptionFix indeterminate progress bar animation jank
Since the progress bar and the indeterminate progress bar animation
updated asynchronously, it was possible for the edges of the animation
and the progress bar to end at the wrong location. This change allows
the animation to be updated when the progress bar is updated.
BUG=464377
Committed: https://crrev.com/872e75170ba1312f8b084140891b6c48f432821e
Cr-Commit-Position: refs/heads/master@{#391295}
Patch Set 1 #Patch Set 2 : nit #
Total comments: 12
Patch Set 3 : address comments #
Depends on Patchset: Messages
Total messages: 13 (5 generated)
mdjones@chromium.org changed reviewers: + dfalcantara@chromium.org
PTAL
https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java (right): https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:30: /** The minimum amount of time that should pass between frames. */ Frames of what? Be more specific. I'm guessing this is maximum amount of time that a rendering pass can take to achieve 60 frames per second, but 1000 / 60 has a fraction and rounds poorly because you're storing it as a long. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:61: ProgressBarUpdateListener mListener; Why does this need to be package protected instead of private? https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:70: * A time listener that moves an ImageView across the progress bar. Out of date comment. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:75: mLastAnimatedFraction = animation.getAnimatedFraction(); Are you worried about a fraction being overwritten before it can be used? https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:77: // update here. Can't make heads or tails of this comment. Just say that not enough time has passed between animation frames. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:145: public void updateAnimation(float animatedFraction) { Why is this public?
https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java (right): https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:30: /** The minimum amount of time that should pass between frames. */ On 2016/05/03 05:05:58, dfalcantara wrote: > Frames of what? Be more specific. I'm guessing this is maximum amount of time > that a rendering pass can take to achieve 60 frames per second, but 1000 / 60 > has a fraction and rounds poorly because you're storing it as a long. Removed this constant. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:61: ProgressBarUpdateListener mListener; On 2016/05/03 05:05:57, dfalcantara wrote: > Why does this need to be package protected instead of private? It doesn't; fixed. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:70: * A time listener that moves an ImageView across the progress bar. On 2016/05/03 05:05:57, dfalcantara wrote: > Out of date comment. Done. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:75: mLastAnimatedFraction = animation.getAnimatedFraction(); On 2016/05/03 05:05:57, dfalcantara wrote: > Are you worried about a fraction being overwritten before it can be used? No, I want this update function driving the progress of the animation (but not necessarily applying transformations to it). https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:77: // update here. On 2016/05/03 05:05:57, dfalcantara wrote: > Can't make heads or tails of this comment. Just say that not enough time has > passed between animation frames. Done. https://codereview.chromium.org/1936253002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBarAnimatingView.java:145: public void updateAnimation(float animatedFraction) { On 2016/05/03 05:05:57, dfalcantara wrote: > Why is this public? Good question, fixed.
lgtm % probress typo
lgtm % probress typo
Description was changed from ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the probress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 ========== to ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the progress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 ==========
The CQ bit was checked by mdjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1936253002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1936253002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the progress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 ========== to ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the progress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the progress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 ========== to ========== Fix indeterminate progress bar animation jank Since the progress bar and the indeterminate progress bar animation updated asynchronously, it was possible for the edges of the animation and the progress bar to end at the wrong location. This change allows the animation to be updated when the progress bar is updated. BUG=464377 Committed: https://crrev.com/872e75170ba1312f8b084140891b6c48f432821e Cr-Commit-Position: refs/heads/master@{#391295} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/872e75170ba1312f8b084140891b6c48f432821e Cr-Commit-Position: refs/heads/master@{#391295} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
