Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960743002:
Adding animations for view pager transitions (Closed)
|
Created:
3 years, 5 months ago by Daniel Park Modified:
3 years, 5 months ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description-Adds clip and translation animations for context menu.
-Adds logic to correctly resize on orientation changes.
-Adds logic to set clip bounds on resize
(i.e. image loads or context menu shows/hides all of the link text).
-Sets rounded corners programmatically instead of in the context menu's xml file.
Note: this does not handle the image loading or link text view enlargement animations.
BUG=730329
Review-Url: https://codereview.chromium.org/2960743002
Cr-Commit-Position: refs/heads/master@{#488506}
Committed: https://chromium.googlesource.com/chromium/src/+/04a7c3fcd6d8d600f2aec4a89e260443dbf21f58
Patch Set 1 #Patch Set 2 : Animations for context menu resize #Patch Set 3 : Reverting dimens.xml change. #
Total comments: 22
Patch Set 4 : Comments #Patch Set 5 : Add a todo #
Total comments: 19
Patch Set 6 : Making resizing logic compatible with API 16+ #Patch Set 7 : Requests layout only when transitioning to a smaller size #
Total comments: 2
Patch Set 8 : Addressing theresa's comments #
Total comments: 17
Patch Set 9 : Addressing twellington's comments #Patch Set 10 : moving animator call #
Total comments: 10
Patch Set 11 : Addressing final comments. #
Messages
Total messages: 26 (9 generated)
Message was sent while issue was closed.
Description was changed from ========== format amend no amend BUG= ========== to ========== -Adds clip and translation animations for context menu -Adds logic to correctly resize on orientation changes -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text) BUG=730329 ==========
danielpark@chromium.org changed reviewers: + tedchoc@chromium.org, twellington@chromium.org
Description was changed from ========== -Adds clip and translation animations for context menu -Adds logic to correctly resize on orientation changes -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text) BUG=730329 ========== to ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). BUG=730329 ==========
Description was changed from ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). BUG=730329 ========== to ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). -Sets rounded corners programmatically instead of in the context menu's xml file. BUG=730329 ==========
Description was changed from ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). -Sets rounded corners programmatically instead of in the context menu's xml file. BUG=730329 ========== to ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). -Sets rounded corners programmatically instead of in the context menu's xml file. Note: this does not handle the image loading or link text view enlargement animations. BUG=730329 ==========
https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:94: int deviceHeightPx = getResources().getDisplayMetrics().heightPixels; nit: This is technically the current available height based on the app's window (which is what you want); the variable could use a better name. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:100: // finished loading or the link became fully visible). The CL description says "Note: this does not handle the image loading or link text view enlargement animations.", but this comment appears to indicate that they are handled? https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:103: mOldHeight = fullHeight; nit: this can fit on the line above if (menuHeight != 0) mOldHeight = fullHeight; https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:112: initAnimator(); nit: this can also fit on the line above https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:115: heightMeasureSpec = MeasureSpec.makeMeasureSpec( Should we measure before starting the animation? https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:123: @SuppressLint("NewApi") This is a real warning. It looks like setClipBounds wasn't added until API level 18, which is JellyBean MR2. We support back to API level 16 (the first JellyBean release). This will likely crash on JellyBean and JellyBean MR1. To avoid a crash, you could use ViewCompat#setClipBounds(View view, Rect clipBounds), but prior to API 18 the method does nothing so this animation will be incorrect. You may be able to clip the canvas in onDraw or otherwise write custom draw code so that the clipping is handled properly on all OS versions that Chrome supports. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:131: mAnimator.setDuration(250); nit: This should be a static final int at the top of the file. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:139: mAtMaxAnimationValue = false; Instead of keeping a class variable, you could just check: if (MathUtils.areFloatsEqual(animatedValue, 1) { mOldHeight = mClipHeight; ... return; } <all of your other code> https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:152: mAtMaxAnimationValue = true; It's more customary for us to do this sort of thing in onAnimationEnd() instead of onAnimationUpdate(). https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:167: drawable.setBounds( You should call drawable.mutate() before setting the bounds: "Make this drawable mutable. This operation cannot be reversed. A mutable drawable is guaranteed to not share its state with any other drawable. This is especially useful when you need to modify properties of drawables loaded from resources. By default, all drawables instances loaded from the same resource share a common state; if you modify the state of one instance, all the other instances will receive the same modification. Calling this method on a mutable Drawable will have no effect." https://developer.android.com/reference/android/graphics/drawable/Drawable.ht... Currently white_with_rounded_corners is only used for the context menu, but if it's used other places in the future and mutate isn't called we may end up with odd visual behavior due to the shared state.
https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:94: int deviceHeightPx = getResources().getDisplayMetrics().heightPixels; On 2017/07/14 16:56:38, Theresa wrote: > nit: This is technically the current available height based on the app's window > (which is what you want); the variable could use a better name. Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:100: // finished loading or the link became fully visible). On 2017/07/14 16:56:38, Theresa wrote: > The CL description says "Note: this does not handle the image loading or link > text view enlargement animations.", but this comment appears to indicate that > they are handled? Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:103: mOldHeight = fullHeight; On 2017/07/14 16:56:38, Theresa wrote: > nit: this can fit on the line above > > if (menuHeight != 0) mOldHeight = fullHeight; Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:112: initAnimator(); On 2017/07/14 16:56:38, Theresa wrote: > nit: this can also fit on the line above Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:115: heightMeasureSpec = MeasureSpec.makeMeasureSpec( On 2017/07/14 16:56:38, Theresa wrote: > Should we measure before starting the animation? Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:123: @SuppressLint("NewApi") On 2017/07/14 16:56:38, Theresa wrote: > This is a real warning. It looks like setClipBounds wasn't added until API level > 18, which is JellyBean MR2. We support back to API level 16 (the first JellyBean > release). This will likely crash on JellyBean and JellyBean MR1. > > To avoid a crash, you could use ViewCompat#setClipBounds(View view, Rect > clipBounds), but prior to API 18 the method does nothing so this animation will > be incorrect. > > You may be able to clip the canvas in onDraw or otherwise write custom draw code > so that the clipping is handled properly on all OS versions that Chrome > supports. Acknowledged. I added a todo. I'll play around with some things, but leaving it with ViewCompat for now. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:131: mAnimator.setDuration(250); On 2017/07/14 16:56:38, Theresa wrote: > nit: This should be a static final int at the top of the file. Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:139: mAtMaxAnimationValue = false; On 2017/07/14 16:56:38, Theresa wrote: > Instead of keeping a class variable, you could just check: > > if (MathUtils.areFloatsEqual(animatedValue, 1) { > mOldHeight = mClipHeight; > ... > return; > } > > <all of your other code> > Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:152: mAtMaxAnimationValue = true; On 2017/07/14 16:56:38, Theresa wrote: > It's more customary for us to do this sort of thing in onAnimationEnd() instead > of onAnimationUpdate(). Done. https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:167: drawable.setBounds( On 2017/07/14 16:56:38, Theresa wrote: > You should call drawable.mutate() before setting the bounds: > > "Make this drawable mutable. This operation cannot be reversed. A mutable > drawable is guaranteed to not share its state with any other drawable. This is > especially useful when you need to modify properties of drawables loaded from > resources. By default, all drawables instances loaded from the same resource > share a common state; if you modify the state of one instance, all the other > instances will receive the same modification. Calling this method on a mutable > Drawable will have no effect." > > https://developer.android.com/reference/android/graphics/drawable/Drawable.ht... > > > Currently white_with_rounded_corners is only used for the context menu, but if > it's used other places in the future and mutate isn't called we may end up with > odd visual behavior due to the shared state. Done.
https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:123: @SuppressLint("NewApi") On 2017/07/14 23:15:39, Daniel Park wrote: > On 2017/07/14 16:56:38, Theresa wrote: > > This is a real warning. It looks like setClipBounds wasn't added until API > level > > 18, which is JellyBean MR2. We support back to API level 16 (the first > JellyBean > > release). This will likely crash on JellyBean and JellyBean MR1. > > > > To avoid a crash, you could use ViewCompat#setClipBounds(View view, Rect > > clipBounds), but prior to API 18 the method does nothing so this animation > will > > be incorrect. > > > > You may be able to clip the canvas in onDraw or otherwise write custom draw > code > > so that the clipping is handled properly on all OS versions that Chrome > > supports. > > Acknowledged. I added a todo. I'll play around with some things, but leaving it > with ViewCompat for now. As you are already overwriting onDraw, you should be able to just save mClipHeight and do something like this inside of onDraw and get rid of setClipBounds entirely. boolean clipped = false; if (mClipHeight != 0) { canvas.save(); canvas.clipRect(0, 0, getMeasuredWidth(), mClipHeight); clipped = true; } super.onDraw(); if (clipped) { canvas.restore(); } setClipBounds seems to be needed for when you haven't overwritten the view. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:126: mAnimator = ValueAnimator.ofFloat(0f, 1f); I would do: if (mAnimator != null && mAnimator.isRunning()) mAnimator.cancel(); Oh, we are reusing the animator, so maybe start on it doesn't cause it to jump. If we are reusing it, I would do the check here: if (mAnimator != null) return; Then it is harder for callers to misuse it. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:150: mAnimator.addListener(new AnimatorListener() { take a look at CancelAwareAnimatorListener or AnimatorListenerAdapter to avoid overwriting all the unneeded methods https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:161: requestLayout(); do we only need to request layout if the new height is smaller? If we've grown to the bigger height, the layout "should" be unneeded. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:174: Drawable drawable = ApiCompatibilityUtils.getDrawable( we might want to save this to a class variable as mutate will cause all the state to be duplicated and we don't really need that to be done per frame (we can mutate it once and just manipulate it forever).
https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:121: // TODO(injae): Find a different way to mimic animations I think we should address this TODO now, since it may change the overall approach for the animation (hopefully it doesn't, but it'd be good to land something that works on all OS versions reasonably well). https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:146: return; We're already at the end of the method so this early return doesn't skip anything and isn't necessary. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:150: mAnimator.addListener(new AnimatorListener() { You can use AnimatorListenerAdapter to avoid providing empty implementations for onAnimationStart(), onAnimationRepeat() and onAnimationCancel(). https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:162: setTranslationY(0); nit: if you can flip setTranslationY(0) and requestLayout(), I think that ordering makes a bit more sense.
https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:123: @SuppressLint("NewApi") On 2017/07/17 21:25:31, Ted C wrote: > On 2017/07/14 23:15:39, Daniel Park wrote: > > On 2017/07/14 16:56:38, Theresa wrote: > > > This is a real warning. It looks like setClipBounds wasn't added until API > > level > > > 18, which is JellyBean MR2. We support back to API level 16 (the first > > JellyBean > > > release). This will likely crash on JellyBean and JellyBean MR1. > > > > > > To avoid a crash, you could use ViewCompat#setClipBounds(View view, Rect > > > clipBounds), but prior to API 18 the method does nothing so this animation > > will > > > be incorrect. > > > > > > You may be able to clip the canvas in onDraw or otherwise write custom draw > > code > > > so that the clipping is handled properly on all OS versions that Chrome > > > supports. > > > > Acknowledged. I added a todo. I'll play around with some things, but leaving > it > > with ViewCompat for now. > > As you are already overwriting onDraw, you should be able to just save > mClipHeight > and do something like this inside of onDraw and get rid of setClipBounds > entirely. > > boolean clipped = false; > if (mClipHeight != 0) { > canvas.save(); > canvas.clipRect(0, 0, getMeasuredWidth(), mClipHeight); > clipped = true; > } > super.onDraw(); > if (clipped) { > canvas.restore(); > } > > setClipBounds seems to be needed for when you haven't overwritten the view. Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:121: // TODO(injae): Find a different way to mimic animations On 2017/07/17 21:28:25, Theresa wrote: > I think we should address this TODO now, since it may change the overall > approach for the animation (hopefully it doesn't, but it'd be good to land > something that works on all OS versions reasonably well). Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:126: mAnimator = ValueAnimator.ofFloat(0f, 1f); On 2017/07/17 21:25:31, Ted C wrote: > I would do: > > if (mAnimator != null && mAnimator.isRunning()) mAnimator.cancel(); > > Oh, we are reusing the animator, so maybe start on it doesn't cause it to jump. > If we are reusing it, I would do the check here: > > if (mAnimator != null) return; > > Then it is harder for callers to misuse it. Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:146: return; On 2017/07/17 21:28:25, Theresa wrote: > We're already at the end of the method so this early return doesn't skip > anything and isn't necessary. the animated value will hit 1 twice if the animation duration is set to 5x or 10x which shouldn't happen outside of debugging purposes. this handles that case. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:150: mAnimator.addListener(new AnimatorListener() { On 2017/07/17 21:25:31, Ted C wrote: > take a look at CancelAwareAnimatorListener or AnimatorListenerAdapter to avoid > overwriting all the unneeded methods Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:161: requestLayout(); On 2017/07/17 21:25:31, Ted C wrote: > do we only need to request layout if the new height is smaller? If we've grown > to the bigger height, the layout "should" be unneeded. Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:162: setTranslationY(0); On 2017/07/17 21:28:25, Theresa wrote: > nit: if you can flip setTranslationY(0) and requestLayout(), I think that > ordering makes a bit more sense. Done. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:174: Drawable drawable = ApiCompatibilityUtils.getDrawable( On 2017/07/17 21:25:31, Ted C wrote: > we might want to save this to a class variable as mutate will cause all the > state to be duplicated and we don't really need that to be done per frame (we > can mutate it once and just manipulate it forever). Done.
https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:126: mAnimator = ValueAnimator.ofFloat(0f, 1f); On 2017/07/19 00:30:09, Daniel Park wrote: > On 2017/07/17 21:25:31, Ted C wrote: > > I would do: > > > > if (mAnimator != null && mAnimator.isRunning()) mAnimator.cancel(); > > > > Oh, we are reusing the animator, so maybe start on it doesn't cause it to > jump. > > If we are reusing it, I would do the check here: > > > > if (mAnimator != null) return; > > > > Then it is harder for callers to misuse it. > > Done. If the animator is running and another layout change occurs, you may see some odd experience. For example, if the height change is animating due to the tab switching and the user rotates the device, what happens? https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:146: return; On 2017/07/19 00:30:08, Daniel Park wrote: > On 2017/07/17 21:28:25, Theresa wrote: > > We're already at the end of the method so this early return doesn't skip > > anything and isn't necessary. > > the animated value will hit 1 twice if the animation duration is set to 5x or > 10x which shouldn't happen outside of debugging purposes. this handles that > case. My point is that nothing happens in this method after this code block, so returning early is a no-op. Am I'm missing something? https://codereview.chromium.org/2960743002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:125: if (mCanvas != null) mCanvas.clipRect(new Rect(0, 0, width, mClipHeight)); Is this needed now that you're setting the clipRect on the canvas in onDraw()?
https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:126: mAnimator = ValueAnimator.ofFloat(0f, 1f); On 2017/07/19 15:23:20, Theresa wrote: > On 2017/07/19 00:30:09, Daniel Park wrote: > > On 2017/07/17 21:25:31, Ted C wrote: > > > I would do: > > > > > > if (mAnimator != null && mAnimator.isRunning()) mAnimator.cancel(); > > > > > > Oh, we are reusing the animator, so maybe start on it doesn't cause it to > > jump. > > > If we are reusing it, I would do the check here: > > > > > > if (mAnimator != null) return; > > > > > > Then it is harder for callers to misuse it. > > > > Done. > > If the animator is running and another layout change occurs, you may see some > odd experience. For example, if the height change is animating due to the tab > switching and the user rotates the device, what happens? The animation finishes and is laid out properly; nothing unusual happens. https://codereview.chromium.org/2960743002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:146: return; On 2017/07/19 15:23:20, Theresa wrote: > On 2017/07/19 00:30:08, Daniel Park wrote: > > On 2017/07/17 21:28:25, Theresa wrote: > > > We're already at the end of the method so this early return doesn't skip > > > anything and isn't necessary. > > > > the animated value will hit 1 twice if the animation duration is set to 5x or > > 10x which shouldn't happen outside of debugging purposes. this handles that > > case. > > My point is that nothing happens in this method after this code block, so > returning early is a no-op. Am I'm missing something? Removed; the issue's not appearing anymore. https://codereview.chromium.org/2960743002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:125: if (mCanvas != null) mCanvas.clipRect(new Rect(0, 0, width, mClipHeight)); On 2017/07/19 15:23:20, Theresa wrote: > Is this needed now that you're setting the clipRect on the canvas in onDraw()? Removed.
https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:69: // getCurrentItem() does not take into account the tab layout unlike getChildCount(). What does getCurrentItem() return, then? https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:72: // Handles the case where this is called while the pager is scrolling between views. nit: Update the comment to say what is done in this scenario e.g. "The height should remain the same." https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:102: // (i.e. an image finished loading or the link became fully visible). nit: add "The pager will immediately snap to the new height." https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:108: // child. nit: add "The pager will animate to the height of the selected child." https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:112: mAnimator.start(); Should we start the animation after giving the super class a chance to measure? https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:130: // undesirably be set to 1f more than once. We're not really doing anything in response to the animateValue being set to 1f more than once any more, right? If not, let's drop this comment. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:157: boolean clipped = false; nit: add a blank line above this one. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:163: super.onDraw(canvas); nit: add blank lines around this super.onDraw() call
https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:69: // getCurrentItem() does not take into account the tab layout unlike getChildCount(). On 2017/07/20 15:25:00, Theresa wrote: > What does getCurrentItem() return, then? Added more detail. Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:72: // Handles the case where this is called while the pager is scrolling between views. On 2017/07/20 15:25:00, Theresa wrote: > nit: Update the comment to say what is done in this scenario e.g. "The height > should remain the same." Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:102: // (i.e. an image finished loading or the link became fully visible). On 2017/07/20 15:25:00, Theresa wrote: > nit: add "The pager will immediately snap to the new height." Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:108: // child. On 2017/07/20 15:25:00, Theresa wrote: > nit: add "The pager will animate to the height of the selected child." Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:112: mAnimator.start(); On 2017/07/20 15:25:00, Theresa wrote: > Should we start the animation after giving the super class a chance to measure? I prefer to keep it here since the results are the same. Moving it after super.onMeasure implies to me that an animation would start no matter what case. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:130: // undesirably be set to 1f more than once. On 2017/07/20 15:25:00, Theresa wrote: > We're not really doing anything in response to the animateValue being set to 1f > more than once any more, right? If not, let's drop this comment. oops. Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:157: boolean clipped = false; On 2017/07/20 15:25:00, Theresa wrote: > nit: add a blank line above this one. Done. https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:163: super.onDraw(canvas); On 2017/07/20 15:25:00, Theresa wrote: > nit: add blank lines around this super.onDraw() call Done.
https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:112: mAnimator.start(); On 2017/07/20 18:55:19, Daniel Park wrote: > On 2017/07/20 15:25:00, Theresa wrote: > > Should we start the animation after giving the super class a chance to > measure? > > I prefer to keep it here since the results are the same. > Moving it after super.onMeasure implies to me that an animation would start no > matter what case. Done.
lgtm https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:119: // The animation is only visible when switching to a tab with a different height. nit: s/visible/run
owners stamp w/ nits https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:34: private int mDifferenceInHeight = 0; 0 is the default value in java, so you don't need to set that (it actually makes the binary a "bit" bigger too). https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:36: private final int mContextMenuMinimumPaddingPx = nit, but put the finals above non-finals https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:38: private final Drawable mDrawable = ApiCompatibilityUtils.getDrawable( Let's call this something like mBackgroundDrawable. https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:41: public TabularContextMenuViewPager(Context context) { I think you should be able to delete this constructor. The second one is the one needed for inflation, so I think this just makes your class a bit more complicated than needed.
lgtm
https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:34: private int mDifferenceInHeight = 0; On 2017/07/20 22:00:55, Ted C wrote: > 0 is the default value in java, so you don't need to set that (it actually makes > the binary a "bit" bigger too). Done. https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:36: private final int mContextMenuMinimumPaddingPx = On 2017/07/20 22:00:55, Ted C wrote: > nit, but put the finals above non-finals Done. https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:38: private final Drawable mDrawable = ApiCompatibilityUtils.getDrawable( On 2017/07/20 22:00:55, Ted C wrote: > Let's call this something like mBackgroundDrawable. Done. https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:41: public TabularContextMenuViewPager(Context context) { On 2017/07/20 22:00:55, Ted C wrote: > I think you should be able to delete this constructor. The second one is the > one needed for inflation, so I think this just makes your class a bit more > complicated than needed. Done. https://codereview.chromium.org/2960743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:119: // The animation is only visible when switching to a tab with a different height. On 2017/07/20 21:54:58, Theresa wrote: > nit: s/visible/run Done.
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2960743002/#ps200001 (title: "Addressing final comments.")
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": 1500589207290560,
"parent_rev": "2f8de5cd8a28b23388b3d340f225e2c7a8180d2f", "commit_rev":
"04a7c3fcd6d8d600f2aec4a89e260443dbf21f58"}
Message was sent while issue was closed.
Description was changed from ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). -Sets rounded corners programmatically instead of in the context menu's xml file. Note: this does not handle the image loading or link text view enlargement animations. BUG=730329 ========== to ========== -Adds clip and translation animations for context menu. -Adds logic to correctly resize on orientation changes. -Adds logic to set clip bounds on resize (i.e. image loads or context menu shows/hides all of the link text). -Sets rounded corners programmatically instead of in the context menu's xml file. Note: this does not handle the image loading or link text view enlargement animations. BUG=730329 Review-Url: https://codereview.chromium.org/2960743002 Cr-Commit-Position: refs/heads/master@{#488506} Committed: https://chromium.googlesource.com/chromium/src/+/04a7c3fcd6d8d600f2aec4a89e26... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/04a7c3fcd6d8d600f2aec4a89e26... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
