Teach Open in Tabs to count to two

RESOLVED FIXED in Firefox 2 alpha2

Status

()

Firefox
Bookmarks & History
P2
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: philor, Assigned: Annie Sullivan)

Tracking

Trunk
Firefox 2 alpha2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

12.70 KB, patch
Darin Fisher
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
Assignee: nobody → annie.sullivan
Priority: -- → P2
Target Milestone: --- → Firefox 2 alpha2
(Assignee)

Comment 1

12 years ago
Created attachment 215072 [details] [diff] [review]
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.
Attachment #215072 - Flags: review?(bugs)
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.
Attachment #215072 - Flags: review?(bugs) → review-
(Assignee)

Comment 3

12 years ago
Created attachment 215078 [details] [diff] [review]
Open in tabs fixes; addresses Ben's comments
Attachment #215072 - Attachment is obsolete: true
Attachment #215078 - Flags: review?(bugs)
Comment on attachment 215078 [details] [diff] [review]
Open in tabs fixes; addresses Ben's comments

r=ben@mozilla.org
Attachment #215078 - Flags: review?(bugs) → review+
Blocks: 329949
(Assignee)

Comment 5

12 years ago
Created attachment 215154 [details] [diff] [review]
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.
Attachment #215078 - Attachment is obsolete: true
Attachment #215154 - Flags: superreview?(darin)
Attachment #215154 - Flags: review?(bugs)
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=ben@mozilla.org but note my nit!

-Ben
Attachment #215154 - Flags: review?(bugs) → review+

Comment 7

12 years ago
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
Attachment #215154 - Flags: superreview?(darin)
Attachment #215154 - Flags: superreview+
Attachment #215154 - Flags: review?(bugs)
Attachment #215154 - Flags: review+

Comment 8

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

restoring r=ben
Attachment #215154 - Flags: review?(bugs) → review+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 9

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

Comment 10

12 years ago
(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?
(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
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.