330130 - Teach Open in Tabs to count to two

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Phil Ringnalda (:philor)]

Pre-Places, the Open in Tabs menu item only appeared in bookmark folders with two or more items. You could maybe make a twisted case for tab discoverability for having it in a folder with one item, but not for having it present and not disabled in an empty folder.

Comment 1

[Annie Sullivan]

Attachment: Various open in tabs fixes

This patch has several open in tabs fixes:
1) It sets the menu selection in the command handler in case there are issues with the DOMMenuItemActive event
2) It handles openInTabs command in the BookmarksEventHandler so that the selection can be set in the menu xbl command handler.
3) It only shows "Open in Tabs" menuitem if there are 2 or more items in the menu.
4) It respects the browser.tabs.loadFolderAndReplace and browser.tabs.loadBookmarksInBackground prefs.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215072 [details] [diff] [review]
Various open in tabs fixes

>   onCommand: function BM_onCommand(event) {
>-    PlacesController.mouseLoadURI(event);
>+    if (event.target.hasAttribute("openInTabs"))
>+      PlacesController.openLinksInTabs();
>+    else
>+      PlacesController.mouseLoadURI(event);

nit: Say what each case is for in a comment. 

>Index: browser/components/places/content/commands.inc
>Index: browser/components/places/content/controller.js
>+      // Check prefs to see whether to open over existing tabs.
>+      var doReplace = getBoolPref("browser.tabs.loadFolderAndReplace");
>+      var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground");
>+      // Get the start index to open tabs at
>+      var browser  = this.browserWindow.document.getElementById("content");
>+      var tabPanels = browser.browsers;
>+      var tabCount  = tabPanels.length;

nit: too many spaces on the browser line, and the tabCount line. 

also, do var browser = this.browserWindow.getBrowser();

>+      if (doReplace)
>+        index0 = 0;
>+      else {
>+        for (index0 = tabCount - 1; index0 >= 0; --index0)
>+          if (browser.browsers[index0].webNavigation.currentURI.spec != "about:blank")
>+            break;
>+        ++index0;
>+      }

if (browser.browsers[index0].currentURI.spec != "about:blank") {
...
}

... i.e. no need to go through webNavigation.

Replace index0 with firstIndex. 

Add a comment saying what this does and what pref it maps to. 

>+        if (this.nodeIsURI(childNode)) {
>+          if (index < tabCount)
>+            tabPanels[index].loadURI(childNode.uri);
>+          else
>+            browser.addTab(childNode.uri);
>+          ++index;

Ditto with the comment. 

>+      // If the bookmark group was completely invalid, just bail.
>+      if (index == index0)
>+        return;

How does index == index0 imply invalid? Say so. 

>Index: browser/components/places/content/menu.xml
>-          if (event.target.node == null && event.target.hasAttribute("command"))
>+          if (event.target.node == null)

nit: if (event.target.node) {

unless you mean === null, which you usually don't.

Comment 3

[Annie Sullivan]

Attachment: Open in tabs fixes; addresses Ben's comments

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215078 [details] [diff] [review]
Open in tabs fixes; addresses Ben's comments

r=[e-mail]

Comment 5

[Annie Sullivan]

Attachment: Open in tabs fixes; fixes mac-only javascript error

When I was testing the last patch, I merged and got the assert changes.  It turns out that debug.js is #included in globalOverlay.js which is src-ed in global-scrips.inc which is #included in both browser.js and macBrowserOverlay.xul.  This causes const variables to be defined twice on mac, which results in JavaScript errors that cause lots of things in the UI to break.  So I moved the #include debug.js from globalOverlay.js to browser.js so that it's only included once.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215154 [details] [diff] [review]
Open in tabs fixes; fixes mac-only javascript error

>-          if (event.target.node == null && event.target.hasAttribute("command"))
>+          if (event.target.node == null)

r=[e-mail] but note my nit!

-Ben

Comment 7

[Darin Fisher]

Comment on attachment 215154 [details] [diff] [review]
Open in tabs fixes; fixes mac-only javascript error

>Index: browser/components/places/content/controller.js

>+      // focus the first tab if prefs say to
>+      if (!loadInBackground || doReplace) {
>+        // Select the first tab in the group.
>+        // Set newly selected tab after quick timeout, otherwise hideous focus problems
>+        // can occur because new presshell is not ready to handle events
>+        function selectNewForegroundTab(browser, tab) {
>+          browser.selectedTab = tab;
>+        }
>+        var tabs = browser.mTabContainer.childNodes;
>+        setTimeout(selectNewForegroundTab, 0, browser, tabs[firstIndex]);
>+      }
>+
>+      // Close any remaining open tabs that are left over.
>+      // (Always skipped when we append tabs)
>+      for (var i = tabCount - 1; i >= index; --i)
>+        browser.removeTab(tabs[i]);
>+
>+      // and focus the content
>+      this.browserWindow.content.focus();

The focus logic seems non-obvious to me, but that's probably because
I'm not so familiar with front-end code.  It might help to document
why it is important to "focus the content" synchronously here.


>Index: toolkit/content/globalOverlay.js
...
>-#include debug.js

Please file a follow-up bug to figure out how to best make debug.js
appear everywhere :-)


sr=darin

Comment 8

[Darin Fisher]

Comment on attachment 215154 [details] [diff] [review]
Open in tabs fixes; fixes mac-only javascript error

restoring r=ben

Comment 9

[PikeUK]

There's a comment in commands.inc that still references the now-removed placesCmd_open:tabsEnabled command.

Comment 10

[Nickolay_Ponomarev]

(In reply to comment #7)
> >Index: toolkit/content/globalOverlay.js
> ...
> >-#include debug.js
> 
> Please file a follow-up bug to figure out how to best make debug.js
> appear everywhere :-)
> 
Was this bug ever filed?

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> (In reply to comment #7)
> > >Index: toolkit/content/globalOverlay.js
> > ...
> > >-#include debug.js
> > 
> > Please file a follow-up bug to figure out how to best make debug.js
> > appear everywhere :-)
> > 
> Was this bug ever filed?
> 
Couldn't find anything so I've filed bug 343165

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv