Closed Bug 969492 Opened 6 years ago Closed 6 years ago

No progress shown during startup when loading URL from external app

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

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

People

(Reporter: Margaret, Assigned: bnicholson)

References

Details

Attachments

(2 files)

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?
tracking-fennec: --- → ?
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
tracking-fennec: ? → 29+
These constants make more sense in Tab since it holds the getLoadProgress/setLoadProgress methods.
Attachment #8381081 - Flags: review?(lucasr.at.mozilla)
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%).
Attachment #8381194 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8381081 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #8381194 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8381194 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8381194 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Well that was fast -- it's open again. Follow-up naming fix:
https://hg.mozilla.org/integration/fx-team/rev/4e909fc80943
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
Attachment #8381081 - Flags: approval-mozilla-aurora?
Attachment #8381194 - Flags: approval-mozilla-aurora?
Attachment #8381081 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8381194 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.