Closed Bug 970719 Opened 6 years ago Closed 6 years ago

Animate progress bar to end of the screen

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
fennec 29+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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)
Attachment #8373777 - Flags: review?(lucasr.at.mozilla)
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+
(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+
(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+
(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
https://hg.mozilla.org/mozilla-central/rev/be798bbb9163
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
tracking-fennec: --- → ?
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+
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
tracking-fennec: ? → 29+
Depends on: 978874
You need to log in before you can comment on or make changes to this bug.