719868 - Progress throbber still shows when we load about:home on startup

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[:Margaret Leibovic]

Somehow my patch from bug 718846 didn't catch this code path :(

Comment 1

[:Margaret Leibovic]

Attachment: patch

Putting the about:home check in the Tab:Load/Tab:Add observer didn't catch the cases where we're loading a URI directly from JS, which we're doing at startup (going through addTab).

Comment 2

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

Comment on attachment 591642 [details] [diff] [review]
patch

I would rather keep addTab and loadURI ignorant of "about:home". Instead, let's pass a param object in browser.js startup:

>             Services.obs.removeObserver(restoreCleanup, "sessionstore-windows-restored");
>             if (aData == "fail")
>-              BrowserApp.addTab("about:home");
>+              BrowserApp.addTab("about:home", { showProgress: false });


>       ss.restoreLastSession(restoreToFront);
>     } else {
>-      this.addTab(url);
>+      this.addTab(url, { showProgress: url != "about:home" });

Would this cover the situation you are trying to fix?

Comment 3

[:Margaret Leibovic]

Attachment: patch

Addressed comments. We should also make sure Brian does this in his patch for bug 701092.

Comment 4

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #3)

> We should also make sure Brian does this in his patch
> for bug 701092.

Oh, I just looked more closely and he's just moving one of these lines around, so he should catch it when he gets a conflict.

Comment 5

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

Comment on attachment 591965 [details] [diff] [review]
patch

I thought some more about your feedback about not wanting to depend on callers to set showProgress. I still think using the param is better for flexibility. I could be proved wrong in time though.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3a4af0e68a

Comment 7

[:Margaret Leibovic]

Backed out because of twinopen failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/687b80084d3d

Comment 8

[:Margaret Leibovic]

Attachment: updated patch

This was green on try. The problem was that by passing a params object ourselves, we weren't picking up some default params from addTab: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#470.

This makes me slightly concerned that it's too easy to make mistakes like this. I was considering changing the way we set default params in addTab to always set those options, but I didn't know if that would break other addTab consumers, so I left it alone. Perhaps something to look into in the future.

Comment 9

[:Margaret Leibovic]

Attachment: updated patch (un-bitrotted)

Comment 10

[:Margaret Leibovic]

Attachment: patch w/ default params changes

This addresses what we talked about on IRC.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/042092c83f22

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/042092c83f22

Comment 13

[:Margaret Leibovic]

Comment on attachment 592913 [details] [diff] [review]
patch w/ default params changes

[Approval Request Comment]
Mobile-only polish fix to make startup appear less janky. Low risk.

Comment 14

[:Margaret Leibovic]

Comment on attachment 592913 [details] [diff] [review]
patch w/ default params changes

[Approval Request Comment]
This made its way into aurora through the merge, but it still needs to be uplifted to beta.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 592913 [details] [diff] [review]
patch w/ default params changes

[Triage Comment]
Mobile only - approved for Beta 11.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d15796d29633

Comment 17

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-06)
20120206031148
http://hg.mozilla.org/mozilla-central/rev/814d0b2dbaba
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 12.0a2 (2012-02-06)
20120206042011
http://hg.mozilla.org/releases/mozilla-aurora/rev/9fb0c06ceb49
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 11.0
20120206202409
http://hg.mozilla.org/releases/mozilla-beta/rev/1c0aba74d116
Device: HTC Desire Z
OS: Android 2.3.3