Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1068123002:
Animate opening and closing of Android page info popup (Closed)
DescriptionAnimate opening and closing of Android page info popup
BUG=394135
Committed: https://crrev.com/df3c6a8ec1fcda1b733509fc9cadd80cab26520d
Cr-Commit-Position: refs/heads/master@{#325390}
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 16
Patch Set 3 : Review comments #
Total comments: 2
Patch Set 4 : Review comments #Messages
Total messages: 16 (7 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
tsergeant@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@, can you please review? See the bug for videos
https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:178: private class AnimatedEnterScrollView extends ScrollView { Despite my example using something similar, I doubt you actually need one. I used it because it was quick, but there is probably doable entirely with: http://developer.android.com/reference/android/view/View.html#addOnLayoutChan... which should remove the need for a custom view class. You should look at what listeners/observers you can add to mContainer and just drive it off of that. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:201: private static final int COPY_BUTTON_CLOSE_DELAY = 250; this isn't used https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:315: new Handler().postDelayed(new Runnable() { you can probably just use mContainer.postDelayed instead of creating a new handler. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:651: view.setAlpha(0f); You need to do this because of the delay start right? If so, you don't actually need the 0f on the next line. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:655: alphaAnim = ObjectAnimator.ofFloat(view, View.ALPHA, 1f, 0f); No need for the 1f here either. Instead it will use the current value, which is useful if a user closes the dialog while the showing animation is running (it would then animate from the current value instead of jumping to 1). And to that effect, you should probably keep a reference to the current animation and be able to cancel it if the user can exit before the initial animation is done (to avoid them clobbering each other). Be warned, onAnimationEnd will be called even if you call cancel (if I recall correctly). https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:658: alphaAnim.setDuration(FADE_DURATION); What does the combination of this duration and the delay add up to? Ideally the animation should run in the 2-300ms time frame, so you might want to tweak these numbers if it's 200+250...because 1/2 a second is realllllly long. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:670: translateAnim = ObjectAnimator.ofFloat(mContainer, View.TRANSLATION_Y, animHeight, 0f); same comment about not needing to specify the starting values for these two animations. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:689: if (!SysUtils.isLowEndDevice()) { why not animate on low end builds? I suspect these animations should behave fine on low memory phones. I would actually only disable animations in low battery situations, but I don't think that is exposed as of yet.
tedchoc@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor -- I'm OOO next week, so hopefully he can finish up the review if need be.
Patchset #3 (id:80001) has been deleted
dtrainor@ - I've addressed Ted's comments, can you please take a look? https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:178: private class AnimatedEnterScrollView extends ScrollView { On 2015/04/10 18:28:57, Ted C (OOO 4.13 - 4.21) wrote: > Despite my example using something similar, I doubt you actually need one. I > used it because it was quick, but there is probably doable entirely with: > > http://developer.android.com/reference/android/view/View.html#addOnLayoutChan... > > which should remove the need for a custom view class. You should look at what > listeners/observers you can add to mContainer and just drive it off of that. Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:201: private static final int COPY_BUTTON_CLOSE_DELAY = 250; On 2015/04/10 18:28:57, Ted C (OOO 4.13 - 4.21) wrote: > this isn't used Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:315: new Handler().postDelayed(new Runnable() { On 2015/04/10 18:28:56, Ted C (OOO 4.13 - 4.21) wrote: > you can probably just use mContainer.postDelayed instead of creating a new > handler. Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:651: view.setAlpha(0f); On 2015/04/10 18:28:57, Ted C (OOO 4.13 - 4.21) wrote: > You need to do this because of the delay start right? > > If so, you don't actually need the 0f on the next line. Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:655: alphaAnim = ObjectAnimator.ofFloat(view, View.ALPHA, 1f, 0f); On 2015/04/10 18:28:56, Ted C (OOO 4.13 - 4.21) wrote: > No need for the 1f here either. Instead it will use the current value, which is > useful if a user closes the dialog while the showing animation is running (it > would then animate from the current value instead of jumping to 1). > > And to that effect, you should probably keep a reference to the current > animation and be able to cancel it if the user can exit before the initial > animation is done (to avoid them clobbering each other). Be warned, > onAnimationEnd will be called even if you call cancel (if I recall correctly). Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:658: alphaAnim.setDuration(FADE_DURATION); On 2015/04/10 18:28:56, Ted C (OOO 4.13 - 4.21) wrote: > What does the combination of this duration and the delay add up to? Ideally the > animation should run in the 2-300ms time frame, so you might want to tweak these > numbers if it's 200+250...because 1/2 a second is realllllly long. The total is over half a second, yeah. I used to think it was slow, but maybe I'm just used to it now. Anyway, I've tweaked the numbers to be a little smaller, and the animation feels a lot snapper now. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:670: translateAnim = ObjectAnimator.ofFloat(mContainer, View.TRANSLATION_Y, animHeight, 0f); On 2015/04/10 18:28:56, Ted C (OOO 4.13 - 4.21) wrote: > same comment about not needing to specify the starting values for these two > animations. Done. https://codereview.chromium.org/1068123002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:689: if (!SysUtils.isLowEndDevice()) { On 2015/04/10 18:28:57, Ted C (OOO 4.13 - 4.21) wrote: > why not animate on low end builds? I suspect these animations should behave > fine on low memory phones. I would actually only disable animations in low > battery situations, but I don't think that is exposed as of yet. Sure, I was basing this off the overflow menu animation, which is disabled on low-end phones. Happy to get rid of this for now.
lgtm! https://codereview.chromium.org/1068123002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1068123002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:670: * Varies based on device: Low-end devices do not animate, tablets use the default Dialog Update the comment (low-end devices part no longer applies)
https://codereview.chromium.org/1068123002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1068123002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:670: * Varies based on device: Low-end devices do not animate, tablets use the default Dialog On 2015/04/15 16:37:49, David Trainor wrote: > Update the comment (low-end devices part no longer applies) Done.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/1068123002/#ps120001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1068123002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df3c6a8ec1fcda1b733509fc9cadd80cab26520d Cr-Commit-Position: refs/heads/master@{#325390} |
