Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1389293003:
Add 200MS delay before starting toolbar scale animation (Closed)
DescriptionAdd 200MS delay before starting toolbar scale animation
Title scale-down animation might happen at a too early stage, making
users confused about what they see. This CL adds a 200MS delay between
toolbar initialization and the time we decide to start to animate.
BUG=505620
Committed: https://crrev.com/36f275146662f6067e34695a97e98bd146c47379
Cr-Commit-Position: refs/heads/master@{#358862}
Committed: https://crrev.com/7ee8a0a8577c594f185335874c0ceb24a5038973
Cr-Commit-Position: refs/heads/master@{#358959}
Patch Set 1 #
Total comments: 4
Patch Set 2 : respond to comment #
Total comments: 3
Patch Set 3 : Revert the change in base folder #Messages
Total messages: 29 (9 generated)
ianwen@chromium.org changed reviewers: + yusufo@chromium.org
PTAL
ping?
https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:219: long duration = System.currentTimeMillis() - mFirstTitleUpdateTime; what happens if the title hasn't updated yet? https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:223: new Handler().postDelayed(mTitleAnimationStarter, TITLE_ANIM_DELAY_MS - duration); post this to UIThread with ThreadUtils.runOnUIThreadDelayed
https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:219: long duration = System.currentTimeMillis() - mFirstTitleUpdateTime; On 2015/10/12 18:08:12, Yusuf wrote: > what happens if the title hasn't updated yet? mFirstTitleUpdateTime is set when initialize() is called, which is guaranteed to happen, isn't it? https://chromiumcodereview.appspot.com/1389293003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:223: new Handler().postDelayed(mTitleAnimationStarter, TITLE_ANIM_DELAY_MS - duration); On 2015/10/12 18:08:13, Yusuf wrote: > post this to UIThread with ThreadUtils.runOnUIThreadDelayed Done.
lgtm
ianwen@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, could you ptal at the one line change in ThreadUtils?
https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (left): https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:183: @VisibleForTesting This is used in tests which proguard would remove or rename in release mode - why do you think it's ok to remove? Or put another way - it's already referenced in chrome app (e.g. ScreenOrientationListener.java) - is that why you thought it's ok to remove? Maybe I just don't understand proguard :S
https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (left): https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:183: @VisibleForTesting On 2015/11/02 21:51:12, Yaron wrote: > This is used in tests which proguard would remove or rename in release mode - > why do you think it's ok to remove? > Or put another way - it's already referenced in chrome app (e.g. > ScreenOrientationListener.java) - is that why you thought it's ok to remove? > > Maybe I just don't understand proguard :S Weird. If this method is already used by other classes for none testing purpose, "VisibleForTesting" tag should have already been removed. IIRC "VisibleForTesting" is a way for the testing classes to have higher access rights to other classes. I am removing the annotation in this CL because I am to call this method from CustomTabToolbar. I thought I was the first person to do it, but apparently others have violated the annotation before.
On 2015/11/02 22:26:32, Ian Wen wrote: > https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... > File base/android/java/src/org/chromium/base/ThreadUtils.java (left): > > https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... > base/android/java/src/org/chromium/base/ThreadUtils.java:183: @VisibleForTesting > On 2015/11/02 21:51:12, Yaron wrote: > > This is used in tests which proguard would remove or rename in release mode - > > why do you think it's ok to remove? > > Or put another way - it's already referenced in chrome app (e.g. > > ScreenOrientationListener.java) - is that why you thought it's ok to remove? > > > > Maybe I just don't understand proguard :S > > Weird. If this method is already used by other classes for none testing purpose, > "VisibleForTesting" tag should have already been removed. IIRC > "VisibleForTesting" is a way for the testing classes to have higher access > rights to other classes. > > I am removing the annotation in this CL because I am to call this method from > CustomTabToolbar. I thought I was the first person to do it, but apparently > others have violated the annotation before. Got it - I see what you were getting at. It's also overloaded for proguard: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... Do release tests work with this removed? If so, then ok to remove, otherwise you'll have to keep it in. In either case lgtm once you sort it out
The CQ bit was checked by ianwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1389293003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1389293003/20001
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 ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1389293003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1389293003/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/36f275146662f6067e34695a97e98bd146c47379 Cr-Commit-Position: refs/heads/master@{#358862}
Message was sent while issue was closed.
robliao@chromium.org changed reviewers: + robliao@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (left): https://codereview.chromium.org/1389293003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:183: @VisibleForTesting On 2015/11/02 22:26:32, Ian Wen wrote: > On 2015/11/02 21:51:12, Yaron wrote: > > This is used in tests which proguard would remove or rename in release mode - > > why do you think it's ok to remove? > > Or put another way - it's already referenced in chrome app (e.g. > > ScreenOrientationListener.java) - is that why you thought it's ok to remove? > > > > Maybe I just don't understand proguard :S > > Weird. If this method is already used by other classes for none testing purpose, > "VisibleForTesting" tag should have already been removed. IIRC > "VisibleForTesting" is a way for the testing classes to have higher access > rights to other classes. > > I am removing the annotation in this CL because I am to call this method from > CustomTabToolbar. I thought I was the first person to do it, but apparently > others have violated the annotation before. Breaks the Android Builder [17814/24986] ACTION Packaging ui_android_unittests Warning: org.chromium.test.reporter.TestStatusReporter: can't find referenced method 'void postOnUiThreadDelayed(java.lang.Runnable,long)' in class org.chromium.base.ThreadUtils Warning: org.chromium.test.reporter.TestStatusReporter$1: can't find referenced method 'void postOnUiThreadDelayed(java.lang.Runnable,long)' in class org.chromium.base.ThreadUtils Warning: there were 2 unresolved references to program class members. Your input classes appear to be inconsistent. You may need to recompile them and try again. Alternatively, you may have to specify the option '-dontskipnonpubliclibraryclassmembers'. Error: Please correct the above warnings first. https://build.chromium.org/p/chromium/builders/Android/builds/48738
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1438583003/ by robliao@chromium.org. The reason for reverting is: Breaks the Android Builder [17814/24986] ACTION Packaging ui_android_unittests Warning: org.chromium.test.reporter.TestStatusReporter: can't find referenced method 'void postOnUiThreadDelayed(java.lang.Runnable,long)' in class org.chromium.base.ThreadUtils Warning: org.chromium.test.reporter.TestStatusReporter$1: can't find referenced method 'void postOnUiThreadDelayed(java.lang.Runnable,long)' in class org.chromium.base.ThreadUtils Warning: there were 2 unresolved references to program class members. Your input classes appear to be inconsistent. You may need to recompile them and try again. Alternatively, you may have to specify the option '-dontskipnonpubliclibraryclassmembers'. Error: Please correct the above warnings first. https://build.chromium.org/p/chromium/builders/Android/builds/48738.
Message was sent while issue was closed.
Description was changed from ========== Add 200MS delay before starting toolbar scale animation Title scale-down animation might happen at a too early stage, making users confused about what they see. This CL adds a 200MS delay between toolbar initialization and the time we decide to start to animate. BUG=505620 Committed: https://crrev.com/36f275146662f6067e34695a97e98bd146c47379 Cr-Commit-Position: refs/heads/master@{#358862} ========== to ========== Add 200MS delay before starting toolbar scale animation Title scale-down animation might happen at a too early stage, making users confused about what they see. This CL adds a 200MS delay between toolbar initialization and the time we decide to start to animate. BUG=505620 Committed: https://crrev.com/36f275146662f6067e34695a97e98bd146c47379 Cr-Commit-Position: refs/heads/master@{#358862} ==========
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1389293003/#ps40001 (title: "Revert the change in base folder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1389293003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1389293003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7ee8a0a8577c594f185335874c0ceb24a5038973 Cr-Commit-Position: refs/heads/master@{#358959} |
