Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670853002:
[Payments] Animates EditorView in and out (Closed)
|
Created:
3 years, 10 months ago by gogerald1 Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAnimates EditorView in and out
This CL also fixed a revealed integration test bug
BUG=687312
Review-Url: https://codereview.chromium.org/2670853002
Cr-Commit-Position: refs/heads/master@{#448424}
Committed: https://chromium.googlesource.com/chromium/src/+/3e4c2f7cca38031be613acc6a0afb98b74b980af
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : more smooth #Patch Set 4 : fix test #
Total comments: 6
Patch Set 5 : address comments #
Total comments: 12
Patch Set 6 : address comments #
Total comments: 8
Patch Set 7 : format #
Messages
Total messages: 51 (36 generated)
Description was changed from ========== animate EditorView in and out format draft new interpolator draft BUG= ========== to ========== Animates EditorView in and out BUG=687312 ==========
The CQ bit was checked by gogerald@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gogerald@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 gogerald@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.
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by gogerald@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...
gogerald@chromium.org changed reviewers: + rouslan@chromium.org, tedchoc@chromium.org
Hi rouslan@ and tedchoc@, PTAL, Check the video attached on the bug for result. Further performance optimization for nexus 5 in next CL.
On 2017/02/03 15:21:13, gogerald1 wrote: > Further performance optimization > for nexus 5 in next CL. I'd prefer to have the performance optimization in this CL, please.
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 gogerald@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...
Here is the optimized solution as preferred. The animation looks smooth on my nexus 5 when workload is light on the device. I do not see abnormal 'invalidate' operations according to "show hardware layers updates". So that's pretty much what we can do right now. Might flat view hierarchy in future for general efficiency.
Description was changed from ========== Animates EditorView in and out BUG=687312 ========== to ========== Animates EditorView in and out This CL also fixed a revealed integration test bug BUG=687312 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:525: if (getCurrentFocus() != null) UiUtils.showKeyboard(getCurrentFocus()); Don't show the keyboard if the current focus is on a dropdown. https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:682: // Notify ready for input for test if this is finishing editing item. Why does this need to be posted? A post like this usually means that some other code needs to run beforehand. This style of code is brittle, however, because it makes an implicit assumption about order of functions being executed. The best approach is to find the piece of code that runs last and put notifyReadyForInput() in there. https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:1047: mIsExpandedToFullHeight = true; Given how tricky this patch is, would you mind splitting the rename of mIsShowingEditDialog into mIsEpxandedToFullHeight into a separate patch?
Hi rouslan@, please take another look, https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:525: if (getCurrentFocus() != null) UiUtils.showKeyboard(getCurrentFocus()); On 2017/02/06 17:03:25, rouslan wrote: > Don't show the keyboard if the current focus is on a dropdown. UiUtils.showKeyboard only show keyboard for the view if necessary. Dropdown do not need soft keyboard, so it will show soft keyboard by invoking this function. Add a comment for clearance, https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:682: // Notify ready for input for test if this is finishing editing item. On 2017/02/06 17:03:25, rouslan wrote: > Why does this need to be posted? A post like this usually means that some other > code needs to run beforehand. This style of code is brittle, however, because it > makes an implicit assumption about order of functions being executed. The best > approach is to find the piece of code that runs last and put > notifyReadyForInput() in there. Done. Thought it's natural to avoid extra boolean and potential cycle call https://codereview.chromium.org/2670853002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:1047: mIsExpandedToFullHeight = true; On 2017/02/06 17:03:25, rouslan wrote: > Given how tricky this patch is, would you mind splitting the rename of > mIsShowingEditDialog into mIsEpxandedToFullHeight into a separate patch? Done.
https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:477: if (mDialogInOutAnimator != null || !isShowing()) return; what happens if we are currently going through the show animation (i.e. you start showing but the user hits back before completion)? Should we just cancel the animator and then animate again? https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:494: private class DialogShowingAnimator extends AnimatorListenerAdapter { I wouldn't use a class for this. I would just inline the animator declaration in onShow(...) above. Then you should also look at CancelAwareAnimatorListener because onEnd is called even if cancel is called first. https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:501: textField.setEnabled(false); Are there any cases where a textfield is disabled initially? If so, I could see this causing them to be incorrectly enabled on completion. If we want to be overly paranoid, I think we could could add a tag object that marks their initial enabled-ness. https://developer.android.com/reference/android/view/View.html#setTag(int, java.lang.Object) But if that is overkill, then we can ignore this for now. https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:504: mLayout.setLayerType(View.LAYER_TYPE_HARDWARE, null); You likely can't/shouldn't do this on svelte. By not doing hardware acceleration, we save a good amount of memory on svelte. This "might" negate that if anyone on svelte triggers this. Adding dskiba@ to keep me honest here as I don't know what cause the acceleration state to change. If this is what we need to do, we might have to simply disable this optimization for SysUtils#isLowEndDevice Or maybe I'm completely wrong and this is fine. ---- But in general, the fact that you are animating a Dialog is likely the source of your jank. It is a separate Window, likely a separate Surface entirely. I wouldn't be surprised if this was an overlaid view that you could do it without this. As a result, you'll be fighting with onLayout slowness by trying to run this at the same time as initially showing. As a quick test, what happens if you set the content to be INVISIBLE, add a layout listener to the dialog, and start animating after the initial layout has occurred? At INVISIBLE, you'll be doing all the expensive measurement without the drawing, so you could cover up a fair amount of the jank. https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:549: private class DialogDismissingAnimator extends AnimatorListenerAdapter { same comment, I would just inline this into dismissDialog
tedchoc@chromium.org changed reviewers: + dskiba@chromium.org
+dskiba for the hardware acceleration comment in: https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr...
https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:504: mLayout.setLayerType(View.LAYER_TYPE_HARDWARE, null); On 2017/02/06 18:27:57, Ted C wrote: > You likely can't/shouldn't do this on svelte. By not doing hardware > acceleration, we save a good amount of memory on svelte. This "might" negate > that if anyone on svelte triggers this. > > Adding dskiba@ to keep me honest here as I don't know what cause the > acceleration state to change. > > If this is what we need to do, we might have to simply disable this optimization > for SysUtils#isLowEndDevice > > Or maybe I'm completely wrong and this is fine. > > ---- > > But in general, the fact that you are animating a Dialog is likely the source of > your jank. It is a separate Window, likely a separate Surface entirely. I > wouldn't be surprised if this was an overlaid view that you could do it without > this. > > As a result, you'll be fighting with onLayout slowness by trying to run this at > the same time as initially showing. > > As a quick test, what happens if you set the content to be INVISIBLE, add a > layout listener to the dialog, and start animating after the initial layout has > occurred? At INVISIBLE, you'll be doing all the expensive measurement without > the drawing, so you could cover up a fair amount of the jank. Regarding LAYER_TYPE_HARDWARE: I think we're safe here. According to docs " LAYER_TYPE_HARDWARE: The view is rendered in hardware into a hardware texture if the application is hardware accelerated. If the application is not hardware accelerated, this layer type behaves the same as LAYER_TYPE_SOFTWARE. " So on svelte (where the activity is not HW accelerated), setting that type shouldn't have any effect.
The CQ bit was checked by gogerald@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...
Hi Ted, please take another look, https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:477: if (mDialogInOutAnimator != null || !isShowing()) return; On 2017/02/06 18:27:57, Ted C wrote: > what happens if we are currently going through the show animation (i.e. you > start showing but the user hits back before completion)? > > Should we just cancel the animator and then animate again? Hit android back button dismisses the dialog since we did not control that event. This dismissDialog is only for 'done', 'cancel', 'back' button on the dialog. I think it's good to ignore them when animation is going on. https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:494: private class DialogShowingAnimator extends AnimatorListenerAdapter { On 2017/02/06 18:27:57, Ted C wrote: > I wouldn't use a class for this. I would just inline the animator declaration > in onShow(...) above. > > Then you should also look at CancelAwareAnimatorListener because onEnd is called > even if cancel is called first. Done. https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:501: textField.setEnabled(false); On 2017/02/06 18:27:57, Ted C wrote: > Are there any cases where a textfield is disabled initially? If so, I could see > this causing them to be incorrectly enabled on completion. > > If we want to be overly paranoid, I think we could could add a tag object that > marks their initial enabled-ness. > > https://developer.android.com/reference/android/view/View.html#setTag(int, > java.lang.Object) > > But if that is overkill, then we can ignore this for now. No EditText is disabled initially until now, do not see that case is necessary in the near future https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:504: mLayout.setLayerType(View.LAYER_TYPE_HARDWARE, null); On 2017/02/06 18:27:57, Ted C wrote: > You likely can't/shouldn't do this on svelte. By not doing hardware > acceleration, we save a good amount of memory on svelte. This "might" negate > that if anyone on svelte triggers this. > > Adding dskiba@ to keep me honest here as I don't know what cause the > acceleration state to change. > > If this is what we need to do, we might have to simply disable this optimization > for SysUtils#isLowEndDevice > > Or maybe I'm completely wrong and this is fine. > > ---- > > But in general, the fact that you are animating a Dialog is likely the source of > your jank. It is a separate Window, likely a separate Surface entirely. I > wouldn't be surprised if this was an overlaid view that you could do it without > this. > > As a result, you'll be fighting with onLayout slowness by trying to run this at > the same time as initially showing. > > As a quick test, what happens if you set the content to be INVISIBLE, add a > layout listener to the dialog, and start animating after the initial layout has > occurred? At INVISIBLE, you'll be doing all the expensive measurement without > the drawing, so you could cover up a fair amount of the jank. Hardware layer is safe for svelte device since we disabled hardware acceleration for them as dskiba@ mentioned. Also do not see improvement by 'animatorSet.setStartDelay(200)' without using hardware layer as we talked offline (still jank). https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:549: private class DialogDismissingAnimator extends AnimatorListenerAdapter { On 2017/02/06 18:27:57, Ted C wrote: > same comment, I would just inline this into dismissDialog Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:476: for (EditText textField : mEditableTextFields) { "for (int i = 0..." loop is more efficient for List on Android. https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:495: for (EditText textField : mEditableTextFields) { "for (int i = 0..." loop is more efficient for List on Android. https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:702: if (isFinishingEditItem) { single-line
lgtm https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:477: if (mDialogInOutAnimator != null || !isShowing()) return; On 2017/02/06 20:22:58, gogerald1 wrote: > On 2017/02/06 18:27:57, Ted C wrote: > > what happens if we are currently going through the show animation (i.e. you > > start showing but the user hits back before completion)? > > > > Should we just cancel the animator and then animate again? > > Hit android back button dismisses the dialog since we did not control that > event. > This dismissDialog is only for 'done', 'cancel', 'back' button on the dialog. I > think it's good to ignore them when animation is going on. FWIW...seems like you could override and keep the showing and hiding consistent: https://developer.android.com/reference/android/app/Dialog.html#onBackPressed() https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:286: ObjectAnimator.ofFloat(mLayout, View.TRANSLATION_Y, 0f, mLayout.getHeight()); for animations, you "shouldn't" need the third param to this function. ofFloat(mLayout, View.TRANSLATION_Y, mLayout.getHeight()); By default the properties pull the current value and animate from that to the desired end value. With it written like this, it will always snap to the start value before animating. It's probably fine here since you're not canceling animations (and you'd have to change the initialization to start in the default hidden case (alpha = 0, translation = getHeight()). Probably not worth it for now, just an FYI
rouslan@ another look? https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:286: ObjectAnimator.ofFloat(mLayout, View.TRANSLATION_Y, 0f, mLayout.getHeight()); On 2017/02/06 21:35:54, Ted C wrote: > for animations, you "shouldn't" need the third param to this function. > > ofFloat(mLayout, View.TRANSLATION_Y, mLayout.getHeight()); > > By default the properties pull the current value and animate from that to the > desired end value. With it written like this, it will always snap to the start > value before animating. > > It's probably fine here since you're not canceling animations (and you'd have to > change the initialization to start in the default hidden case (alpha = 0, > translation = getHeight()). > > Probably not worth it for now, just an FYI Acknowledged. https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:476: for (EditText textField : mEditableTextFields) { On 2017/02/06 21:04:39, rouslan wrote: > "for (int i = 0..." loop is more efficient for List on Android. Done. https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:495: for (EditText textField : mEditableTextFields) { On 2017/02/06 21:04:40, rouslan wrote: > "for (int i = 0..." loop is more efficient for List on Android. Done. https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2670853002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:702: if (isFinishingEditItem) { On 2017/02/06 21:04:40, rouslan wrote: > single-line Done.
The CQ bit was checked by gogerald@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...
lgtm
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 gogerald@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/2670853002/#ps200001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486421144399540,
"parent_rev": "faaa2fd0a05f1622d9a8806da118d4f3b602e707", "commit_rev":
"3e4c2f7cca38031be613acc6a0afb98b74b980af"}
Message was sent while issue was closed.
Description was changed from ========== Animates EditorView in and out This CL also fixed a revealed integration test bug BUG=687312 ========== to ========== Animates EditorView in and out This CL also fixed a revealed integration test bug BUG=687312 Review-Url: https://codereview.chromium.org/2670853002 Cr-Commit-Position: refs/heads/master@{#448424} Committed: https://chromium.googlesource.com/chromium/src/+/3e4c2f7cca38031be613acc6a0af... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/3e4c2f7cca38031be613acc6a0af... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
