Closed Bug 719868 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
fennec 11+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Whiteboard: startup-ux)

Attachments

(1 file, 4 obsolete files)

Somehow my patch from bug 718846 didn't catch this code path :(
tracking-fennec: --- → 11+
Priority: -- → P3
Blocks: 721008
Whiteboard: startup-ux
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #591642 - Flags: review?(mark.finkle)
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?
Attachment #591642 - Flags: review?(mark.finkle) → review-
Attached patch patch (obsolete) — Splinter Review
Addressed comments. We should also make sure Brian does this in his patch for bug 701092.
Attachment #591642 - Attachment is obsolete: true
Attachment #591965 - Flags: review?(mark.finkle)
(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 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.
Attachment #591965 - Flags: review?(mark.finkle) → review+
Backed out because of twinopen failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/687b80084d3d
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #591965 - Attachment is obsolete: true
Attachment #592404 - Flags: review?(mark.finkle)
Attached patch updated patch (un-bitrotted) (obsolete) — Splinter Review
Attachment #592404 - Attachment is obsolete: true
Attachment #592404 - Flags: review?(mark.finkle)
Attachment #592778 - Flags: review?(mark.finkle)
This addresses what we talked about on IRC.
Attachment #592778 - Attachment is obsolete: true
Attachment #592778 - Flags: review?(mark.finkle)
Attachment #592913 - Flags: review?(mark.finkle)
Attachment #592913 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/042092c83f22
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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.
Attachment #592913 - Flags: approval-mozilla-aurora?
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.
Attachment #592913 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 592913 [details] [diff] [review]
patch w/ default params changes

[Triage Comment]
Mobile only - approved for Beta 11.
Attachment #592913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.