330929 - "bookmark all tabs" broken

Status: RESOLVED FIXED

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

Description

[Grayson Mixon (ColdFusion650)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060318 Firefox/2.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060318 Firefox/2.0a1

when choosing file>bookmark all tabs, no dialog comes up to select the folder to put them in, and they do not get bookmarked. in fact, nothing happens at all. there are no js error messages, even with chrome errors turned on and javascript in strict mode.

Reproducible: Always

Steps to Reproduce:
1.open a bunch of tabs
2.select file>bookmark all tabs
3.prepare for a very anti-climactic nothing

Actual Results:  
nothing happens

Expected Results:  
a dialog appears that allows you to select a folder to store the bookmarks, and then the tabs are bookmarked in that folder.

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060318 Firefox/1.6a1 ID:2006031801 [cairo]

confirmed

Comment 2

[Joe Hughes]

Attachment: Implements "Bookmark All Tabs..."

Comment 3

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

Comment on attachment 216873 [details] [diff] [review]
Implements "Bookmark All Tabs..."

>Index: browser/base/content/browser.js
>+/**
>+ * This function returns a list of nsIURI objects characterizing the
>+ * tabs currently open in the given browser.  The URIs will appear in the
>+ * list in the order in which their corresponding tabs appeared.  However,
>+ * only the first instance of each URI will be returned.
>+ *
>+ * @param aTabBrowser  the tabBrowser to get the contents of
>+ *
>+ * @returns a list of nsIURI objects representing unique locations open
>+ */
>+
>+function getUniqueTabInfo(aTabBrowser)

Expose this as a private method on BookmarkAllTabsCommand's prototype. All my standard nits apply. 

>Index: browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties
>+dialogAcceptLabelAddMulti=Add Bookmarks

As a rule, the title of the dialog should more closely match the command that invoked it... so the dialog should be titled "Bookmark All Tabs"

>   _isDeletePossible: function BPP__isDeletePossible() {
>     switch(this._variant) {
>     case this.EDIT_HISTORY_VARIANT: return false;
>     case this.ADD_BOOKMARK_VARIANT: return false;
>+    case this.ADD_MULTIPLE_BOOKMARKS_VARIANT: return false;
>     case this.EDIT_FOLDER_VARIANT:  return false;
>     default: return true;
>     }
>   },

I thought I'd just comment, the style you use here (and throughout this file) can make it harder to follow what's going on. If I were writing this I'd write:

switch(this._variant) {
case this.EDIT_HISTORY_VARIANT:
case this.ADD_BOOKMARK_VARIANT:
case this.ADD_MULTIPLE_BOOKMARKS_VARIANT:
case this.EDIT_FOLDER_VARIANT:
  return false;
}
return true;

Having the body of a case on the same line is unusual at least in mozilla code. 

>+      if (title.length > 100) {
>+          title = title.substr(0, 100);
>+      }

nit: watch your indentation. 

>-    else {
>+    else if (this._identifierIsFolderID()){
>       this._folderId = identifier;
>+    } else if (this._isVariant(this.ADD_MULTIPLE_BOOKMARKS_VARIANT)) {
>+      this._URIList = identifier;

nit: Newline before else if

>Index: browser/components/places/content/controller.js
>+ * Create a new transaction that represents the one-step creation of a
>+ * new folder and population of said folder with a set of bookmarks.
>+ * This is used to implement the "Bookmark All Tabs..." operation.
>+ * We can't do this as an aggregate transaction because the operations for
>+ * adding the bookmarks to the folder need to know the ID of the new folder,
>+ * which we can't know before actually creating the new folder.
>+ *
>+ * @param name string for the name of the new folder
>+ * @param parentID an integer representing the ID of the parent folder
>+ *                 into which we'll place the new folder

In my files at least I prefer to put the documentation on a new line from the parameter name, since it makes everything line up better:

 @param parentID
        an integer representing the ID of ...

Not sure how parsers for this sort of thing work though. Do they consume all whitespace after the identifier name or just the first space?

>+function PlacesCreatePopulatedFolderTransaction(name, parentID, bookmarkList) {
>+  this._name = name;
>+  this._bookmarkList = bookmarkList;
>+  this._parentID = parentID;
>+  this._id = null;
>+  this._oldTitles = []; // list of previous titles for existing bookmarks
>+  this.redoTransaction = this.doTransaction;
>+}

Why isn't this done using a PAT of existing unit transactions? I see that you save existing titles for items, but shouldn't CreateItemTxn take care of that anyway?

Comment 4

[Joe Hughes]

Attachment: Update to address most of Ben's comments

Incorporated most of your feedback.  I'm going to leave the javadocs as-is for now--I want to run tests with Doxygen first before I go reformat all my comments to your proposed style.

As for your last comment, that very question is addressed in the documentation for that transaction. :]

Comment 5

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

Comment on attachment 217058 [details] [diff] [review]
Update to address most of Ben's comments

Look at how PlacesCreateFolderTransaction solves this problem. It's almost exactly the same:

var folderTxn = new PlacesCreateFolderTransaction("Foo", bms.toolbarRoot, -1);
folderTxn.childTransactions.push(new PlacesCreateItemTransaction("Item 1", -1, -1));
folderTxn.childTransactions.push(new PlacesCreateItemTransaction("Item 2", -1, -1));
folderTxn.childTransactions.push(new PlacesCreateItemTransaction("Item 3", -1, -1));
PlacesController.tm.doTransaction(folderTxn);

See: http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/controller.js#1697

for more info

Comment 6

[Joe Hughes]

Good point--I hadn't stumbled upon that feature.  Filed related bug 332629 to make that code clearer.

Comment 7

[Joe Hughes]

Attachment: Adjusted to use PlacesCreateFolderTransaction's child transaction feature

Comment 8

[Joe Hughes]

Attachment: Added one more stray comma, and fixed an unrelated fencepost error in PAT.undoTransaction() in controller.js

Comment 9

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

Comment on attachment 217098 [details] [diff] [review]
Added one more stray comma, and fixed an unrelated fencepost error in PAT.undoTransaction() in controller.js

>+      } else if (this._isVariant(this.ADD_MULTIPLE_BOOKMARKS_VARIANT)) {

nit: newline after }

sr=[e-mail]

Comment 10

[Joe Hughes]

Landed on branch & trunk.

Comment 11

[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