746380 - Throbber stops too early when loading new page from about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Go to about:home
2) Click AwesomeBar, enter nytimes.com, hit go

Expected result:
Throbber will animate until page is loaded

Actual result:
Throbber stops shortly after entering URL, which is long before the page finishes loading

Loading from any page other than about:home will animate the throbber until the page is finished.

Comment 1

[Brian Nicholson (:bnicholson)]

Nom'ing since loading a page after being on about:home is a common case.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

This could be a regression from bug 738938

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: part 1: Use URI of nsIChannel on state change

When a document start event is received, the URI of the browser still refers to previous URI. We can pull the URI associated with the document start directly from the nsIChannel.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: part 2: Set success state for about:home on document start

In bug 738938, we relied on the tab's URL to determine whether the request is for about:home. Unfortunately, we get the document start before the location change, so the URL will refer to the URL of the previous page. This patch checks the URL given by the nsIChannel from the first patch.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: part 3: Set success state for tabs initialized to about:home

If we create a new about:home tab, our tab selected event can fire before we receive the document start. To prevent the throbber from immediately appearing, this patch checks the URI of the tab when it's initialized; if it's about:home, it's immediately set to the success state.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 617058 [details] [diff] [review]
part 2: Set success state for about:home on document start

>+    void handleDocumentStart(int tabId, final boolean showProgress, String uri) {

>-        tab.setState(Tab.STATE_LOADING);
>+        final boolean isAboutHome = "about:home".equals(uri);
>+        tab.setState(isAboutHome ? Tab.STATE_SUCCESS : Tab.STATE_LOADING);

You don't need to do this. Tab.setState already has a check for "about:home"

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#326

>-                    if (showProgress)
>+                    if (showProgress && !isAboutHome)
>                         mBrowserToolbar.setProgressVisibility(true);

Let's not use isAboutHome here. Use Tab.getState()

>+                    if (showProgress && tab.getState() == Tab.STATE_LOADING)

The less hard coded "about:home" checks the better

r+ with those fixes

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 617061 [details] [diff] [review]
part 3: Set success state for tabs initialized to about:home

NO WAY!

I refuse to live like this.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: part 2: Set tab URL on document start

Nicer approach that keeps the about:home disease contained.

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/04994824992a
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ab101096ab3

Comment 10

[Brian Nicholson (:bnicholson)]

Backed out part 2 for breaking favicons:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a0a2fbe7b47

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: part 3: Update browser toolbar on document start

Since we now set the URL on document start, we need to refresh the browser toolbar.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: part 3: Add same document flag to location change

This patch lets our location change listener know whether the location change was fired on the same document (e.g., anchor tags). This means we don't need to rely on the old URL, which was broken in part 2.

Comment 13

[Brian Nicholson (:bnicholson)]

Relanded part 2 and landed the new part 3:

http://hg.mozilla.org/integration/mozilla-inbound/rev/22d17b21018c
http://hg.mozilla.org/integration/mozilla-inbound/rev/0df5ff135d64

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/04994824992a
https://hg.mozilla.org/mozilla-central/rev/4ab101096ab3

Comment 15

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/3a0a2fbe7b47

Comment 16

[Ed Morley [:emorley]]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> Relanded part 2 and landed the new part 3:
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/22d17b21018c
> http://hg.mozilla.org/integration/mozilla-inbound/rev/0df5ff135d64

https://hg.mozilla.org/mozilla-central/rev/22d17b21018c
https://hg.mozilla.org/mozilla-central/rev/0df5ff135d64

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 617057 [details] [diff] [review]
part 1: Use URI of nsIChannel on state change

[Approval Request Comment]
Regression caused by (bug #): bug 738938
User impact if declined: throbber stops too early
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): causes 2 known regressions that have been fixed: bug 748498 and bug 750130
String changes made by this patch: none

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 617103 [details] [diff] [review]
part 2: Set tab URL on document start

[Approval Request Comment]
Regression caused by (bug #): bug 738938
User impact if declined: throbber stops too early
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): causes 2 known regressions that have been fixed: bug 748498 and bug 750130
String changes made by this patch: none

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 617639 [details] [diff] [review]
part 3: Add same document flag to location change

[Approval Request Comment]
Regression caused by (bug #): bug 738938
User impact if declined: throbber stops too early
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): causes 2 known regressions that have been fixed: bug 748498 and bug 750130
String changes made by this patch: none

Comment 20

[Brian Nicholson (:bnicholson)]

Looks like this was already merged into Aurora.

Comment 21

[Alfredos-Panagiotis Damkalis [:fredy]]

I verify that this bug is fixed on beta version (v14).

Comment 22

[Catalin Suciu [:csuciu]]

Verified fixed on:
Nightly 15.0a1 (2012-05-29) 
Aurora 14.0a2 (2012-05-29)

Device: Galaxy SII (Android 2.3.4)