Animate progress bar to end of the screen

VERIFIED FIXED in Firefox 29

Status

()

Firefox for Android
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 30
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, fennec29+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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)
(Assignee)

Comment 1

4 years ago
Created attachment 8373777 [details] [diff] [review]
Animate progress bar to end of the screen
Attachment #8373777 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Keep an eye on pageload performance, both eideticker and phonedash.
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?
Attachment #8373777 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 8374478 [details] [diff] [review]
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.
Attachment #8373777 - Attachment is obsolete: true
Attachment #8374478 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8374478 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8374910 [details] [diff] [review]
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.
Attachment #8374478 - Attachment is obsolete: true
Attachment #8374910 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8374910 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 8

4 years ago
(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
(Assignee)

Updated

4 years ago
status-firefox29: --- → affected
status-firefox30: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/be798bbb9163
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(Assignee)

Updated

4 years ago
tracking-fennec: --- → ?
(Assignee)

Comment 10

4 years ago
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
Attachment #8374910 - Flags: approval-mozilla-aurora?
Attachment #8374910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe87bde35f74
status-firefox29: affected → fixed
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
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
tracking-fennec: ? → 29+
(Assignee)

Updated

4 years ago
Depends on: 978874
You need to log in before you can comment on or make changes to this bug.