970719 - Animate progress bar to end of the screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=962106#c0:

* Animate the progress bar all the way to the end of the screen. Currently it just blinks off half way if it's done earlier. Adding a quick 'shoot across the screen' animation would make this feel more refined and also faster. (See the animations in the above link)

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Animate progress bar to end of the screen

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Keep an eye on pageload performance, both eideticker and phonedash.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8373777 [details] [diff] [review]
Animate progress bar to end of the screen

Review of attachment 8373777 [details] [diff] [review]:
-----------------------------------------------------------------

Wouldn't it simpler to just set progress to 100 when a STOP tab event is emitted, call animateProgress() as usual, and them have the HIDE message when it reaches MAX_PROGRESS? Just wondering why you needed a separate finishProgress() method to do this.

This looks good but I'd like to get some more background before giving r+.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +478,5 @@
>                      break;
>  
>                  case STOP:
> +                    flags.add(UpdateFlags.PROGRESS);
> +                    mProgressBar.finishProgress();

Why can't STOP be handled in the same way than the other tab events? In other words, progress is set to 100 in Tabs.java with a LOAD_PROGRESS_STOP constant?

::: mobile/android/base/toolbar/ToolbarProgressView.java
@@ +77,5 @@
> +
> +                        updateBounds();
> +
> +                        if (mCurrentProgress < mTargetProgress) {
> +                            final int delay = msg.what == MSG_UPDATE ? DELAY : DELAY / 4;

Enclose the expression in ()'s i.e. (msg.what == ... ? ... : ...)

@@ +79,5 @@
> +
> +                        if (mCurrentProgress < mTargetProgress) {
> +                            final int delay = msg.what == MSG_UPDATE ? DELAY : DELAY / 4;
> +                            sendMessageDelayed(mHandler.obtainMessage(msg.what), delay);
> +                        } else if (mCurrentProgress == MAX_PROGRESS) {

Is (mCurrentProgress == MAX_PROGRESS) guaranteed to happen? Can't an inexact rounding on mIncrement cause (mCurrentProgress > MAX_PROGRESS) to happen?

@@ +141,5 @@
>          mHandler.sendEmptyMessage(MSG_UPDATE);
>      }
>  
> +    void finishProgress() {
> +        mCurrentProgress = mTargetProgress;

Not specific to this patch (animateProgress has the same code) but I wonder: won't this make the progress bar jump to the previously set mTargetProgress before animating to 100? Is this intentional? I'd expect overlapping animation requests to simply update the mTargetProgress and mIncrement and leave mCurrentProgress untouched.

Is it done this way to avoid having animations going backwards when, say, you switch tabs?

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Animate progress bar to end of the screen, v2

(In reply to Lucas Rocha (:lucasr) from comment #3)
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +478,5 @@
> >                      break;
> >  
> >                  case STOP:
> > +                    flags.add(UpdateFlags.PROGRESS);
> > +                    mProgressBar.finishProgress();
> 
> Why can't STOP be handled in the same way than the other tab events? In
> other words, progress is set to 100 in Tabs.java with a LOAD_PROGRESS_STOP
> constant?

Good point, changed.
 
> @@ +79,5 @@
> > +
> > +                        if (mCurrentProgress < mTargetProgress) {
> > +                            final int delay = msg.what == MSG_UPDATE ? DELAY : DELAY / 4;
> > +                            sendMessageDelayed(mHandler.obtainMessage(msg.what), delay);
> > +                        } else if (mCurrentProgress == MAX_PROGRESS) {
> 
> Is (mCurrentProgress == MAX_PROGRESS) guaranteed to happen? Can't an inexact
> rounding on mIncrement cause (mCurrentProgress > MAX_PROGRESS) to happen?

No; we use Math.min() above, so mCurrentProgress will never exceed mTargetProgress.

> @@ +141,5 @@
> >          mHandler.sendEmptyMessage(MSG_UPDATE);
> >      }
> >  
> > +    void finishProgress() {
> > +        mCurrentProgress = mTargetProgress;
> 
> Not specific to this patch (animateProgress has the same code) but I wonder:
> won't this make the progress bar jump to the previously set mTargetProgress
> before animating to 100? Is this intentional? I'd expect overlapping
> animation requests to simply update the mTargetProgress and mIncrement and
> leave mCurrentProgress untouched.
> 
> Is it done this way to avoid having animations going backwards when, say,
> you switch tabs?

I didn't write this part, so I'm not sure what the intention was. Removed.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8374478 [details] [diff] [review]
Animate progress bar to end of the screen, v2

Review of attachment 8374478 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just need to understand why STOP needs to be handled differently than the other events that also update progress level based on the Tab load progress.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +479,5 @@
>  
>                  case STOP:
> +                    flags.add(UpdateFlags.PROGRESS);
> +                    if (mProgressBar.getVisibility() == View.VISIBLE) {
> +                        mProgressBar.animateProgress(Tabs.LOAD_PROGRESS_STOP);

Why does this need to be handled differently? i.e. tab.getLoadProgress() vs Tabs.LOAD_PROGRESS_STOP. I'd expect the code to handle STOP would be exactly the same as when handling, say, the LOADED event.

::: mobile/android/base/toolbar/ToolbarProgressView.java
@@ +34,5 @@
>   * bar also includes incremental animation between each step to improve
>   * perceived performance.
>   */
>  public class ToolbarProgressView extends ImageView {
> +    private static final int MAX_PROGRESS = 10000;

Sneaking some unrelated fixes in? :-)

@@ +126,5 @@
>       * @param progress Percentage (0-100) to which progress bar should be animated
>       */
>      void animateProgress(int progressPercentage) {
>          final int absoluteProgress = getAbsoluteProgress(progressPercentage);
> +        if (absoluteProgress <= mTargetProgress) {

Why is this necessary?

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Animate progress bar to end of the screen, v3

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Why does this need to be handled differently? i.e. tab.getLoadProgress() vs
> Tabs.LOAD_PROGRESS_STOP. I'd expect the code to handle STOP would be exactly
> the same as when handling, say, the LOADED event.

Sorry, some day I'll send you patches without all the silly churn!

> ::: mobile/android/base/toolbar/ToolbarProgressView.java
> 
> Sneaking some unrelated fixes in? :-)

The patch adds code that uses MAX_PROGRESS, so it's not *completely* unrelated :P

> @@ +126,5 @@
> >       * @param progress Percentage (0-100) to which progress bar should be animated
> >       */
> >      void animateProgress(int progressPercentage) {
> >          final int absoluteProgress = getAbsoluteProgress(progressPercentage);
> > +        if (absoluteProgress <= mTargetProgress) {
> 
> Why is this necessary?

After we manually click stop, we can still receive page load events (e.g., DOMContentLoaded). When that happens, the bar goes backwards and is stuck (since we won't get a STOP to clear it). So this check ensures that receiving these events after hitting stop won't break things.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8374910 [details] [diff] [review]
Animate progress bar to end of the screen, v3

Review of attachment 8374910 [details] [diff] [review]:
-----------------------------------------------------------------

A lot simpler, thanks.

::: mobile/android/base/toolbar/ToolbarProgressView.java
@@ +126,5 @@
>       * @param progress Percentage (0-100) to which progress bar should be animated
>       */
>      void animateProgress(int progressPercentage) {
>          final int absoluteProgress = getAbsoluteProgress(progressPercentage);
> +        if (absoluteProgress <= mTargetProgress) {

For future reference, could you add a comment explaining why this is necessary?

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> For future reference, could you add a comment explaining why this is
> necessary?

Done.

https://hg.mozilla.org/integration/fx-team/rev/be798bbb9163

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be798bbb9163

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8374910 [details] [diff] [review]
Animate progress bar to end of the screen, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962106
User impact if declined: progress bar disappears before reaching end of screen
Testing completed (on m-c, etc.):  m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe87bde35f74

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 29.0a2 (2014-02-17) and Firefox for Android 30.0a1 (2014-02-17)
Device: LG Nexus 4
OS: Android 4.4