Closed Bug 917896 Opened 6 years ago Closed 6 years ago

Replace progress throbber with a progress bar

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox27 --- affected
firefox28 --- affected
firefox29 --- verified
relnote-firefox --- 29+
fennec 29+ ---

People

(Reporter: lucasr, Assigned: bnicholson)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(1 file, 7 obsolete files)

This is about improving our perceived pageload performance. Progressbars tends to give a better sense of progress than a spinner.

Just to be clear: this progress bar is not meant to be tracking the actual page loading progress as we know this causes pageload regressions.
Blocks: 918851
Based on Traceview profiles, a lot of time is taken in HardwareRenderer$GlRenderer.draw during a pageload. I see about 25% of time (1597ms) used. I thought this was due to the web content being painted, but Snorp told me that HardwareRenderer would only be used for Android UI widgets, not Gecko painting. He suggested it was the spinner/throbber, so I removed it and re-profiled. Sure enough, it was the spinner/throbber.

Removing the spinner reduced the HardwareRenderer$GlRenderer.draw time to 1.4% (196ms).
Blocks: 947390
Attached patch Progress bar v0.1 (obsolete) — Splinter Review
Here is a simple conversion from a spinner to a progress bar. I copied the approach the Android stock browser (and Chrome by the look of it) and do not use a real ProgressBar widget. This is an ImageView with a pseudo-animation system built into it. We do not want a real animation since it will cause too much rendering.

This patch feels OK, but we need to move the progress bar to lay on the transition from BrowserToolbar to web content, like Stock and Chrome. I figure there are a few ways to do it and wanted some feedback.
Attachment #8355541 - Flags: feedback?(wjohnston)
Attachment #8355541 - Flags: feedback?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #0)

> Just to be clear: this progress bar is not meant to be tracking the actual
> page loading progress as we know this causes pageload regressions.

In the WIP patch, I use 4 progress "points":
1. On NetworkStart: 20%
2. On LocationChange: 60%
3. On DOMContentLoaded: 80%
4. On NetworkStop: 100%

Because of the pseudo-animation, the transitions between points is somewhat smooth. We can tweak these points, but I don't know that we can add many more. Maybe "On FirstPaint"?
Who are you and what have you done with mfinkle.
Attached patch Progress bar v0.2 (obsolete) — Splinter Review
I tried to clean up BrowserToolbar.onTabChanged to make it a bit clearer how the progress points were structured.

I also removed a call to BrowserToolbar.setProgressVisibility(true) in BrowserApp.openUrlAndStopEditing:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1385

This was originally added to show the spinner as soon as possible. Because we can actually be in the process of loading a page when this call was made, the progressbar could show the current loading progress (almost finished) and then jump back to the beginning of loading the new page. A spinner did not have this problem.
Attachment #8355541 - Attachment is obsolete: true
Attachment #8355541 - Flags: feedback?(wjohnston)
Attachment #8355541 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8355662 - Flags: feedback?(wjohnston)
Attachment #8355662 - Flags: feedback?(lucasr.at.mozilla)
(In reply to Wesley Johnston (:wesj) from comment #5)
> Who are you and what have you done with mfinkle.

LOL. I like this approach because it kills the spinner, reduces rendering during pageload, but does not use Gecko progress events. Win, Win, Win.
Comment on attachment 8355662 [details] [diff] [review]
Progress bar v0.2

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

Drive-by.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +591,5 @@
> +                    setProgress(80);
> +                    break;
> +                case STOP:
> +                    setProgressVisibility(false);
> +                    setProgress(100);

The second line is redundant:

 public void setProgress(int progress) {
   if (mProgress.getVisibility() == View.VISIBLE) {
     mProgress.setProgress(progress * PageProgressView.MAX_PROGRESS / 100);
   }
 }

Did you mean to invert these two lines? Or just kill the setProgressVisibility call -- you're using auto-hide anyway.

@@ +793,5 @@
>          // The "Throbber start" and "Throbber stop" log messages in this method
>          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (visible) {
> +            mFavicon.setImageResource(R.drawable.favicon);

There might be some simplification we can do here now that the favicon has only two states (R.d.f or the real favicon), rather than also being the spinner.

Also worth checking if you'll be fixing Bug 936857.

::: mobile/android/base/toolbar/PageProgressView.java
@@ +1,1 @@
> +

Kill blank line.

@@ +12,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

... and put it here :)

@@ +87,5 @@
> +                    invalidate();
> +                    if (mCurrentProgress < mTargetProgress) {
> +                        sendMessageDelayed(mHandler.obtainMessage(MSG_UPDATE), DELAY);
> +                    }
> +                    

ws.

@@ +89,5 @@
> +                        sendMessageDelayed(mHandler.obtainMessage(MSG_UPDATE), DELAY);
> +                    }
> +                    
> +                    if (mHideOnComplete && mCurrentProgress == MAX_PROGRESS) {
> +                        Log.i("PageProgressView", "Auto hide");

Might want to kill this.

@@ +125,5 @@
> +    }
> +
> +    @Override
> +    public void onDraw(Canvas canvas) {
> +//        super.onDraw(canvas);

*eyebrows*
(In reply to Richard Newman [:rnewman] from comment #8)

> ::: mobile/android/base/toolbar/BrowserToolbar.java

> > +                    setProgressVisibility(false);
> > +                    setProgress(100);
> 
> The second line is redundant:
> 
>  public void setProgress(int progress) {
>    if (mProgress.getVisibility() == View.VISIBLE) {
>      mProgress.setProgress(progress * PageProgressView.MAX_PROGRESS / 100);
>    }
>  }
> 
> Did you mean to invert these two lines? Or just kill the
> setProgressVisibility call -- you're using auto-hide anyway.

setProgressVisibility does not hide mProgress, but still does other things. Calling setProgress(100) will eventually hide mProgress, thanks to auto-hide.

> There might be some simplification we can do here now that the favicon has
> only two states (R.d.f or the real favicon), rather than also being the
> spinner.
> 
> Also worth checking if you'll be fixing Bug 936857.

Yes to both, but I want to keep this bug fairly well scoped.

> ::: mobile/android/base/toolbar/PageProgressView.java

> > +
> 
> Kill blank line.
> 
> > + * limitations under the License.
> > + */
> 
> ... and put it here :)
> 
> > +                        sendMessageDelayed(mHandler.obtainMessage(MSG_UPDATE), DELAY);
> > +                    }
> > +                    
> 
> ws.

Just copied from original source

> > +                    if (mHideOnComplete && mCurrentProgress == MAX_PROGRESS) {
> > +                        Log.i("PageProgressView", "Auto hide");
> 
> Might want to kill this.

Yeah, it's debug

> > +    public void onDraw(Canvas canvas) {
> > +//        super.onDraw(canvas);
> 
> *eyebrows*

It is in the original Android source code. I think they want to show that we explicitly *don't* want to call the super.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Created attachment 8355662 [details] [diff] [review]
> Progress bar v0.2
> 
> I tried to clean up BrowserToolbar.onTabChanged to make it a bit clearer how
> the progress points were structured.
> 
> I also removed a call to BrowserToolbar.setProgressVisibility(true) in
> BrowserApp.openUrlAndStopEditing:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#1385
> 
> This was originally added to show the spinner as soon as possible. Because
> we can actually be in the process of loading a page when this call was made,
> the progressbar could show the current loading progress (almost finished)
> and then jump back to the beginning of loading the new page. A spinner did
> not have this problem.

FYI: I removed this call in my patches for bug 944533.
Comment on attachment 8355662 [details] [diff] [review]
Progress bar v0.2

Some feedback from just testing the APK.

The main problem I see with this approach is that there's no sense of continuous progress due to the fact that the bar simply snaps to different levels at each stage (network start, location change, ...). I wonder if the HardwareRenderer perf issue is caused by the fact that we're using Animation (with a hw layer on the view) to spin the throbber. In theory, this should be quite fast.

The design I had in mind was something like this:

NetworkStart: max speed / blue glow bar / until 40%
LocationChange: medium speed / blue glow bar / until 70%
DomContentLoaded: slow speed / blue glow bar / until 100%
FirstPaint: slow speed / dark gray glow bar / until 100%
NetworkStop: restore progress bar to light gray

The idea here is that the progress bar moves fast in the beginning of the page load and gradually slows down so that it moves "infinitely" until the page is loaded. Furthermore, it would be nice to experiment with "de-emphasizing" the progress bar after FirstPaint (change it to dark gray glow or something) but still show it until the page is loaded.

I do realize that keeping the progress bar continuously growing might cause the very same performance issues than the throbber. But I'd prefer to get the UX right first and make perf compromises based on it. For instance, we can try different approaches for redrawing the progress bar without causing too much redraw overhead.

In terms of implementation, I think the progress bar should replace what we have as the 'shadow' view right now. The progress bar would serve as the shadow while inactive.
Attachment #8355662 - Flags: feedback?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #11)

> The main problem I see with this approach is that there's no sense of
> continuous progress due to the fact that the bar simply snaps to different
> levels at each stage (network start, location change, ...). I wonder if the
> HardwareRenderer perf issue is caused by the fact that we're using Animation
> (with a hw layer on the view) to spin the throbber. In theory, this should
> be quite fast.

But it's not and we know it.

> The idea here is that the progress bar moves fast in the beginning of the
> page load and gradually slows down so that it moves "infinitely" until the
> page is loaded. Furthermore, it would be nice to experiment with
> "de-emphasizing" the progress bar after FirstPaint (change it to dark gray
> glow or something) but still show it until the page is loaded.

We can play with using a different DELAY in the PageProgressView message handler for diffent parts of the page progress. I do not want to use Animation at all.

> I do realize that keeping the progress bar continuously growing might cause
> the very same performance issues than the throbber. But I'd prefer to get
> the UX right first and make perf compromises based on it. For instance, we
> can try different approaches for redrawing the progress bar without causing
> too much redraw overhead.

I want to improve pageload times more than I want a pretty progressbar.

> In terms of implementation, I think the progress bar should replace what we
> have as the 'shadow' view right now. The progress bar would serve as the
> shadow while inactive.

I agree.
(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 8355662 [details] [diff] [review]
> Progress bar v0.2
> 
> Some feedback from just testing the APK.
> 
> The main problem I see with this approach is that there's no sense of
> continuous progress due to the fact that the bar simply snaps to different
> levels at each stage (network start, location change, ...). I wonder if the
> HardwareRenderer perf issue is caused by the fact that we're using Animation
> (with a hw layer on the view) to spin the throbber. In theory, this should
> be quite fast.
> 
> The design I had in mind was something like this:
> 
> NetworkStart: max speed / blue glow bar / until 40%
> LocationChange: medium speed / blue glow bar / until 70%
> DomContentLoaded: slow speed / blue glow bar / until 100%
> FirstPaint: slow speed / dark gray glow bar / until 100%
> NetworkStop: restore progress bar to light gray
> 
> The idea here is that the progress bar moves fast in the beginning of the
> page load and gradually slows down so that it moves "infinitely" until the
> page is loaded. Furthermore, it would be nice to experiment with
> "de-emphasizing" the progress bar after FirstPaint (change it to dark gray
> glow or something) but still show it until the page is loaded.
> 
> I do realize that keeping the progress bar continuously growing might cause
> the very same performance issues than the throbber. But I'd prefer to get
> the UX right first and make perf compromises based on it. For instance, we
> can try different approaches for redrawing the progress bar without causing
> too much redraw overhead.
> 
> In terms of implementation, I think the progress bar should replace what we
> have as the 'shadow' view right now. The progress bar would serve as the
> shadow while inactive.

Lucas, the animation you are describing is actually the other way around from the designs we were exploring earlier in the year, where the progress bar starts off moving slowly for about 25% of the way, and then quickly shoots the rest of the way across the screen, which increases the users' perception of page load performance. See https://bugzilla.mozilla.org/show_bug.cgi?id=867633#c1 for some animated mockups around this. 

Now, the tricky part here is that with an animation like that (start slow, end fast), we may need to adjust our definition of what a "completely loaded page" is for the user. I wonder if we can pick up some of the work Mark was exploring last year when he turned off the throbber earlier in the load cycle to make it feel like pages were appearing faster. The balance to strive for here is, how do we make the bar feel fast, and can we time it such that a page is usable enough to feel like it's fully loaded even if there may be some piece of content that is still loading somewhere.  

It would be great to see both approaches in action (yours and mine) to compare how they feel in practice. We've taken our time getting to this point, it's worth taking a little more to explore and get it right.  

The other feedback I have on Marks build is that we never want the progress bar to stop dead in its tracks, because it suddenly feels like nothing is happening. Even if it's moving very slowly at certain times, it's a preferable experience to what looks like a stalled bar. (It should also be orange, in a lower position but I'm more concerned about getting the movement right for now.)
(In reply to Ian Barlow (:ibarlow) from comment #13)

> The other feedback I have on Marks build is that we never want the progress
> bar to stop dead in its tracks, because it suddenly feels like nothing is
> happening. Even if it's moving very slowly at certain times, it's a
> preferable experience to what looks like a stalled bar. (It should also be
> orange, in a lower position but I'm more concerned about getting the
> movement right for now.)

As long as we focus on doing the "creeping bar" using non-Animation techniques. Animation appears to be the enemy of fast page loads. I don't even want to mix Animations and non-Animations approaches.
This is the phonedash pageload results performed while the throbber was disabled in Nightly:
http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-blank/2014-01-02/2014-01-04/notcached/noerrorbars/standarderror

Old phones show the largest improvements
Nexus One: ~22%
Nexus S: ~17%
Droid Pro: ~23%

Newer devices are in the 5% - 7% range.

This is just loading a blank page. Loading a local cached Twitter page shows similar results.
(In reply to Mark Finkle (:mfinkle) from comment #12)
> > The idea here is that the progress bar moves fast in the beginning of the
> > page load and gradually slows down so that it moves "infinitely" until the
> > page is loaded. Furthermore, it would be nice to experiment with
> > "de-emphasizing" the progress bar after FirstPaint (change it to dark gray
> > glow or something) but still show it until the page is loaded.
> 
> We can play with using a different DELAY in the PageProgressView message
> handler for diffent parts of the page progress. I do not want to use
> Animation at all.

After playing a bit with Chrome for Android, I'm more inclined to get this landed without the continuous progress feedback (the "animated" progress bar approach ibarlow and I suggested). The only thing I'd suggest for now is to make it so that the progress bar never gets stalled for more than X seconds (5 maybe?) to at least give a sense of a "still trying" state.

We can work on the smoother progress bar feedback in a follow-up and (probably) only enable it for higher-end devices if doesn't cause any perf issues.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
(In reply to Ian Barlow (:ibarlow) from comment #13)
> Now, the tricky part here is that with an animation like that (start slow,
> end fast), we may need to adjust our definition of what a "completely loaded
> page" is for the user.

Even trickier than that is knowing where we are in the current step. Regardless of the threshold we decide to use for when the page is fully loaded, we don't know *when* that will happen. We have very little information -- just four points: 1) start, 2) location change, 3) document loaded, 4) stop. The time between any two of these points is indeterminate and can range from less than a second to over a minute (see cnn.com). So with a "slow start, fast end" approach, how do we know when the "start" ends and the "end" begins?

Some realistic scenarios:
Site  Step 1  Step 2  Step 3  Step 4
A    1s       1s      5s      20s
B    1s       1s      20s     5s
C    1s       5s      5s      5s

If we try to set a generic rule (e.g., start the fast animation after step 3 is over), it won't work consistently (e.g., site A). We have no idea how long each step will take, so we also can't set rules like "start the fast animation 5 seconds before step 4 finishes". All we know is a) what's happened so far, and b) how many steps are left. We could likely improve accuracy by tracking individual resources instead of measuring them as a group, which Chrome seems to do; however, several people have tried this already and have said that there's a noticeable performance hit, so that probably won't be an option.

> The other feedback I have on Marks build is that we never want the progress
> bar to stop dead in its tracks, because it suddenly feels like nothing is
> happening. Even if it's moving very slowly at certain times, it's a
> preferable experience to what looks like a stalled bar. (It should also be
> orange, in a lower position but I'm more concerned about getting the
> movement right for now.)

Unless we're able to increase the number of steps, the best we can do is animate the bar between each step to give the illusion of progress, which Mark's patch does. But, for the reasons described above, that's still guesswork. Let's say there's 1cm between step 3 and step 4; do we animate in 10 frames, 1mm each? How long should the delay be between each frame? If we say 100ms between each frame, the animation will be perfect if step 3 takes exactly one second. But if step 3 takes 5 seconds, the animation will be frozen for 4 seconds. If step 3 takes 500ms, the animation will execute for 5/10 frames, and then the next step is jumped to.

Since we don't know how long each step will be, we don't know how to divide or time the animation. The slower the animation, the longer it will take until the animation is frozen, but if it's too slow, the bar will hardly appear to be moving at all. Using linear interpolation, we cannot prevent the animation from freezing altogether; if we adjust our animation to cover sites that take up to N seconds to load, it's possible we'll run into a site that takes N+1 seconds.

We could perhaps improve the algorithm a bit to use exponential decay instead of a linear one, so the animation will start fast and finish slowly (similar to what Lucas mentioned). But the longer the step, the slower the animation, and it will eventually appear to be frozen (even if it's technically moving very, very slow).

Hopefully that helps explains the barriers a bit; I'll post some APKs using different approaches for comparison.
(In reply to Brian Nicholson (:bnicholson) from comment #17)

Let's just keep some points in mind:
* Keep it simple. Let's not go crazy on the effort to stop the "stalled progressbar". 
* No Android Animations
* No nsIWebProgressListener.onProgressChange
* We can add a point or two, like FirstPaint

> We could perhaps improve the algorithm a bit to use exponential decay
> instead of a linear one, so the animation will start fast and finish slowly
> (similar to what Lucas mentioned). But the longer the step, the slower the
> animation, and it will eventually appear to be frozen (even if it's
> technically moving very, very slow).

This sounds about as complicated as I would like to go and might be worth a followup. Let's focus on getting the progressbar to replace/overlay the shadow, and land that first.

I don't want to spend too much time experimenting/tweaking before landing something that makes pageloads much faster on many devices.
Minor changes to Mark's patch, with progress.9.png recolored orange and resized so it can overlay the action bar shadow. There was some discussion about adding first paint as an additional point, but I didn't see any existing event for first paint (and didn't want to create a new one), so I omitted that change.

The main problem I found with this approach is that changing tabs while the progress bar is active results in unwanted behavior: switching to about:home shows the progress bar when it normally wouldn't be shown, and switching back to the loading tab doesn't show the progress bar for several seconds, then suddenly jumps to near the end. I don't hit this often unless loading certain sites like this, but older phones will be more susceptible. Fixing this would require remembering the progress per tab, so I'll save that fun for later.

I found a couple other minor peeves, but nothing worth blocking on:
* For one, calling setProgress(100) fully animates all steps between point 3 and 4, even though the page is fully loaded at that point; this adds a few hundred ms to the perceived load time. I tried hiding the progress bar immediately when calling setProgress(100), but that feels weird since the progress bar never reaches the end.
* When a session restore is triggered, there are several seconds of no progress shown at startup since we're still waiting for Gecko to load and for the initial start message, so the browser feels frozen. Again, this will be more noticeable on older devices. Previously, we started a spinning throbber immediately for new pages without waiting for Gecko; we might want to consider doing something similar for this in a follow-up.

Test build: https://dl.dropboxusercontent.com/u/35559547/fennec/progressbar1.apk
Attachment #8355662 - Attachment is obsolete: true
Attachment #8355662 - Flags: feedback?(wjohnston)
Attachment #8357012 - Flags: review?(lucasr.at.mozilla)
(In reply to Brian Nicholson (:bnicholson) from comment #17)
> We could perhaps improve the algorithm a bit to use exponential decay
> instead of a linear one, so the animation will start fast and finish slowly
> (similar to what Lucas mentioned). But the longer the step, the slower the
> animation, and it will eventually appear to be frozen (even if it's
> technically moving very, very slow).

I don't follow everything you're writing (I think just Any thing that attempts to say "well hit 2 cm exactly when the page location change event hits" isn't going to work?). My plan here was always just animate the bar at some constant "rate" that was something like x% of the remaining progress / second. That would never hit the end of the bar (assuming x was less than 100), but would always move "forward". Whether it weights toward the start or end depends on what we choose for a value of x.

Eventually it would move very slowly for a very long loading pages, but a page that's taken more than 10-20 seconds and still hasn't painted a significant amount of content is likely going to feel slow regardless of what we do.
Comment on attachment 8357012 [details] [diff] [review]
Replace throbber with progress bar

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

Tried the apk. I like it, let's land this approach and iterate on follow-ups. The patch itself needs some tweaks. It will need a rebase to account for the changes from bug 944533. Please, have mercy and let me land it first. I've had to deal with enough rebase conflicts already :-)

::: mobile/android/base/BrowserApp.java
@@ -1381,5 @@
>          openUrlAndStopEditing(url, searchEngine, false);
>      }
>  
>      private void openUrlAndStopEditing(String url, String searchEngine, boolean newTab) {
> -        mBrowserToolbar.setProgressVisibility(true);

This is gone in bug 944533.

::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml
@@ +152,5 @@
>                 android:layout_alignParentBottom="true"
>                 android:background="@color/url_bar_shadow"
>                 android:contentDescription="@null"/>
>  
> +    <org.mozilla.gecko.toolbar.PageProgressView android:id="@+id/progress"

It would be nice if we simply *replaced* the shadow view with the progress view instead. Just set the background of PageProgressView to be @color/url_bar_shadow and make it draw nothing when progress either 0 or 100. Definitely something worth trying.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +797,1 @@
>              mLastFavicon = null;

I think you can remove all this favicon mangling now that the favicon view is just for favicon?

::: mobile/android/base/toolbar/PageProgressView.java
@@ +33,5 @@
> + * Because we're given limited information about the page load progress, the
> + * bar also includes incremental animation between each step to improve
> + * perceived performance.
> + */
> +public class PageProgressView extends ImageView {

Rename to ToolbarProgressView for consistency.

@@ +63,5 @@
> +
> +    private void init(Context ctx) {
> +        mBounds = new Rect(0,0,0,0);
> +        mCurrentProgress = 0;
> +        mTargetProgress = 0;

nit: add empty line here.

@@ +69,5 @@
> +            @Override
> +            public void handleMessage(Message msg) {
> +                if (msg.what == MSG_UPDATE) {
> +                    mCurrentProgress = Math.min(mTargetProgress, mCurrentProgress + mIncrement);
> +                    mBounds.right = getWidth() * mCurrentProgress / MAX_PROGRESS;

nit: empty line here.

@@ +70,5 @@
> +            public void handleMessage(Message msg) {
> +                if (msg.what == MSG_UPDATE) {
> +                    mCurrentProgress = Math.min(mTargetProgress, mCurrentProgress + mIncrement);
> +                    mBounds.right = getWidth() * mCurrentProgress / MAX_PROGRESS;
> +                    invalidate();

nit: empty line here.

@@ +101,5 @@
> +        }
> +
> +        mCurrentProgress = mTargetProgress;
> +        mTargetProgress = progress;
> +        mIncrement = (mTargetProgress - mCurrentProgress) / STEPS;

nit: add empty line here.

@@ +108,5 @@
> +    }
> +
> +    @Override
> +    public void onDraw(Canvas canvas) {
> +        Drawable d = getDrawable();

final
Attachment #8357012 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 8357012 [details] [diff] [review]
Replace throbber with progress bar

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

One more thing: you can probably remove this setLayerType() call now: 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java#296

There might be other animation-related assumptions around the favicon view that can/should be removed too. Please double check.
(In reply to Wesley Johnston (:wesj) from comment #20)
> My plan here was always just animate the bar at
> some constant "rate" that was something like x% of the remaining progress /
> second. That would never hit the end of the bar (assuming x was less than
> 100), but would always move "forward". Whether it weights toward the start
> or end depends on what we choose for a value of x.

It sounds like we're talking about the same thing (maybe "exponential decay" was the wrong term?). I think there are two things we can "decay": either the distance each animation frame is covering, or the speed between each frame.

E.g., using a "decay" of 10% for distance, where the formula is progress + (target - progress) / 10, the bar would move 10 units, 9 units, 8.1 units, etc. Alternatively, we could have the bar move the same amount every time, but then the delay increases at each frame (I think this is what you're referring to?).

> I don't follow everything you're writing (I think just Any thing that
> attempts to say "well hit 2 cm exactly when the page location change event
> hits" isn't going to work?).

I know that won't work -- that was what I was trying to explain :)

The reason I was explaining this is that speeding up toward the end of the animation (start slow, end fast suggested by Ian) can't really work since we don't know when we're about to finish. Speeding up at some arbitrary point during the animation will just end up abruptly freezing at the end, unless we're able to time it so that the animation finishes when the current step finishes (which isn't possible).

The only workaround I can think of is adding additional frames to finish the animation once the event has completed, but that would probably add to the perceived load time.

> Eventually it would move very slowly for a very long loading pages, but a
> page that's taken more than 10-20 seconds and still hasn't painted a
> significant amount of content is likely going to feel slow regardless of
> what we do.

Right, I think we're arguing the same thing again -- given a sufficiently long page load time, a frozen (or near frozen) progress bar is inevitable.
(In reply to Brian Nicholson (:bnicholson) from comment #23)
> (In reply to Wesley Johnston (:wesj) from comment #20)
> > My plan here was always just animate the bar at
> > some constant "rate" that was something like x% of the remaining progress /
> > second. That would never hit the end of the bar (assuming x was less than
> > 100), but would always move "forward". Whether it weights toward the start
> > or end depends on what we choose for a value of x.
> 
> It sounds like we're talking about the same thing (maybe "exponential decay"
> was the wrong term?). I think there are two things we can "decay": either
> the distance each animation frame is covering, or the speed between each
> frame.

Yeah, I usually picture decay differently, but dp(t)/dt = (1-p(t))*x would give you some e^-t*x term in the result. My plan was to layer a really slow version of that on top of the normal progress events, which we're not using anymore :). We can probably find a better function that keeps this on the low end of the bar though...

Regardless, it sounds like we're moving away from worrying about this here.
(In reply to Lucas Rocha (:lucasr) from comment #21)
> It would be nice if we simply *replaced* the shadow view with the progress
> view instead. Just set the background of PageProgressView to be
> @color/url_bar_shadow and make it draw nothing when progress either 0 or
> 100. Definitely something worth trying.

While this would probably work, I'm not sure why it would be better. We would have to do more work to hide/show the shadow, and it requires another attribute on the progress bar. What would the advantages be?
Oops, I understood what you really meant right after clicking submit. By "replace", you don't mean dynamically replace when showing the progress bar, you mean removing the shadow view altogether. I'll give that a try.
Noting more issues I found testing this patch:

* Clicking the URL bar while the progress bar is visible shows the progress bar in editing mode
* Reloading a page while the progress bar is visible makes the stop button disappear
(In reply to Lucas Rocha (:lucasr) from comment #21)

> Tried the apk. I like it, let's land this approach and iterate on
> follow-ups. 

+1 

I'll reserve my design comments until we land. ;)
Here's an updated patch with most of the issues from comment 19 and comment 27 fixed. Of those issues, the only remaining (and minor) one is the fact that we don't show any kind of progress UI at startup during a session restore.

(In reply to Lucas Rocha (:lucasr) from comment #21)
> It would be nice if we simply *replaced* the shadow view with the progress
> view instead. Just set the background of PageProgressView to be
> @color/url_bar_shadow and make it draw nothing when progress either 0 or
> 100. Definitely something worth trying.

Unfortunately, we won't be able to do this since we need to hide the progress bar (which would mean setting the progress to 0) when the home fragment is visible, yet the tab's progress still needs to be updated for when the home pager is closed. Explicitly hiding the progress bar when showing the home fragment allows us to update progress without having to show it.

> One more thing: you can probably remove this setLayerType() call now: 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/
> BrowserToolbar.java#296

Nice catch. I wasn't able to find any similar remnants like this.
Attachment #8357012 - Attachment is obsolete: true
Attachment #8358569 - Flags: review?(lucasr.at.mozilla)
Blocks: 918015
Comment on attachment 8358569 [details] [diff] [review]
Replace throbber with progress bar, v2

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

I like the direction but I feel we need to be more explicit about the conditions and invariants around the progress UI updates.

::: mobile/android/base/Tab.java
@@ +764,5 @@
>  
>      public boolean isPrivate() {
>          return false;
>      }
> +

A comment explaining the expected range for 'progress' would be useful here. Maybe throw or log a warning and/or clamp the value if the given 'progress' is out of range?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +504,5 @@
> +            setTabProgress(tab, 0);
> +            setProgressVisibility(tab.getState() == Tab.STATE_LOADING);
> +        }
> +
> +        // Update progress for each tab, regardless of whether the tab is

I guess you meant 'any' instead of 'each' here?

@@ +517,5 @@
> +                break;
> +            case LOAD_ERROR:
> +            case LOADED:
> +                setTabProgress(tab, 80);
> +                break;

It seems a bit backwards that the progress level is set by the UI (the toolbar, in this case). The UI shouldn't really need to know how the levels are set on each tab. For example, if, hypothetically, we come up with a very performant way to track real pageload progress with Gecko events, this UI code shouldn't really change much. So, with that in mind, I feel like this code should reside somewhere in Tab.java/Tabs.java and the setLoadProgress() should not be accessible outside the org.mozilla.gecko package.

@@ +636,5 @@
>                                      mActivity.getString(R.string.num_tabs, count) :
>                                      mActivity.getString(R.string.one_tab));
>      }
>  
> +    private void setProgressVisibility(boolean visible) {

I'd keep all the progress-related methods together.

@@ +644,5 @@
> +            mProgressBar.setProgress(selectedTab.getLoadProgress());
> +        }
> +
> +        mProgressBar.setVisibility(visible ? View.VISIBLE : View.GONE);
> +        updateDisplayLayout(selectedTab, EnumSet.of(UpdateFlags.PROGRESS));

This is redudant btw. ToolbarDisplayLayout will already update according to the tab state. Which takes me to my other concern: it feel that we should be more deliberate/explicit about keeping ToolbarDisplayLayout mode and progress bar visibility in sync i.e. the progress bar should be visible whenever ToolbarDisplayLayout is in PROGRESS mode and vice-versa.

Furthermore, we should also be more clear about the different BrowserToolbar states that lead to showing or hiding the progress bar i.e. the progress bar should only be visible if the currently selected tab is loading a page *and* the toolbar is not in edit mode.

With that in mind, here is my suggestion (which might need some tweaking):
Have a updateProgress(Tab tab) method that updates the ToolbarProgressView visibility and level. It first updates the visibility by checking mIsEditing and the given tab's state (state == STATE_LOADING) and the progress level if the progress bar is set to visible. Call this in startEditing(), stopEditing() and updateDisplayLayout() (if UpdateFlags.PROGRESS is set, to ensure ToolbarProgressView and ToolbarDisplayLayout are always in sync).

@@ +1028,5 @@
>          final String url = mUrlEditLayout.getText();
>          if (!isEditing()) {
>              return url;
>          }
> +

unrelated?

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +397,5 @@
>          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (mUiMode == UIMode.PROGRESS) {
>              mLastFavicon = null;
> +            mFavicon.setImageResource(R.drawable.favicon);

You don't need to do anything about favicons when updating UI progress state. This was only necessary because we were using the same view to show both the page's favicon and the throbber. You can safely remove both lines above.

@@ +402,5 @@
>  
>              Log.i(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - Throbber start");
>          } else {
>              updateFavicon(tab);
>              mFavicon.setAnimation(null);

Both these calls can be removed.

@@ +408,5 @@
>              Log.i(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - Throbber stop");
>          }
>  
>          updatePageActions(flags);
>      }

In summary, setUiMode() will now only:
1. Update mUiMode
2. Do the zerdatime logging
3. Call updatePageActions()

The favicon updates can be handled solely by updateFavicon(). You have to remove the tab state check from updateFavicon() loading btw.
Attachment #8358569 - Flags: review?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #30)
> A comment explaining the expected range for 'progress' would be useful here.
> Maybe throw or log a warning and/or clamp the value if the given 'progress'
> is out of range?

I had meant to change this to just accept a percentage value (0-100) and then do the conversion internally instead of exposing a MAX_PROGRESS that clients use, but I forgot. Updated the patch with this change (along with comments).

> @@ +517,5 @@
> > +                break;
> > +            case LOAD_ERROR:
> > +            case LOADED:
> > +                setTabProgress(tab, 80);
> > +                break;
> 
> It seems a bit backwards that the progress level is set by the UI (the
> toolbar, in this case). The UI shouldn't really need to know how the levels
> are set on each tab. For example, if, hypothetically, we come up with a very
> performant way to track real pageload progress with Gecko events, this UI
> code shouldn't really change much. So, with that in mind, I feel like this
> code should reside somewhere in Tab.java/Tabs.java and the setLoadProgress()
> should not be accessible outside the org.mozilla.gecko package.

One nice thing about having these in BrowserToolbar is that we're able to group events into their own switch statement, making it easy to see the progress level progression with the events. We could still extract these into their own group of if statements in Tabs.java, but it would be more costly since these are String comparisons rather than int ones. I moved these into Tabs.java, and I agree that it's good that we can remove the public setter, but I dislike how these setLoadProgress calls are scattered around.

If we ever convert the message Strings to be ints instead (bug 732177), we can think about extracting a separate switch statement for these like the previous patch did.

> With that in mind, here is my suggestion (which might need some tweaking):
> Have a updateProgress(Tab tab) method that updates the ToolbarProgressView
> visibility and level. It first updates the visibility by checking mIsEditing
> and the given tab's state (state == STATE_LOADING) and the progress level if
> the progress bar is set to visible. Call this in startEditing(),
> stopEditing() and updateDisplayLayout() (if UpdateFlags.PROGRESS is set, to
> ensure ToolbarProgressView and ToolbarDisplayLayout are always in sync).

I changed setProgressVisibility -> updateProgressVisibility, making it more generic so it can be used everywhere the visibility needs to be changed. Unfortunately, it's not quite so simple to put this in updateDisplayLayout() because of animations. Sometimes we want to animate the next progress step, but sometimes we don't (when leaving editing mode or when the selected tab changes). Selectively showing animations requires passing around animation flags to refreshState() and updateProgress(), which was ugly when I tried it.

I went with a different approach: I moved all PROGRESS updates into updateProgress(). This makes sure all PROGRESS updates are contained within updateProgress to ensure they're in sync, and it allows us to directly call animateProgress() or setProgress() without passing animation flags around.

> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +397,5 @@
> >          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
> >          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
> >          if (mUiMode == UIMode.PROGRESS) {
> >              mLastFavicon = null;
> > +            mFavicon.setImageResource(R.drawable.favicon);
> 
> You don't need to do anything about favicons when updating UI progress
> state. This was only necessary because we were using the same view to show
> both the page's favicon and the throbber. You can safely remove both lines
> above.

Actually, we do still need these. If we don't reset the favicon on new page loads, the favicon from the previous page will hang around until the new page's favicon is updated.

That said, these would probably be better off in updateFavicon() instead of here; I agree that updateUiMode is not the ideal place to update favicons. This requires a bit more tinkering, so I'll put these changes in a separate patch.
Attachment #8358569 - Attachment is obsolete: true
Attachment #8359489 - Flags: review?(lucasr.at.mozilla)
This patch explicitly resets the favicon on START, and makes sure the favicon is updated on SELECTED and STOP events (I found the latter was necessary for page reloads since we don't appear to get multiple FAVICON updates on refreshes). This required adding a RESET_FAVICON flag since a generic updateFavicon() call doesn't have enough context to know whether to reset the favicon.
Attachment #8359504 - Flags: review?(lucasr.at.mozilla)
Drive-by testing - I've run into a problem with the forward button (on tablets), where the forward button gets stuck and doesn't go away.

On an Asus Transformer:
1. Have at least two pages on the forward stack (by going back two pages)
2. Load a new page through the url
3. Tap the forward button a few times.

The forward button doesn't go away, even if you get to the end of the forward stack. Maybe a message is getting lost or consumed somewhere?
Comment on attachment 8359489 [details] [diff] [review]
Replace throbber with progress bar, v3

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

Looks good overall. Just want to avoid unnecessary and redundant updateDisplayLayout() calls during pageload.

::: mobile/android/base/Tabs.java
@@ +434,5 @@
>                  closeTab(tab);
>              } else if (event.equals("Tab:Select")) {
>                  selectTab(tab.getId());
>              } else if (event.equals("Content:LocationChange")) {
> +                tab.setLoadProgress(60);

I'd probably turn these into constants for better visibility and context. Something like:

LOAD_PROGRESS_START = 20
LOAD_PROGRESS_LOCATION_CHANGE = 60
LOAD_PROGRESS_LOAD_ERROR = 80
...

@@ +448,5 @@
>                  if ((state & GeckoAppShell.WPL_STATE_IS_NETWORK) != 0) {
>                      if ((state & GeckoAppShell.WPL_STATE_START) != 0) {
>                          boolean showProgress = message.getBoolean("showProgress");
>                          tab.handleDocumentStart(showProgress, message.getString("uri"));
> +                        tab.setLoadProgress(20);

I'd prefer to have these setLoadProgress() calls inside their handle* methods. But I can see how this approach would break for the messages that Tab doesn't have handle* methods for.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +493,5 @@
>          }
>      }
>  
> +    private void updateProgress(Tab tab, Tabs.TabEvents msg) {
> +        if (Tabs.getInstance().isSelectedTab(tab)) {

I'd prefer an early return here. But wouldn't it better to just avoid calling updateProgress() altogether for non-selected tabs? I'd actually say it's plain wrong to call updateProgress() with a non-selected tab.

@@ +506,5 @@
> +                case LOADED:
> +                    if (mProgressBar.getVisibility() == View.VISIBLE) {
> +                        mProgressBar.animateProgress(tab.getLoadProgress());
> +                    }
> +                    updateDisplayLayout(tab, EnumSet.of(UpdateFlags.PROGRESS));

This is guaranteed to cause both redundant and unnecessary updateDisplayLayout() calls given that updateProgress() is always called in onTabChanged(). We better avoid extra work, especially considering this happens during pageload. In fact, I just saw one case where we needlessly do an extra updateDisplayLayout() call in refreshState(). Filed bug 960529 to track it.

Here's quick idea:
1. Remove updateDisplayLayout() calls from updateProgress() and let it be handled as usual in onTabChanged. Restore any UpdateFlags.PROGRESS instance you removed.
2. Move the updateProgress() call inside the 'if (tabs.isSelectedTab(tab))' block in onTabChanged (just after the updateDisplayLayout call?). This means you can remove the isSelectedTab() check from updateProgress() too.

I realize I had suggested being more explicit about the keeping the state of progressbar and displaylayout in sync but, after looking at your patch, I'd rather lean towards a more efficient approach. We'll only call updateDisplayLayout() and updateProgress() in onTabChanged so it should be fairly simple to follow how the UI state changes.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +397,5 @@
>          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (mUiMode == UIMode.PROGRESS) {
>              mLastFavicon = null;
> +            mFavicon.setImageResource(R.drawable.favicon);

The fact that you still need to do this here is a sign that we have a bug in the way we update favicons during pageload.
Attachment #8359489 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 8359504 [details] [diff] [review]
Update favicon with RESET_FAVICON flag for new page loads

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

Wouldn't it be a lot simpler/cleaner if we simply set the tab's favicon to null (maybe try to fetch from Favicons memcache first?) in Tab.handleDocumentStart() (we already do something similar for identity data).

We should only show default favicon for the target URL on LOCATION_CHANGE (not START). We already do this in refreshState() btw. The idea being that we should only update the favicon at all when the target url shows up in the toolbar.

Also, if you do what I suggested, you can probably remove this code to show the default favicon in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1422

This is only necessary because we were not showing resetting the favicon during pageload (because of the throbber).
Attachment #8359504 - Flags: review?(lucasr.at.mozilla) → review-
(In reply to Lucas Rocha (:lucasr) from comment #35)

Sorry for all the churn, and thanks for the thorough reviews! I knew there was something fishy about that RESET_FAVICON patch...

> I realize I had suggested being more explicit about the keeping the state of
> progressbar and displaylayout in sync but, after looking at your patch, I'd
> rather lean towards a more efficient approach. We'll only call
> updateDisplayLayout() and updateProgress() in onTabChanged so it should be
> fairly simple to follow how the UI state changes.

How about the best of both worlds? We pass `flags` into updateProgress to keep the PROGRESS updates contained, allowing us to reuse the updateDisplayLayout() that's already there.

Side note: since updateFromTab() calls helper methods only for the given flags, there wasn't really any extra work happening here (other than the extremely cheap method calls and int comparisons). The previous patch wasn't calling updateFromTab() with any duplicate flags, so I don't think there was any negative performance impact from calling it multiple times. That said, I do agree that it's better to consolidate them.

> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +397,5 @@
> >          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
> >          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
> >          if (mUiMode == UIMode.PROGRESS) {
> >              mLastFavicon = null;
> > +            mFavicon.setImageResource(R.drawable.favicon);
> 
> The fact that you still need to do this here is a sign that we have a bug in
> the way we update favicons during pageload.

I'm not sure what was wrong before when I was testing this; indeed, we already have logic to reset the favicon on location change. This seems to be working now.

Re: comment 34: Chenxia and I did some more testing, and this seems to be unrelated. Filed bug 960746.
Attachment #8359489 - Attachment is obsolete: true
Attachment #8359504 - Attachment is obsolete: true
Attachment #8361390 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8361390 [details] [diff] [review]
Replace throbber with progress bar, v4

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

Awesome. This looks nice and clean. Two final questions:

- Don't you still need to reset Tab.mFavicon to null in handleDocumentStart() in order to get the default favicon shown in the toolbar when the tab starts loading a new page? Bonus points (in a follow-up maybe?) if we use the actual favicon if it's available in our Favicons memcache. 

- I like the idea of updateProgress() expressing that a UpdateFlags.PROGRESS is needed but, in terms of API, I find it a bit surprising that updateProgress() actually changes the given flags EnumSet. I wouldn't expect that by while reading onTabChanged(). I'd really like to keep all the changes to flags centralized in onTabChanged for clarity. Two possible alternatives here:
  1. Change updateProgress to return a boolean and in onTabChanged() add UpdateFlags.PROGRESS to the 'flags' EnumSet if it returns true.
  2. Change updateProgress to return a UpdateFlags EnumSet and add the returned flags to the 'flags' EnumSet in onTabChanged().

Options 2 is a bit more explicit about the intended semantics. Thoughts?
Flags: needinfo?(bnicholson)
Moves updateProgress inline in onTabChanged. handleLocationChange already clears the favicon for us at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#633, so we shouldn't need to do any additional clearing.
Attachment #8361390 - Attachment is obsolete: true
Attachment #8361390 - Flags: review?(lucasr.at.mozilla)
Attachment #8361822 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(bnicholson)
Comment on attachment 8361822 [details] [diff] [review]
Replace throbber with progress bar, v5

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

Ship it :-)
Attachment #8361822 - Flags: review?(lucasr.at.mozilla) → review+
Tracking in case we want this uplifted.
tracking-fennec: --- → ?
Hmm...same patch on central is green on try:
https://tbpl.mozilla.org/?tree=Try&rev=e56510991555

I wonder if there was something pushed today that conflicts with this patch? I'll go ahead and do another try push, this time on fx-team.
Depends on: 961339
Second try push on fx-team still green: https://tbpl.mozilla.org/?tree=Try&rev=7e2e23d43969

Probably just needs a clobber according to philor, so touched CLOBBER and filed bug 961339. Hopefully it sticks.

https://hg.mozilla.org/integration/fx-team/rev/31688c2aca28
https://hg.mozilla.org/mozilla-central/rev/31688c2aca28
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Tested on an Asus Nexus 7 (2013, 4.4.2), and a LG Nexus 4 (4.4.2), looks good to me.

SoftVision, can you try this out on a couple other devices of varying versions?
Flags: needinfo?(fennec)
Keywords: verifyme
Verified as fixed in build:
- Nightly 29.0a1 (2014-01-20);
Devices:
- Samsung Galaxy S (Android 2.3.4);
- Samsung Galaxy Nexus (Android 4.2.1);
- Asus Transformer Tab (Android 4.0.1).
Flags: needinfo?(fennec)
Keywords: verifyme
Depends on: 962103
Depends on: 962106
Too close to merge to track for 28.
tracking-fennec: ? → 29+
Blocks: 969302
Depends on: 969492
Depends on: 976144
Depends on: 976426
You need to log in before you can comment on or make changes to this bug.