Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 3 months ago by Ian Wen Modified:
5 years, 3 months ago Reviewers:
Yusuf CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Custom Tabs]Introduce the url scaling animation
This CL lets the urlbar to animate from large to small, when custom tab
is launching the first given URL. After the scaling animation, the title
bar will fade in.
BUG=505620
Committed: https://crrev.com/5afa2934ff3810486b77982d4a815763a7b17fdd
Cr-Commit-Position: refs/heads/master@{#348257}
Patch Set 1 #
Total comments: 9
Patch Set 2 : address comments #
Created: 5 years, 3 months ago
Depends on Patchset: Messages
Total messages: 13 (2 generated)
ianwen@chromium.org changed reviewers: + yusufo@chromium.org
ptal:)
https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:203: if (mShouldShowTitle && !TextUtils.equals(currentTab.getTitle(), currentTab.getUrl())) { the title can update more than once per page. We should only play the animation once per page. https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java (right): https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:89: android.R.interpolator.fast_out_slow_in); there is probably a bakedBezierinterpolator we should be using for this per material design spec. see other interpolators toolbar uses. https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:94: * How does the title animation work? This seems to be better suited to the class definition javadoc https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:106: if (!mShouldRunTitleAnimation) return; does this get reset on every page load? what happens when use clicks a link?
https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:203: if (mShouldShowTitle && !TextUtils.equals(currentTab.getTitle(), currentTab.getUrl())) { On 2015/09/03 05:46:56, Yusuf wrote: > the title can update more than once per page. We should only play the animation > once per page. We should probably unit test the resetting behavior for this
https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:203: if (mShouldShowTitle && !TextUtils.equals(currentTab.getTitle(), currentTab.getUrl())) { On 2015/09/03 05:46:56, Yusuf wrote: > the title can update more than once per page. We should only play the animation > once per page. startTitleAnimation() will do nothing if the title animation has run once. https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java (right): https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:89: android.R.interpolator.fast_out_slow_in); On 2015/09/03 05:46:56, Yusuf wrote: > there is probably a bakedBezierinterpolator we should be using for this per > material design spec. see other interpolators toolbar uses. Done. https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:94: * How does the title animation work? On 2015/09/03 05:46:56, Yusuf wrote: > This seems to be better suited to the class definition javadoc Acknowledged. https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:106: if (!mShouldRunTitleAnimation) return; On 2015/09/03 05:46:56, Yusuf wrote: > does this get reset on every page load? what happens when use clicks a link? Currently the animation will be started only when we load the very first webpage. The flag is only set true in prepareTitleAnim(), which is called in CustomTabToolbar#setShouldShowTitle(), and it is called for only one time during initialization.
On 2015/09/03 22:25:23, Ian Wen wrote: > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java > (right): > > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:203: > if (mShouldShowTitle && !TextUtils.equals(currentTab.getTitle(), > currentTab.getUrl())) { > On 2015/09/03 05:46:56, Yusuf wrote: > > the title can update more than once per page. We should only play the > animation > > once per page. > > startTitleAnimation() will do nothing if the title animation has run once. > > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java > (right): > > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:89: > android.R.interpolator.fast_out_slow_in); > On 2015/09/03 05:46:56, Yusuf wrote: > > there is probably a bakedBezierinterpolator we should be using for this per > > material design spec. see other interpolators toolbar uses. > > Done. > > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:94: > * How does the title animation work? > On 2015/09/03 05:46:56, Yusuf wrote: > > This seems to be better suited to the class definition javadoc > > Acknowledged. > > https://codereview.chromium.org/1329793002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbarAnimationDelegate.java:106: > if (!mShouldRunTitleAnimation) return; > On 2015/09/03 05:46:56, Yusuf wrote: > > does this get reset on every page load? what happens when use clicks a link? > > Currently the animation will be started only when we load the very first > webpage. The flag is only set true in prepareTitleAnim(), which is called in > CustomTabToolbar#setShouldShowTitle(), and it is called for only one time during > initialization. Do you mind sending a video of this to the bug on initial page load and when a user clicks on the a link?
Done. Updated the bug.
lgtm
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329793002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329793002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5afa2934ff3810486b77982d4a815763a7b17fdd Cr-Commit-Position: refs/heads/master@{#348257}
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5afa2934ff3810486b77982d4a815763a7b17fdd Cr-Commit-Position: refs/heads/master@{#348257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
