Closed Bug 558614 Opened 14 years ago Closed 14 years ago

Implement Firefox TabBrowser API: loadTabs(), loadOneTab() and change addTab() to get feature parity

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1a3
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: sgautherie, Assigned: misak.bugzilla)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(3 files, 14 obsolete files)

20.36 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.45 KB, patch
misak.bugzilla
: review+
Details | Diff | Splinter Review
6.70 KB, patch
misak.bugzilla
: review+
misak.bugzilla
: superreview+
Details | Diff | Splinter Review
http://mxr.mozilla.org/mozilla-central/search?string=loadTabs&case=1
{
/browser/base/content/tabbrowser.xml
    * line 1064 -- <method name="loadTabs">
}

Currently needed by bug 544170
/accessible/tests/mochitest/tree/test_tabbrowser.xul
Will be needed to port
/browser/components/sessionstore/test/browser/browser_522545.js
Might be needed for some places related stuff too (or not).
Flags: wanted-seamonkey2.1?
Depends on: 558589
As a note, I'd rather have the wanted flag on the whole feature, i.e. bug 467867 than on the single bugs implementing it - for now. But then, the value of wanted flags is ambiguous at best anyhow.
Blocks: 558996
Attached patch patch (obsolete) — Splinter Review
This patch implements loadOneTab and loadTabs, as well as modifies parameters for addTab.
Assignee: nobody → misak.bugzilla
Status: NEW → ASSIGNED
Attachment #439231 - Flags: review?(neil)
Comment on attachment 439231 [details] [diff] [review]
patch


http://mxr.mozilla.org/comm-central/search?string=addTab%5B%5Ea-z%5D&regexp=on&case=on
{
/editor/ui/composer/content/ComposerCommands.js
    * line 2195 -- browser.selectedTab = browser.addTab(params.url, null, null, false,
}
Fwiw, I guess either this call needs to be updated or the current situation might explain (part of) bug 557050 comment 15.
(NB: That's the only case out of /suite/ to double-check.)

http://mxr.mozilla.org/comm-central/search?string=addTab%5B%5Ea-z%5D&regexp=1&case=1&find=%2Fsuite%2F
It looks like some of our own calls do need to be updated, don't they?

http://mxr.mozilla.org/comm-central/search?string=loadURIWithFlags&case=1
Have you double-checked if we want/need to update some of our calls to match Firefox ones?
Attachment #439231 - Flags: feedback-
(In reply to comment #3)
> http://mxr.mozilla.org/comm-central/search?string=addTab%5B%5Ea-z%5D&regexp=1&case=1&find=%2Fsuite%2F
> It looks like some of our own calls do need to be updated, don't they?

We need to check all those calls and update those that need it. I already mentioned to Misak on IRC that we need to look at all those callers and check if they need to be updated.
Note that I'd only care for updating in a way that the correct params are in the correct places, but potentially file a followup on correcting some to use the newly available params (probably by looking at where Firefox uses those as well).

> http://mxr.mozilla.org/comm-central/search?string=loadURIWithFlags&case=1
> Have you double-checked if we want/need to update some of our calls to match
> Firefox ones?

loadURIWithFlags is a <browser> function, which is in toolkit only, so this patch just hands it an additional param, which is just fine. Any updates to other calls to this function are completely out of scope for this bug here, which is about implementing loadTabs(), after all.
Attached patch with updated callers (obsolete) — Splinter Review
My first plan was to get code review, then update callers, but since they are few, i modified them.
Attachment #439231 - Attachment is obsolete: true
Attachment #439508 - Flags: review?(neil)
Attachment #439231 - Flags: review?(neil)
Attached patch with updated callers, fixed (obsolete) — Splinter Review
same as above, fixed some typos
Attachment #439508 - Attachment is obsolete: true
Attachment #439518 - Flags: review?(neil)
Attachment #439508 - Flags: review?(neil)
Comment on attachment 439518 [details] [diff] [review]
with updated callers, fixed

>+      gBrowser.selectedTab = gBrowser.addTab(url, { aAllowThirdPartyFixup: true});
Nit: no space after {

>+      <field name="_lastRelatedTab">
>+        null
>+      </field>
If you're going to add this then you'll need to add all the code that updates it, not just the bits that you use.

>+                                  ownerTab: owner,
This doesn't seem to get used anywhere either. Instead in suite if you set aFocusNewTab to true it will also set the "owner".

>+        <parameter name="aPostData"/>
>+        <parameter name="aOwner"/>
>+        <parameter name="aAllowThirdPartyFixup"/>
I'm not sure what will confuse extensions the least - supporting the new 2-arg and the "old" Firefox API, or supporting the new 2-arg and the "old" Suite API.

>         <parameter name="aFocusNewTab"/>
>         <parameter name="aFlags"/>
No callers use these any more, and they try to use {aFocusNewTab} or {aFlags} but they doesn't work any more either. The aFlags users could be switched to using {aAllowThirdPartyFixup} instead though.
(In reply to comment #7)
> I'm not sure what will confuse extensions the least - supporting the new 2-arg
> and the "old" Firefox API, or supporting the new 2-arg and the "old" Suite API.

In case of confusion, it's best for us to follow the Firefox API, as the likeliness of extensions having code to support it is very very high. I don't fear some extensions needing updates to remove SeaMonkey-only workarounds to be compatible with 2.1 ;-)
Attached patch v2 (obsolete) — Splinter Review
fixed all comments, added Flags and FocusNewTab to the params. loadOneTab uses FocusNewTab.
Attachment #439518 - Attachment is obsolete: true
Attachment #439793 - Flags: review?(neil)
Attachment #439518 - Flags: review?(neil)
Assignee: misak.bugzilla → nobody
Component: UI Design → Tabbed Browser
QA Contact: ui-design → tabbed-browser
Philip, be careful about resetting assignees when switching components (that's checked by default but not always wanted!)
Assignee: nobody → misak.bugzilla
Flags: wanted-seamonkey2.1?
Comment on attachment 439793 [details] [diff] [review]
v2

Philip, can you give feedback for patch, especially on parameter addition part ? I suspect i did wrong things there.
Attachment #439793 - Flags: feedback?(philip.chee)
Comment on attachment 439793 [details] [diff] [review]
v2

+            var tab = this.addTab(aURI, {
+                                  referrerURI: aReferrerURI,
+                                  charset: aCharset,
+                                  postData: aPostData,
+                                  FocusNewTab: !bgLoad,
+                                  allowThirdPartyFixup: aAllowThirdPartyFixup,
+                                  relatedToCurrent: aRelatedToCurrent});

According to the mozilla coding style guide:
<https://developer.mozilla.org/En/Developer_Guide/Coding_Style#JavaScript_Objects>
For javascript objects, if the opening brace is on a separate line the closing brace should also be on a separate line so perhaps something like:
+                                  relatedToCurrent: aRelatedToCurrent
+                                 });

+                !(arguments[1] instanceof Ci.nsIURI)) {

Components.interfaces

+              let fixupFlag = aAllowThirdPartyFixup ?
+                          Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP :
+                          Ci.nsIWebNavigation.LOAD_FLAGS_NONE;

Components.interfaces


+              let newTabPos = (this._lastRelatedTab ||
+                               this.selectedTab)._tPos + 1;
+              this.moveTabTo(t, newTabPos);

You don't initialize _tPos nor update it ever so this doesn't do what you think it does. Since it is an internal implementation detail in Firefox we are not obliged to implement this. What you can do is probably something like:

let newTabPos = this.getTabIndex(this._lastRelatedTab || this.selectedTab) + 1;

<method name="removeTab">

Just before this:

+            this._lastRelatedTab = null;
             var index = this.getTabIndex(aTab);

I think you should also remove this tab as the owner of any other tabs, since it's going away.

There are also several internal uses of addTab() in tabbrowser.xul that you haven't converted to the new API.

       case nsIBrowserDOMWindow.OPEN_NEWTAB:
-        var newTab = gBrowser.addTab("about:blank", null, null,
-            !Services.prefs.getBoolPref("browser.tabs.loadDivertedInBackground"));
+        var newTab = gBrowser.addTab("about:blank", { 
+            aFocusNewTab: !Services.prefs.getBoolPref("browser.tabs.loadDivertedInBackground")});

I think it would be more readable with an intermediate var e.g.

        var focus = !Services.prefs.getBoolPref("browser.tabs.loadDivertedInBackground");
        var newTab = gBrowser.addTab("about:blank", {aFocusNewTab: focus});

+  browser.addTab(aURL, {
+                         aReferrerURI: referrer,
+                         aCharset:     originCharset,
+                         aFocusNewTab: !aLoadInBackground});
Bracing style {} of the second parameter. Also only one space after each :

@@ -1226,17 +1229,17 @@ function openUILinkIn(url, where, allowT
   case "tab":
     var browser = w.getBrowser();
-    var tab = browser.addTab(url, null, null, false, flags);
+    var tab = browser.addTab(url, {aFlags: flags}); 

The Firefox signature for openUILinkIn() is:
  function openUILinkIn(url, where, aAllowThirdPartyFixup, aPostData, aReferrerURI)

Also it can also take an object for the third parameter. This was implemented in:
Bug 539594 [Middle-clicking back/forward/reload should open the new tab next to the current one]
See <http://hg.mozilla.org/mozilla-central/diff/f53d65371670/browser/base/content/utilityOverlay.js>

Please file a followup bug to port Bug 539594 (depends on this bug) once this patch lands.

function openUILinkIn()

  case "tab":
    var browser = w.getBrowser();
    var tab = browser.addTab(url, {aFlags: flags});
    if (!loadInBackground) {
      browser.selectedTab = tab;

Could just pass loadInBackground directly to addTab().


bookmarks.js:
    case "tab":
      var tab = browser.addTab(url);
      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
        browser.selectedTab = tab;
      break;

Could just pass shouldLoadTabInBackground() directly to addTab().

contentAreaClick.js

    else if (event.target == browser) {
      tab = browser.addTab(url);
      if (event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
        browser.selectedTab = tab;
    }

Ditto.

notification.xml

                else {
                  // new tab
                  var browser = browserWin.getBrowser();
                  var newTab = browser.addTab(url);
                  browser.selectedTab = newTab;
                }

Ditto.

Opening a new tab by middle click on a link or right click->Open in new tab. Always opens in the background ignoring browser.tabs.loadInBackground.

Open browser component.
Set browser.tabs.loadInBackground to false.
Open mailnews component.
Open a message with a http:// link.
Right click on the link and select Open link in new tab.
Browser window is focused but the new tab still loads in the background.

With browser.tabs.loadInBackground to true, tabs load in the background but the browser window doesn't come into focus.
Attachment #439793 - Flags: feedback?(philip.chee) → feedback-
Attached patch v3 (obsolete) — Splinter Review
Fixed all stuff that Philip pointed, requesting feedback and review again.
Attachment #439793 - Attachment is obsolete: true
Attachment #444303 - Flags: superreview?(neil)
Attachment #444303 - Flags: review?(neil)
Attachment #444303 - Flags: feedback?(philip.chee)
Attachment #439793 - Flags: review?(neil)
Blocks: 564677
Comment on attachment 444303 [details] [diff] [review]
v3

>+        var newTab = gBrowser.addTab("about:blank", {aFocusNewTab: focus});
You don't actually handle the aFocusNewTab property.

>-          var newTab = gBrowser.addTab(defaultSearchURL);
>           var loadInBackground = Services.prefs.getBoolPref("browser.tabs.loadInBackground");
>           if (reverseBackgroundPref)
>             loadInBackground = !loadInBackground;
>-          if (!loadInBackground)
>-            gBrowser.selectedTab = newTab;
>+          var newTab = gBrowser.addTab(defaultSearchURL, {aFocusNewTab: !loadInBackground});
The existing code is written like this because it doesn't want the current tab to become the "owner" of the new tab; our ownership model is different¹, and while you could switch ownership model you would then have to fix up the current callers who expect that setting aFocusNewTab also sets the owner.

>+            var aRelatedToCurrent;
I'm not sure that this gets used either.

>         <parameter name="aFocusNewTab"/>
>         <parameter name="aFlags"/>
Unused?

>+            // If this new tab is owned by another, assert that relationship
>+            if (aOwner)
>+              t.owner = aOwner;
If you are going to set the (as yet unused) owner, best do it after creating the new tab!

>+            // Check if we're opening a tab related to the current tab and
>+            // move it to after the current tab.
>+            // aReferrerURI is null or undefined if the tab is opened from
>+            // an external application or bookmark, i.e. somewhere other
>+            // than the current tab.
>+            if ((aRelatedToCurrent == null ? aReferrerURI : aRelatedToCurrent) &&
(aRelatedToCurrent || aReferrerURI)

>+                Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
this.mPrefs, and add it to browser-prefs.js (Services is OK in .js files.)

>+              let newTabPos = (this._lastRelatedTab || this.selectedTab) + 1;
Ah, now I see where _lastRelatedTab fits in. Ideally this could would use indexes something like this:
if (!this._lastRelatedIndex)
  this._lastRelatedIndex = this.mTabContainer.selectedIndex;
this.moveTabTo(t, ++this._lastRelatedIndex);

>+                tab = this.addTab(getShortcutOrURI(url), { aFocusNewTab: !bgLoad});
Inconsistency is wrong, but I can't remember whether we prefer
{aFocusNewTab: !bgLoad} or { aFocusNewTab: !bgLoad }.

>+  browser.addTab(aURL, {
>+                         aReferrerURI: referrer,
>+                         aCharset: originCharset,
>+                         aFocusNewTab: !aLoadInBackground
>+                       });
Nit: continuation lines don't need so much indentation.

¹If you pass true for aFocusNewTab, and the user's next tab action is to close the newly opened tab, then the previous tab is selected.
Attached patch v4 (obsolete) — Splinter Review
(In reply to comment #14)
> (From update of attachment 444303 [details] [diff] [review])
> >+        var newTab = gBrowser.addTab("about:blank", {aFocusNewTab: focus});
> You don't actually handle the aFocusNewTab property.
> 
I removed aFocusNewTab, as You previously recommend and converted all callers to use loadOneTab.

> The existing code is written like this because it doesn't want the current tab
> to become the "owner" of the new tab; our ownership model is different¹, and
> while you could switch ownership model you would then have to fix up the
> current callers who expect that setting aFocusNewTab also sets the owner.

I'm not sure that i get all right, but i removed our ownership code and used FF, trying to mimic our behavior. Likely I didn't fixed all callers, but I want to get approval to what done.


> >+            var aRelatedToCurrent;
> I'm not sure that this gets used either.

It's used later in code.

> >         <parameter name="aFocusNewTab"/>
> >         <parameter name="aFlags"/>
> Unused?
 
Removed.

> >+            // If this new tab is owned by another, assert that relationship
> >+            if (aOwner)
> >+              t.owner = aOwner;
> If you are going to set the (as yet unused) owner, best do it after creating
> the new tab!

Moved.
 
> >+            // Check if we're opening a tab related to the current tab and
> >+            // move it to after the current tab.
> >+            // aReferrerURI is null or undefined if the tab is opened from
> >+            // an external application or bookmark, i.e. somewhere other
> >+            // than the current tab.
> >+            if ((aRelatedToCurrent == null ? aReferrerURI : aRelatedToCurrent) &&
> (aRelatedToCurrent || aReferrerURI)

I noticed that SMILE browser_Browser test failing with this patch, but unsure why, anyway it worth to ask for review and feedback, especially Philip is the author of SMILE implementation.
Not sure that I get it right, please check.
 
> >+              let newTabPos = (this._lastRelatedTab || this.selectedTab) + 1;
> Ah, now I see where _lastRelatedTab fits in. Ideally this could would use
> indexes something like this:
> if (!this._lastRelatedIndex)
>   this._lastRelatedIndex = this.mTabContainer.selectedIndex;
> this.moveTabTo(t, ++this._lastRelatedIndex);

Fixed, please check.
 
> >+                tab = this.addTab(getShortcutOrURI(url), { aFocusNewTab: !bgLoad});
> Inconsistency is wrong, but I can't remember whether we prefer
> {aFocusNewTab: !bgLoad} or { aFocusNewTab: !bgLoad }.

used no space after { everywhere. 

> ¹If you pass true for aFocusNewTab, and the user's next tab action is to close
> the newly opened tab, then the previous tab is selected.

This behavior should stay as loadOneTab set's owner if tab loaded in background.
Attachment #444303 - Attachment is obsolete: true
Attachment #444368 - Flags: review?(neil)
Attachment #444368 - Flags: feedback?(philip.chee)
Attachment #444303 - Flags: superreview?(neil)
Attachment #444303 - Flags: review?(neil)
Attachment #444303 - Flags: feedback?(philip.chee)
Summary: Implement Firefox TabBrowser API: loadTabs() → Implement Firefox TabBrowser API: loadTabs(), loadOneTab() and change addTab() to get feature parity
Attached patch v4a (obsolete) — Splinter Review
Sorry for spamming with patches, previous one misses one line in loadOnetab. Also i changed bug summary to reflect all work that going here.
Attachment #444368 - Attachment is obsolete: true
Attachment #444438 - Flags: review?(neil)
Attachment #444438 - Flags: feedback?(philip.chee)
Attachment #444368 - Flags: review?(neil)
Attachment #444368 - Flags: feedback?(philip.chee)
Comment on attachment 444438 [details] [diff] [review]
v4a

Some of your conversions of loadOneTab are correct, but some are wrong. Here's an example:

>-          var newTab = gBrowser.addTab(defaultSearchURL);
>           var loadInBackground = Services.prefs.getBoolPref("browser.tabs.loadInBackground");
>           if (reverseBackgroundPref)
>             loadInBackground = !loadInBackground;
>-          if (!loadInBackground)
>-            gBrowser.selectedTab = newTab;
>+          var newTab = gBrowser.loadOneTab(defaultSearchURL, {aLoadInBackground: loadInBackground});
The old code always passed false to aFocusNewTab, to ensure that the old tab wasn't the owner of the new tab. But because you're using loadOneTab, this sets the owner of the new tab.

>+        <parameter name="aOwner"/>
Bah, the Firefox code has a very complex replacement for previousTab...

>+            if (((aRelatedToCurrent || aReferrerURI) == null ? aReferrerURI : aRelatedToCurrent) &&
You misunderstood my previous comment, I think - the whole line should have been (aRelatedToCurrent || aReferrerURI)

>-              // We need to explicitly clear this, because updateCurrentBrowser
>-              // doesn't get called for a background tab
>-              this.mPreviousTab = null;
Hmm... so in Firefox, if have three tabs, you select the middle tab, you open a new tab in the foreground, you close the left tab, you close the new tab, which tab becomes current?

>-                  var newTab = browser.addTab(url);
>-                  browser.selectedTab = newTab;
>+                  var newTab = browser.loadOneTab(url, {aLoadInBackground: true});
This is just completely wrong.

>+  browser.loadOneTab(aURL, {
>+                  aReferrerURI: referrer,
>+                  aCharset: originCharset,
>+                  inBackground: aLoadInBackground
>+                 });
Nit: odd indentation. I would probably put the } on the same column as the browser, and the properties with a two-space indent.

>+    browser.addTab(urlArray[i], {aAllowThirdPartyFixup: flags});
flags isn't really a boolean (this only works by accident).
Attachment #444438 - Flags: review?(neil) → review-
(In reply to comment #17)
> >+        <parameter name="aOwner"/>
> Bah, the Firefox code has a very complex replacement for previousTab...

Is that because they can open a tab next to the tab it has been opened from (depending on a pref, IIRC) instead of always to the right? If so, this is another thing that would be nice to have as an option...
(In reply to comment #17)
> (From update of attachment 444438 [details] [diff] [review])
> Some of your conversions of loadOneTab are correct, but some are wrong. Here's
> an example:
> 
> >-          var newTab = gBrowser.addTab(defaultSearchURL);
> >           var loadInBackground = Services.prefs.getBoolPref("browser.tabs.loadInBackground");
> >           if (reverseBackgroundPref)
> >             loadInBackground = !loadInBackground;
> >-          if (!loadInBackground)
> >-            gBrowser.selectedTab = newTab;
> >+          var newTab = gBrowser.loadOneTab(defaultSearchURL, {aLoadInBackground: loadInBackground});
> The old code always passed false to aFocusNewTab, to ensure that the old tab
> wasn't the owner of the new tab. But because you're using loadOneTab, this sets
> the owner of the new tab.
> 

Reverted to previous behavior.

> >+            if (((aRelatedToCurrent || aReferrerURI) == null ? aReferrerURI : aRelatedToCurrent) &&
> You misunderstood my previous comment, I think - the whole line should have
> been (aRelatedToCurrent || aReferrerURI)

Sorry, fixed.

> 
> >-              // We need to explicitly clear this, because updateCurrentBrowser
> >-              // doesn't get called for a background tab
> >-              this.mPreviousTab = null;
> Hmm... so in Firefox, if have three tabs, you select the middle tab, you open a
> new tab in the foreground, you close the left tab, you close the new tab, which
> tab becomes current?

The right tab. Actually I forgot to include pref mentioned by KaiRo to make things work as they should.
Attached patch v5 (obsolete) — Splinter Review
I used wrong names of parameters passed to functions, so things are almost broken. Introduced pref browser.tabs.insertRelatedAfterCurrent, fixed all above comments.
Attachment #444438 - Attachment is obsolete: true
Attachment #445960 - Flags: review?(neil)
Attachment #444438 - Flags: feedback?(philip.chee)
Comment on attachment 445960 [details] [diff] [review]
v5

Pass 1: code review

> diff --git a/suite/common/contentAreaClick.js b/suite/common/contentAreaClick.js
> +      var bgLoad = (event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground")));

The outer () isn't needed I think.

> diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
> -function openUILinkIn(url, where, allowThirdPartyFixup)
> +function openUILinkIn(url, where, aAllowThirdPartyFixup, aPostData, aReferrerURI)

>    var flags = allowThirdPartyFixup ? nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP :
>                                       nsIWebNavigation.LOAD_FLAGS_NONE;
> -    w.loadURI(url, null, flags);
> +    w.loadURI(url, aPostData, aAllowThirdPartyFixup);

I think this should be:

    w.loadURI(url, aPostData, flags);

Pass 2: appply patch.

A bit of fuzz when patching:

> patching file suite/common/utilityOverlay.js
> Hunk #1 succeeded at 934 (offset 5 lines).
> Hunk #2 succeeded at 1201 (offset 5 lines).
> Hunk #3 succeeded at 1244 (offset 5 lines).
> Hunk #4 succeeded at 1305 (offset 5 lines).

Middle clicking several links in a page opens each link in a tab immediately to the right of the current tab. I think new tabs should be opened at the end of the tab group.

Other than that everything appears to be working.
Attachment #445960 - Flags: feedback+
(In reply to comment #21)
> Middle clicking several links in a page opens each link in a tab immediately to
> the right of the current tab. I think new tabs should be opened at the end of
> the tab group.
> 
That behavior is controlled by browser.tabs.insertRelatedAfterCurrent, which is introduced by this patch and true by default, as in FF and looks reasonable for me. But I can change default value to false to mimic old suite.

> Other than that everything appears to be working.
Can You look at comment 15 about failing SMILE browser_Browser test ?
(In reply to comment #22)
> That behavior is controlled by browser.tabs.insertRelatedAfterCurrent, which is
> introduced by this patch and true by default, as in FF and looks reasonable for
> me. But I can change default value to false to mimic old suite.

I also think the FF default makes sense for us, but Neil should decide that. In any case, I think adding a UI pref for that would be nice, possibly in a followup.
> That behavior is controlled by browser.tabs.insertRelatedAfterCurrent, which is
> introduced by this patch and true by default, as in FF and looks reasonable for
> me. But I can change default value to false to mimic old suite.

No I mean the following. Lets say initially we have four tabs:

A[selected] B C D

In tab A I click on four links in order 1,2,3,4. The result is:

A A4 A3 A2 A1 B C D

What I was expecting is:

A A1 A2 A3 A4 B C D

The Tabs Open Relative extension does it properly (from my point of view).
Interesting. I just checked with Minefield, and apparently they have a mechanism in place that what Philip expects does work, i.e. I get A A1 A2 A3 A4 B - would be nice to have that as well (though if it's not so easy, I'm happy with doing it in a followup).
Blocks: 161836
I fixed this in my local patch, but will wait for Neil review for this one before upload new.
Attached patch v6 (obsolete) — Splinter Review
Fixed all Philip comments.
Attachment #445960 - Attachment is obsolete: true
Attachment #446964 - Flags: review?(neil)
Attachment #445960 - Flags: review?(neil)
micro-nit:

> +              this._lastRelatedIndex= this.getTabIndex(t);
Space before "= ".
Bug 465673 almost ported by this patch, only test missing, making depended. I'll add test here for review later.
Blocks: 465673
FYI:
Bug 562649 set and correctly handle userTypedValue when loading external URIs
http://hg.mozilla.org/mozilla-central/rev/f2070673fdbc
Comment on attachment 446964 [details] [diff] [review]
v6

>+        var newTab = gBrowser.loadOneTab("about:blank", {inBackground: bgLoad,
>+                                         referrerURI: referrer});
This makes no sense. Why should a blank tab have a referrer?

>+              if (!this._lastRelatedIndex)
>+                this._lastRelatedIndex = this.mTabContainer.selectedIndex;
>+              this.moveTabTo(t, ++this._lastRelatedIndex);
>+              this._lastRelatedIndex= this.getTabIndex(t);
Bah, of course moveTabTo kills this._lastRelatedIndex, but rather than using this.getTabIndex we should save it in a local variable i.e.
>var lastRelatedIndex = this._lastRelatedIndex || this.mTabContainer.selectedIndex;
>this.moveTabTo(t, ++lastRelatedIndex);
>this._lastRelatedIndex = lastRelatedIndex;
Comment on attachment 446964 [details] [diff] [review]
v6

>-      <field name="mPreviousTab">
>-        null
>-      </field>
So, I had a look at all the tab owner stuff, and I don't see any benefit of using it instead of our mPreviousTab code. I won't ask you to do that though.

>-      var tab = browser.addTab(url);
>-      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>-        browser.selectedTab = tab;
>+      var tab = browser.loadOneTab(url, {inBackground: BookmarksUtils.shouldLoadTabInBackground(aEvent)});
We can't do this because loadOneTab sets the owner, and we don't want that.

>-      tab = browser.addTab(url);
>-      if (event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
>-        browser.selectedTab = tab;
>+      var bgLoad = event.shiftKey != (pref && pref.getBoolPref("browser.tabs.loadInBackground"));
>+      tab = browser.loadOneTab(url, {inBackground: bgLoad});
Ditto.

>-    var tab = browser.addTab(url, null, null, false, flags);
>+    var tab = browser.loadOneTab(url, {
>+                referrerURI: aReferrerURI,
>+                postData: aPostData,
>+                inBackground: loadInBackground,
>+                allowThirdPartyFixup: aAllowThirdPartyFixup,
>+                relatedToCurrent: aRelatedToCurrent
>+              });
>     if (!loadInBackground) {
>-      browser.selectedTab = tab;
And again here.

>-    var tab = browser.addTab(urlArray[0], null, null, false, flags);
>+    var tab = browser.loadOneTab(urlArray[0], {allowThirdPartyFixup: allowThirdPartyFixup,
>+                                 inBackground: loadInBackground});
>     if (!loadInBackground) {
>-      browser.selectedTab = tab;
And here too.
Attachment #446964 - Flags: review?(neil) → review-
This is my diff of tabbrowser.xml with the owner tab stuff removed and the focusNewTab and mPreviousTab stuff restored, and I got carried away a bit ;-)
Attachment #450948 - Flags: review?
Comment on attachment 450948 [details] [diff] [review]
tabbrowser.xml reverted to mPreviousTab code

Bah, did you change your email address?
Attachment #450948 - Flags: review? → review?(misak.bugzilla)
(In reply to comment #33)
> This is my diff of tabbrowser.xml with the owner tab stuff removed and the
> focusNewTab and mPreviousTab stuff restored, and I got carried away a bit ;-)

I hope that doesn't make things too difficult for people trying to modify tabbrowser with their extension.
> neil@parkwaycc.co.uk      2010-06-13 13:13:52 PDT
> Bah, did you change your email address?

Yes he did ages ago. You didn't notice?

> Robert Kaiser 2010-06-13 15:39:22 PDT
>> This is my diff of tabbrowser.xml with the owner tab stuff removed and the
>> focusNewTab and mPreviousTab stuff restored, and I got carried away a bit ;-)

> I hope that doesn't make things too difficult for people trying to modify
> tabbrowser with their extension.

You could file a bug with AMO for them to use their private MXR to see how many Firefox tabbrowser extensions are using these "internal" implementation details.
Also given how often the Firefox tabbrowser changes, Extension tabbrowser extension authors are more or less used to (or at least resigned to) almost constant breakages. Not to mention a large number of promising tabbrowser extensions that have died due to the authors giving up in despair.
Attachment #450948 - Flags: review?(misak.bugzilla) → review+
Attached patch v7 (obsolete) — Splinter Review
I merged Neil's tabbrowser.xml changes into main patch and fixed all other comments, though it seems all comment #32 stuff already fixed by Neil's patch.
Attachment #446964 - Attachment is obsolete: true
Attachment #451037 - Flags: review?(neil)
(In reply to comment #38)
> I merged Neil's tabbrowser.xml changes into main patch and fixed all other
> comments, though it seems all comment #32 stuff already fixed by Neil's patch.
That's not true; although I switched owner tab stuff back to previous tab, the comments still apply, but with s/owner/previous (closing a bookmark opened in a new tab switches back to the previous tab when it shouldn't, for instance).
Fixed all Neil, comments. Also in navigator.js i used referrer for about:blank to force diverted tab from Google Reader for example opens next to it. I changed code here and used relatedtoCurrent.
Attachment #451037 - Attachment is obsolete: true
Attachment #451524 - Flags: review?(neil)
Attachment #451037 - Flags: review?(neil)
Comment on attachment 451524 [details] [diff] [review]
v8
[Checked in: Comment 42]

Ah yes, it's much clearer to use relatedToCurrent, thanks!
Attachment #451524 - Flags: review?(neil) → review+
Attachment #451524 - Flags: superreview?(neil)
Attachment #451524 - Flags: superreview?(neil) → superreview+
Pushed: http://hg.mozilla.org/comm-central/rev/3b18535c8c2c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The change to the addTab() method seems to have broken the Home Button:
<http://forums.mozillazine.org/viewtopic.php?f=6&t=1928183>

Error: arguments[1] is null
Source File: chrome://navigator/content/tabbrowser.xml
Line: 1398

You need to have more than one home page to reproduce this bug.

BrowserHome() calls gBrowser.loadGroup() which calls replaceGroup() which calls addTab("about:blank", null);

However the following check:
typeof arguments[1] == "object"
returns true since typeof null == "object"
> addTab("about:blank", null);
Should remove the bogus null passed in the second parameter as well.
Attached patch tests plus fix for the issue (obsolete) — Splinter Review
Here is one line fix for the issue Plilip Chee reaised, plus i added tests for this bug.
Attachment #453665 - Flags: superreview?(neil)
Attachment #453665 - Flags: review?(neil)
Comment on attachment 453665 [details] [diff] [review]
tests plus fix for the issue

I know Callek likes stealing my reviews ;-) If he likes the tests then they can land, but I want to check up on that addTab bug and get back to you.
Attachment #453665 - Flags: review?(neil) → review?(bugspam.Callek)
I still think we should null check the addTab(). Who knows what crazy extension authors out there (myself excluded) will do.
Comment on attachment 453665 [details] [diff] [review]
tests plus fix for the issue

>               if ("sessionHistory" in data) {
>-                this.addTab("about:blank", null);
>+                this.addTab("about:blank");
This isn't even the call that goes wrong in the reported failure case.
sr- on the (lack of) tabbrowser.xml changes

We have a couple of options:
1. Fix addTab(uri, null)
2. Change replace/appendGroup to never pass a referrer (we never give it one)
2a. Possibly change the callers of loadGroup to pass URIs instead of objects
Attachment #453665 - Flags: superreview?(neil) → superreview-
Attachment #453665 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 453665 [details] [diff] [review]
tests plus fix for the issue

r+ for Makefile and test, which can land now. The code change, as Neil says needs work.

Note for future, test is a replica of http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_relatedTabs.js
The check-in has caused the bug 573119.  Pls take a look at it.
> The check-in has caused the bug 573119.  Pls take a look at it.
Please see Comment 43
Depends on: 573119
I forgot that aRelatedToCurrent isn't actually a parameter to loadOneTab...
Attachment #455934 - Flags: review?(misak.bugzilla)
Attachment #455934 - Flags: review?(misak.bugzilla) → review+
Attached patch test plus fix v2 (obsolete) — Splinter Review
I added check in addTab for arguments[1] not be null. Carrying forward r+ from Callek for test part.
Attachment #453665 - Attachment is obsolete: true
Attachment #455935 - Flags: superreview?(neil)
Attachment #455935 - Flags: review+
Comment on attachment 455935 [details] [diff] [review]
test plus fix v2

>             if (arguments.length == 2 &&
>                 typeof arguments[1] == "object" &&
>+                arguments[1] &&
>                 !(arguments[1] instanceof Components.interfaces.nsIURI)) {
[It looks odd for these tests to be in this order. Maybe change it to arguments[1] != null && to be clear what is being tested. Or possibly move the check to before the typeof check.]
Attachment #455935 - Flags: superreview?(neil) → superreview+
Attached patch test plus fix v2 - for checkin (obsolete) — Splinter Review
Fixed check order. Carrying forward r+ from Callek and sr+ from Neil.
Attachment #450948 - Attachment is obsolete: true
Attachment #455935 - Attachment is obsolete: true
Attachment #455939 - Flags: superreview+
Attachment #455939 - Flags: review+
Attachment #455939 - Flags: approval-seamonkey2.1a2?
No longer depends on: 573119
Comment on attachment 455939 [details] [diff] [review]
test plus fix v2 - for checkin

builds have already spun; so unless this is a severe regression I don't want to take it. If you meant it as an "approval-trunk" I also don't want to take any new tests while we have no coverage.
Attachment #455939 - Flags: approval-seamonkey2.1a2? → approval-seamonkey2.1a2-
Hmm, actually, could we have that case with the second argument being null in the test, please?
(In reply to comment #57)
> Comment on attachment 455939 [details] [diff] [review]
> test plus fix v2 - for checkin
> 
> builds have already spun; so unless this is a severe regression I don't want to
> take it. If you meant it as an "approval-trunk" I also don't want to take any
> new tests while we have no coverage.

Looking at the patch that broke it (v8) reveals that openUILinkIn() in utilityOverlay.js is affected, too (same, but with arguments[2]). As is the original addTab method in FF's tabbrowser.xml (did anyone tell them?). So I'd vote for holding checking this in (the tree is still closed anyway) and for having the fix updated.
upon closing a tab, wouldn't it make more sense to make the tab to the left the current tab instead of the tab to the right?
No, at least not in LTR locales. You expect "holes" to be filled with stuff from the right, and keeping the selection on the same tab position as the old one is the least surprising way to do.
I would suggest to reopen this bug until the regression is fixed (i.e. open bookmark group in an existing window), this could avoid further duplicates.
(In reply to comment #65)
> I would suggest to reopen this bug until the regression is fixed (i.e. open
> bookmark group in an existing window), this could avoid further duplicates.

or we could file a bug, mark it as a regression blocking this bug; and dupe to there... for each regression encountered... we could also mark said bug blocking, even though *this* bug will never be blocking
aAnd I thought just landing the most recent patch would be enough. But it's right, followup fixes for regressions _should_ go into followup bugs, as in a fixed bug they easily fail to get the right attention.
(In reply to comment #67)
> aAnd I thought just landing the most recent patch would be enough.

See comment 59...
(In reply to comment #68)
> (In reply to comment #67)
> > aAnd I thought just landing the most recent patch would be enough.
> 
> See comment 59...

So, you think not fixing bookmark groups and waiting for someone to do a larger fix is the way to go? I'd vote for at least fixing this part as the patch is here and reviewed and filing a new bug for the followups
I fixed comment 59 in same way and got r+ and sr+ from Neil by IRC
Attachment #455939 - Attachment is obsolete: true
Attachment #458197 - Flags: superreview+
Attachment #458197 - Flags: review+
(In reply to comment #69)
> (In reply to comment #68)
> > (In reply to comment #67)
> > > aAnd I thought just landing the most recent patch would be enough.
> > 
> > See comment 59...
> 
> So, you think not fixing bookmark groups and waiting for someone to do a larger
> fix is the way to go?

When I wrote that comment, the tree was still closed (as I pointed out) so in that time frame it was possible to do a quick update of the patch, get a quick review etc. And given that you're talking about a "larger fix" reveals that you didn't care to check what I was talking about. Anyway, it seems at least Misak got what I meant... Thanks for the unbureaucratic solution.
Blocks: 583997
I forgot to push Neil's patch. Fixed that http://hg.mozilla.org/comm-central/rev/b4b29dbcc863
Blocks: 593683
Target Milestone: --- → seamonkey2.1a3
Attachment #455934 - Attachment description: Fix calling loadOneTab with separate arguments → Fix calling loadOneTab with separate arguments [Checked in: Comment 73]
Attachment #458197 - Attachment description: what actually checked in - test plus regression fix → what actually checked in - test plus regression fix [Checked in: Comment 71]
Attachment #451524 - Attachment description: v8 → v8 [Checked in: Comment 42]
Flags: in-testsuite+
Blocks: 198963
You need to log in before you can comment on or make changes to this bug.