Last Comment Bug 558614 - Implement Firefox TabBrowser API: loadTabs(), loadOneTab() and change addTab() to get feature parity
: Implement Firefox TabBrowser API: loadTabs(), loadOneTab() and change addTab(...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: seamonkey2.1a3
Assigned To: Misak Khachatryan
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
: 156264 573119 578676 579324 (view as bug list)
Depends on: 558589
Blocks: SMtabAPI 161836 198963 465673 544170 558996 564677 583997 593683
  Show dependency treegraph
 
Reported: 2010-04-10 20:20 PDT by Serge Gautherie (:sgautherie)
Modified: 2010-12-05 06:05 PST (History)
19 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
patch (5.59 KB, patch)
2010-04-15 07:25 PDT, Misak Khachatryan
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review
with updated callers (12.24 KB, patch)
2010-04-16 05:25 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
with updated callers, fixed (12.22 KB, patch)
2010-04-16 06:21 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
v2 (13.09 KB, patch)
2010-04-18 10:41 PDT, Misak Khachatryan
philip.chee: feedback-
Details | Diff | Splinter Review
v3 (22.70 KB, patch)
2010-05-09 05:53 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
v4 (25.58 KB, patch)
2010-05-10 02:12 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
v4a (25.64 KB, patch)
2010-05-10 11:29 PDT, Misak Khachatryan
neil: review-
Details | Diff | Splinter Review
v5 (26.70 KB, patch)
2010-05-18 07:51 PDT, Misak Khachatryan
philip.chee: feedback+
Details | Diff | Splinter Review
v6 (26.87 KB, patch)
2010-05-23 09:22 PDT, Misak Khachatryan
neil: review-
Details | Diff | Splinter Review
tabbrowser.xml reverted to mPreviousTab code (10.32 KB, patch)
2010-06-13 13:12 PDT, neil@parkwaycc.co.uk
misak.bugzilla: review+
Details | Diff | Splinter Review
v7 (22.09 KB, patch)
2010-06-14 07:36 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
v8 [Checked in: Comment 42] (20.36 KB, patch)
2010-06-16 01:11 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
tests plus fix for the issue (6.12 KB, patch)
2010-06-23 23:55 PDT, Misak Khachatryan
bugspam.Callek: review+
neil: superreview-
Details | Diff | Splinter Review
Fix calling loadOneTab with separate arguments [Checked in: Comment 73] (1.45 KB, patch)
2010-07-04 02:14 PDT, neil@parkwaycc.co.uk
misak.bugzilla: review+
Details | Diff | Splinter Review
test plus fix v2 (5.84 KB, patch)
2010-07-04 02:49 PDT, Misak Khachatryan
misak.bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
test plus fix v2 - for checkin (5.99 KB, patch)
2010-07-04 04:48 PDT, Misak Khachatryan
misak.bugzilla: review+
misak.bugzilla: superreview+
bugspam.Callek: approval‑seamonkey2.1a2-
Details | Diff | Splinter Review
what actually checked in - test plus regression fix [Checked in: Comment 71] (6.70 KB, patch)
2010-07-18 11:55 PDT, Misak Khachatryan
misak.bugzilla: review+
misak.bugzilla: superreview+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2010-04-10 20:20:03 PDT
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).
Comment 1 Robert Kaiser 2010-04-12 08:30:11 PDT
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.
Comment 2 Misak Khachatryan 2010-04-15 07:25:27 PDT
Created attachment 439231 [details] [diff] [review]
patch

This patch implements loadOneTab and loadTabs, as well as modifies parameters for addTab.
Comment 3 Serge Gautherie (:sgautherie) 2010-04-15 12:32:11 PDT
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?
Comment 4 Robert Kaiser 2010-04-15 14:50:32 PDT
(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.
Comment 5 Misak Khachatryan 2010-04-16 05:25:18 PDT
Created attachment 439508 [details] [diff] [review]
with updated callers

My first plan was to get code review, then update callers, but since they are few, i modified them.
Comment 6 Misak Khachatryan 2010-04-16 06:21:35 PDT
Created attachment 439518 [details] [diff] [review]
with updated callers, fixed

same as above, fixed some typos
Comment 7 neil@parkwaycc.co.uk 2010-04-18 07:23:20 PDT
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.
Comment 8 Robert Kaiser 2010-04-18 07:29:40 PDT
(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 ;-)
Comment 9 Misak Khachatryan 2010-04-18 10:41:36 PDT
Created attachment 439793 [details] [diff] [review]
v2

fixed all comments, added Flags and FocusNewTab to the params. loadOneTab uses FocusNewTab.
Comment 10 Robert Kaiser 2010-04-20 04:59:05 PDT
Philip, be careful about resetting assignees when switching components (that's checked by default but not always wanted!)
Comment 11 Misak Khachatryan 2010-05-06 23:21:41 PDT
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.
Comment 12 Philip Chee 2010-05-08 09:05:01 PDT
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.
Comment 13 Misak Khachatryan 2010-05-09 05:53:08 PDT
Created attachment 444303 [details] [diff] [review]
v3

Fixed all stuff that Philip pointed, requesting feedback and review again.
Comment 14 neil@parkwaycc.co.uk 2010-05-09 08:28:34 PDT
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.
Comment 15 Misak Khachatryan 2010-05-10 02:12:06 PDT
Created attachment 444368 [details] [diff] [review]
v4

(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.
Comment 16 Misak Khachatryan 2010-05-10 11:29:00 PDT
Created attachment 444438 [details] [diff] [review]
v4a

Sorry for spamming with patches, previous one misses one line in loadOnetab. Also i changed bug summary to reflect all work that going here.
Comment 17 neil@parkwaycc.co.uk 2010-05-16 11:51:20 PDT
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).
Comment 18 Robert Kaiser 2010-05-16 13:40:57 PDT
(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...
Comment 19 Misak Khachatryan 2010-05-17 01:58:19 PDT
(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.
Comment 20 Misak Khachatryan 2010-05-18 07:51:51 PDT
Created attachment 445960 [details] [diff] [review]
v5

I used wrong names of parameters passed to functions, so things are almost broken. Introduced pref browser.tabs.insertRelatedAfterCurrent, fixed all above comments.
Comment 21 Philip Chee 2010-05-19 03:52:25 PDT
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.
Comment 22 Misak Khachatryan 2010-05-19 06:15:52 PDT
(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 ?
Comment 23 Robert Kaiser 2010-05-19 06:44:51 PDT
(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.
Comment 24 Philip Chee 2010-05-19 07:42:13 PDT
> 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).
Comment 25 Robert Kaiser 2010-05-19 07:59:31 PDT
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).
Comment 26 Misak Khachatryan 2010-05-19 22:46:41 PDT
I fixed this in my local patch, but will wait for Neil review for this one before upload new.
Comment 27 Misak Khachatryan 2010-05-23 09:22:20 PDT
Created attachment 446964 [details] [diff] [review]
v6

Fixed all Philip comments.
Comment 28 Philip Chee 2010-05-23 10:41:54 PDT
micro-nit:

> +              this._lastRelatedIndex= this.getTabIndex(t);
Space before "= ".
Comment 29 Misak Khachatryan 2010-05-24 03:33:49 PDT
Bug 465673 almost ported by this patch, only test missing, making depended. I'll add test here for review later.
Comment 30 Philip Chee 2010-06-09 00:51:15 PDT
FYI:
Bug 562649 set and correctly handle userTypedValue when loading external URIs
http://hg.mozilla.org/mozilla-central/rev/f2070673fdbc
Comment 31 neil@parkwaycc.co.uk 2010-06-13 12:33:12 PDT
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 32 neil@parkwaycc.co.uk 2010-06-13 12:38:16 PDT
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.
Comment 33 neil@parkwaycc.co.uk 2010-06-13 13:12:19 PDT
Created attachment 450948 [details] [diff] [review]
tabbrowser.xml reverted to mPreviousTab code

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 ;-)
Comment 34 neil@parkwaycc.co.uk 2010-06-13 13:13:52 PDT
Comment on attachment 450948 [details] [diff] [review]
tabbrowser.xml reverted to mPreviousTab code

Bah, did you change your email address?
Comment 35 Robert Kaiser 2010-06-13 15:39:22 PDT
(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.
Comment 36 Philip Chee 2010-06-13 18:03:52 PDT
> 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.
Comment 37 Philip Chee 2010-06-13 18:06:42 PDT
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.
Comment 38 Misak Khachatryan 2010-06-14 07:36:26 PDT
Created attachment 451037 [details] [diff] [review]
v7

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.
Comment 39 neil@parkwaycc.co.uk 2010-06-14 07:44:47 PDT
(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).
Comment 40 Misak Khachatryan 2010-06-16 01:11:26 PDT
Created attachment 451524 [details] [diff] [review]
v8
[Checked in: Comment 42]

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.
Comment 41 neil@parkwaycc.co.uk 2010-06-17 03:58:51 PDT
Comment on attachment 451524 [details] [diff] [review]
v8
[Checked in: Comment 42]

Ah yes, it's much clearer to use relatedToCurrent, thanks!
Comment 42 Misak Khachatryan 2010-06-17 08:50:19 PDT
Pushed: http://hg.mozilla.org/comm-central/rev/3b18535c8c2c
Comment 43 Philip Chee 2010-06-21 11:27:15 PDT
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"
Comment 44 Philip Chee 2010-06-21 11:37:17 PDT
> addTab("about:blank", null);
Should remove the bogus null passed in the second parameter as well.
Comment 45 Misak Khachatryan 2010-06-23 23:55:56 PDT
Created attachment 453665 [details] [diff] [review]
tests plus fix for the issue

Here is one line fix for the issue Plilip Chee reaised, plus i added tests for this bug.
Comment 46 neil@parkwaycc.co.uk 2010-06-24 03:36:18 PDT
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.
Comment 47 Philip Chee 2010-06-24 03:45:55 PDT
I still think we should null check the addTab(). Who knows what crazy extension authors out there (myself excluded) will do.
Comment 48 neil@parkwaycc.co.uk 2010-06-24 12:02:13 PDT
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
Comment 49 Justin Wood (:Callek) (Away until Aug 29) 2010-06-28 21:13:20 PDT
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
Comment 50 doctor__j 2010-06-30 04:41:06 PDT
The check-in has caused the bug 573119.  Pls take a look at it.
Comment 51 Philip Chee 2010-06-30 04:52:14 PDT
> The check-in has caused the bug 573119.  Pls take a look at it.
Please see Comment 43
Comment 52 neil@parkwaycc.co.uk 2010-07-04 02:14:47 PDT
Created attachment 455934 [details] [diff] [review]
Fix calling loadOneTab with separate arguments
[Checked in: Comment 73]

I forgot that aRelatedToCurrent isn't actually a parameter to loadOneTab...
Comment 53 Misak Khachatryan 2010-07-04 02:49:46 PDT
Created attachment 455935 [details] [diff] [review]
test plus fix v2

I added check in addTab for arguments[1] not be null. Carrying forward r+ from Callek for test part.
Comment 54 neil@parkwaycc.co.uk 2010-07-04 03:17:55 PDT
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.]
Comment 55 Misak Khachatryan 2010-07-04 04:48:20 PDT
Created attachment 455939 [details] [diff] [review]
test plus fix v2 - for checkin

Fixed check order. Carrying forward r+ from Callek and sr+ from Neil.
Comment 56 Misak Khachatryan 2010-07-04 04:57:05 PDT
*** Bug 573119 has been marked as a duplicate of this bug. ***
Comment 57 Justin Wood (:Callek) (Away until Aug 29) 2010-07-04 11:32:35 PDT
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.
Comment 58 Robert Kaiser 2010-07-13 12:14:38 PDT
Hmm, actually, could we have that case with the second argument being null in the test, please?
Comment 59 Jens Hatlak (:InvisibleSmiley) 2010-07-13 12:20:23 PDT
(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.
Comment 60 Robert Kaiser 2010-07-14 15:55:48 PDT
*** Bug 578676 has been marked as a duplicate of this bug. ***
Comment 61 lode_leroy 2010-07-15 00:12:41 PDT
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?
Comment 62 Karsten Düsterloh 2010-07-15 01:01:40 PDT
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.
Comment 63 Robert Longson 2010-07-16 07:00:36 PDT
*** Bug 579324 has been marked as a duplicate of this bug. ***
Comment 64 Misak Khachatryan 2010-07-16 21:31:19 PDT
*** Bug 579324 has been marked as a duplicate of this bug. ***
Comment 65 OstGote! 2010-07-18 06:16:01 PDT
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.
Comment 66 Justin Wood (:Callek) (Away until Aug 29) 2010-07-18 07:00:08 PDT
(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
Comment 67 Robert Kaiser 2010-07-18 07:27:33 PDT
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.
Comment 68 Jens Hatlak (:InvisibleSmiley) 2010-07-18 11:01:48 PDT
(In reply to comment #67)
> aAnd I thought just landing the most recent patch would be enough.

See comment 59...
Comment 69 Robert Kaiser 2010-07-18 11:10:47 PDT
(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
Comment 70 Misak Khachatryan 2010-07-18 11:55:29 PDT
Created attachment 458197 [details] [diff] [review]
what actually checked in - test plus regression fix
[Checked in: Comment 71]

I fixed comment 59 in same way and got r+ and sr+ from Neil by IRC
Comment 71 Misak Khachatryan 2010-07-18 12:00:25 PDT
Pushed http://hg.mozilla.org/comm-central/rev/e7d118a232ab
Comment 72 Jens Hatlak (:InvisibleSmiley) 2010-07-18 13:35:37 PDT
(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.
Comment 73 Misak Khachatryan 2010-08-10 08:16:02 PDT
I forgot to push Neil's patch. Fixed that http://hg.mozilla.org/comm-central/rev/b4b29dbcc863
Comment 74 Philip Chee 2010-11-02 01:03:27 PDT
*** Bug 156264 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.