Closed Bug 656923 Opened 13 years ago Closed 13 years ago

Tab opened on command-line is in the background behind restored tabs

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 6

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Keywords: regression, Whiteboard: [fennec-sessionstore])

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
1. Open a web page in Fennec.
2. Kill Fennec.
3. Open Fennec with a URL on the command line (desktop) for from another app (Android).

Expected results: The URL from the command line opens in the foreground tab.
Actual results: The URL from the command line opens in a background tab, while a restored tab from the previous session is in the foreground.

Looks like this is probably a regression from bug 653091.
Commenting out the code from bug 653091 does not fix this bug.
No longer blocks: 653091
This happens because SessionStore.restoreLastSession now uses NetUtil.asyncFetch, which means its tabs get added after the startup tab.  Regression from bug 630398.
Blocks: 630398
We can use "sessionstore-windows-restored" to listen for when the session is finished being restored and update the selected tab. We already do something like this to kill the dummy tab we need to create:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#353

Looks like we just need to tweak the code there a little bit.
Attached patch patch (obsolete) — Splinter Review
This patch uses a different approach (I wrote it before reading your comment) - if there is already an active tab, then we don't create the dummy tab at all, and we don't bring other tabs to the front during restore.

If we don't want to modify the nsISessionStore interface, then we could move more of this logic into SessionStore.js.  (I thought it was better to let Browser make the decisions about focusing tabs, however.)
Attachment #532252 - Flags: review?(mark.finkle)
Comment on attachment 532252 [details] [diff] [review]
patch

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>     let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>     if (ss.shouldRestore()) {
>+      // First open any commandline URLs, except the homepage
>+      if (commandURL && commandURL != this.getHomePage())
>+        this.addTab(commandURL, true);
>+

Can't we move the dummy-tab code into an "else" block here? Also, use a local bool "bringToFront" instead of using "hasTabs"

>+      ss.restoreLastSession(!hasTabs);

I'm not a huge fan of this. I agree with you about letting browser.js handle the selection. I suppose it works for now.

>   addTab: function browser_addTab(aURI, aBringFront, aOwner, aParams) {
>+    debugger;

remove

r-, just to see the nits fixed and the minor refactor
Attachment #532252 - Flags: review?(mark.finkle) → review-
Attached patch patch v2Splinter Review
Addressed review comments
Attachment #532252 - Attachment is obsolete: true
Attachment #532437 - Flags: review?(mark.finkle)
Attachment #532437 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/992f91cd375d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110517 Firefox/6.0a1 Fennec/6.0a1 
Device: LG Optimus 2X (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: