969492 - No progress shown during startup when loading URL from external app

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

If fennec isn't running, and you try opening a URL from another app, you will see a blank white page with no indication of prgress anywhere in the UI. Maybe we should just move the progress bar 10% or so to start?

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Move LOAD_PROGRESS constants into Tab

These constants make more sense in Tab since it holds the getLoadProgress/setLoadProgress methods.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Use 10% as the load progress reset state

The problem with simply setting the progress bar to some value at startup is that we'll eventually get a document start, causing the progress bar to reset to 0 before animating.

Mark suggested using 10% as the "initial" progress bar value; that is, the progress bar is reset to 10% instead of 0% when receiving a document start or opening a new tab. Doing this effectively prevents the reset progress issue mentioned above since the pre-start value and the reset value are the same (10%).

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8381081 [details] [diff] [review]
Move LOAD_PROGRESS constants into Tab

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

::: mobile/android/base/Tab.java
@@ +79,5 @@
>  
> +    public static final int LOAD_PROGRESS_START = 20;
> +    public static final int LOAD_PROGRESS_LOCATION_CHANGE = 60;
> +    public static final int LOAD_PROGRESS_LOADED = 80;
> +    public static final int LOAD_PROGRESS_STOP = 100;

Public? At the very least, this should package-scoped.

@@ +687,5 @@
>              }
>          }, 500);
> +    }
> +
> +    void handleLoaded() {

nit: I'd go with handleContentLoaded() to give a bit more context.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8381194 [details] [diff] [review]
Use 10% as the load progress reset state

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

Need questions answered before giving r+.

::: mobile/android/base/Tab.java
@@ +76,5 @@
>      public static final int STATE_LOADING = 1;
>      public static final int STATE_SUCCESS = 2;
>      public static final int STATE_ERROR = 3;
>  
> +    public static final int LOAD_PROGRESS_INIT = 10;

Ah, now I see why you meant these constants public. That's fine.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +469,5 @@
>  
>              // Progress-related handling
>              switch (msg) {
>                  case START:
> +                    updateProgressVisibility(tab, Tab.LOAD_PROGRESS_INIT);

Does this mean we'll just snap to 10% instead of animating the progress bar on startup? Ideally, we'd initially animate the progress bar to 10% until the page actually starts loading.

@@ +474,2 @@
>                      // Fall through.
> +                case ADDED:

What issue are you fixing with handling ADDED here?

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8381194 [details] [diff] [review]
Use 10% as the load progress reset state

(In reply to Lucas Rocha (:lucasr) from comment #4)
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +469,5 @@
> >  
> >              // Progress-related handling
> >              switch (msg) {
> >                  case START:
> > +                    updateProgressVisibility(tab, Tab.LOAD_PROGRESS_INIT);
> 
> Does this mean we'll just snap to 10% instead of animating the progress bar
> on startup? Ideally, we'd initially animate the progress bar to 10% until
> the page actually starts loading.

On startup, we will animate -- we don't get a START event until *after* ADDED (which calls animateProgress). By the time START comes in, the progress bar will have already animated to the 10% from ADDED, so this will have no effect.

But for all other situations where we load a new page -- clicking links, entering URLs, etc. -- it will jump straight to 10% before animating. I would prefer to animate these too, but that brings us back to the reset problem I mentioned in comment 0.

> @@ +474,2 @@
> >                      // Fall through.
> > +                case ADDED:
> 
> What issue are you fixing with handling ADDED here?

The main bug filed here, startup. BrowserToolbar is isolated from our startup code in GeckoApp, so it needs to listen to some event to animate the progress bar to the initial 10%. START isn't enough since Gecko hasn't been initialized yet.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8381194 [details] [diff] [review]
Use 10% as the load progress reset state

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

I have this nagging feeling that there's a slightly better way to do this but I don't have anything useful to suggest right now. Let's do this for now.

Comment 7

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/6f3132653066
https://hg.mozilla.org/integration/fx-team/rev/3b9473873820

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> @@ +687,5 @@
> >              }
> >          }, 500);
> > +    }
> > +
> > +    void handleLoaded() {
> 
> nit: I'd go with handleContentLoaded() to give a bit more context.

Shoot, I missed this suggestion. Tried pushing a follow-up, but of course fx-team closed immediately after landing. I'll try to remember to land it once it reopens.

Comment 9

[Brian Nicholson (:bnicholson)]

Well that was fast -- it's open again. Follow-up naming fix:
https://hg.mozilla.org/integration/fx-team/rev/4e909fc80943

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f3132653066
https://hg.mozilla.org/mozilla-central/rev/3b9473873820
https://hg.mozilla.org/mozilla-central/rev/4e909fc80943

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8381081 [details] [diff] [review]
Move LOAD_PROGRESS constants into Tab

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 917896
User impact if declined: progress bar does not appear at startup, making Fennec appear frozen
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 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30f1791291fe
https://hg.mozilla.org/releases/mozilla-aurora/rev/711d6777df04
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebe8ca2d3ebf

Comment 13

[Flaviu Cos]

Verified as fixed in builds:
- 29 beta 2; 
- 30.0a2 (2014-03-25);
- 31.0a1 (2014-03-25);
Device: Google Nexus 10 (Android 4.4.2).