[Android] Add phone progress bar animation behind a flag.

[Android] Add phone progress bar animation behind a flag.

Add progress bar animation behind "progress-bar-animation" flag with
the following options:

- "disabled" (default)
- "smooth"
- "fast-start"

BUG=352839
Committed: https://crrev.com/5d14d9a3c4f02548a672837c591f6cea78918b24
Cr-Commit-Position: refs/heads/master@{#341472}

[Kibeom Kim (inactive), 2015-07-31 02:14:59]

kkimlabs@chromium.org changed reviewers:
+ dtrainor@chromium.org, tedchoc@chromium.org

[Kibeom Kim (inactive), 2015-07-31 02:15:00]

ptal. I'll measure page loading time and battery drain rate today or tomorrow.

[Kibeom Kim (inactive), 2015-07-31 10:36:37]

I'm running perf bots and just got the first result: blink_perf.svg on Nexus 5.
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-07...

Mostly equal but 1.x% regressions on few tests. Though, blink_perf.svg doesn't
represent a realistic usecase as it just loads local svgs, and I expect other
more realistic tests, page_cycler.top_10_mobile or page_cycler.typical_25 are
less sensitive to this change compared to blink_perf.svg test.

[Kibeom Kim (inactive), 2015-07-31 11:46:47]

page_cycler.top_10_mobile on Nexus 6
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-07...

Unfortunately, it doesn't look good, especially on N6 :/. I'll change this CL so
that the default flag is no animation.

[Kibeom Kim (inactive), 2015-07-31 19:16:48]

ptal. No animation by default.

[David Trainor- moved to gerrit, 2015-07-31 21:01:52]

lgtm

https://codereview.chromium.org/1256343004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ProgressAnimationFastStart.java
(right):

https://codereview.chromium.org/1256343004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ProgressAnimationFastStart.java:12:
private static final float NORMALIZED_INITIAL_SPEED = 1.5f;
Can we add units to these?  Are these in dp, percent?  Might help understand how
to tweak these in the future.  Same with the other file.

https://codereview.chromium.org/1256343004/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1256343004/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7056: +        <message
name="IDS_FLAGS_PROGRESS_BAR_ANIMATION_NAME" desc= "Name of the flag to
configure Android page loading progress bar animation.">
Remove spaces after desc=?  Looks like other flags around these lines don't have
it.

[Kibeom Kim (inactive), 2015-07-31 21:36:32]

kkimlabs@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Kibeom Kim (inactive), 2015-07-31 21:36:32]

rkaplow@ : ptal tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/1256343004/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1256343004/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7056: +        <message
name="IDS_FLAGS_PROGRESS_BAR_ANIMATION_NAME" desc= "Name of the flag to
configure Android page loading progress bar animation.">
On 2015/07/31 21:01:51, David Trainor wrote:
> Remove spaces after desc=?  Looks like other flags around these lines don't
have
> it.

Done.

[rkaplow, 2015-07-31 21:39:41]

lgtm

histograms lgtm

[Kibeom Kim (inactive), 2015-07-31 21:42:50]

https://codereview.chromium.org/1256343004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ProgressAnimationFastStart.java
(right):

https://codereview.chromium.org/1256343004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ProgressAnimationFastStart.java:12:
private static final float NORMALIZED_INITIAL_SPEED = 1.5f;
On 2015/07/31 21:01:51, David Trainor wrote:
> Can we add units to these?  Are these in dp, percent?  Might help understand
how
> to tweak these in the future.  Same with the other file.

This is a normalized scale, 0 to 1, and I wasn't sure the best way to express in
the name. So added comment above instead.

Done.

[Kibeom Kim (inactive), 2015-07-31 21:43:02]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-31 21:43:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1256343004/#ps80001
(title: "added unit comment and fixed spaces")

[commit-bot: I haz the power, 2015-07-31 21:44:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/80001

[commit-bot: I haz the power, 2015-07-31 21:56:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-31 21:56:52]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kibeom Kim (inactive), 2015-07-31 21:58:07]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-31 21:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1256343004/#ps100001
(title: "fix compile error on non-Android platforms by ifdefing
kProgressBarAnimationChoices")

[commit-bot: I haz the power, 2015-07-31 21:58:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/100001

[Kibeom Kim (inactive), 2015-07-31 23:48:11]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-07-31 23:48:14]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-07-31 23:48:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/100001

[commit-bot: I haz the power, 2015-08-01 04:00:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-01 04:00:02]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Kibeom Kim (inactive), 2015-08-01 06:59:12]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-08-01 06:59:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/100001

[Kibeom Kim (inactive), 2015-08-01 08:55:00]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-08-01 08:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1256343004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-01 08:55:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/120001

[commit-bot: I haz the power, 2015-08-01 14:55:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-01 14:55:35]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Kibeom Kim (inactive), 2015-08-01 18:28:15]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-08-01 18:28:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/120001

[Kibeom Kim (inactive), 2015-08-01 21:54:51]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-08-01 21:54:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1256343004/#ps140001
(title: "telemetry_perf_unittest fix attempt #1")

[commit-bot: I haz the power, 2015-08-01 21:54:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256343004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256343004/140001

[commit-bot: I haz the power, 2015-08-01 23:17:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-01 23:18:18]

Patchset 8 (id:??) landed as
https://crrev.com/5d14d9a3c4f02548a672837c591f6cea78918b24
Cr-Commit-Position: refs/heads/master@{#341472}

[Kibeom Kim (inactive), 2015-08-03 18:47:59]

OK it seems this crashes on tablet native initializaiton (and upstream doesn't
have tablet bot). Fix should be simple will fix asap

[jdduke (slow), 2015-08-03 23:35:35]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-08-03 23:35:35]

https://codereview.chromium.org/1256343004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1256343004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:127:
updateVisibleProgress();
Silly ignorant question: Is finish() allowed to be called multiple times
redundantly? If so we might get into a scenario where the hide callback is
posted a ton of times. I have no idea if this happens in practice. There are
other places that call setProgress too, basically I'm wondering if there's any
way we can safeguarding against posting a ton of hide runnables? Also, why not
postOnAnimationDelayed?

[Kibeom Kim (inactive), 2015-08-04 02:08:45]

https://codereview.chromium.org/1256343004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java
(right):

https://codereview.chromium.org/1256343004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/ToolbarProgressBar.java:127:
updateVisibleProgress();
On 2015/08/03 23:35:35, jdduke wrote:
> Silly ignorant question: Is finish() allowed to be called multiple times
> redundantly? If so we might get into a scenario where the hide callback is
> posted a ton of times. I have no idea if this happens in practice. There are
> other places that call setProgress too, basically I'm wondering if there's any
> way we can safeguarding against posting a ton of hide runnables? Also, why not
> postOnAnimationDelayed?

Yeah currently finish() can be called multiple times aftert start(), though
normally it will be called only once. The list of events that calls finish():

- onCrash
- onPageLoadFailed
- onPageLoadFinished
- onPageLoadStarted (if it's native page)
- ToolbarManager#updateCurrentTabDisplayStatus()

So I don't think we're spamming, and actually only onPageLoadFinished will call
finished() with "delayed==true" argument, so definitely we're not spamming
posting tasks at least. I could have just guarded by |if (!mIsStarted) return;|
but didn't feel it's worth when I was writing this.

And yeah I think I should have used postOnAnimationDelayed. Thanks for pointing
out, I'll prepare CL shortly.