335512 - Import Bookmarks from file is broken

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Nikola Kocić]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060425 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060425 Minefield/3.0a1

When trying to import bookmarks from file, Import Wizard closes.

Reproducible: Always

Steps to Reproduce:
1.Bookmarks
2.Organize Bookmarks...
3.File
4.Import...
5.From File
Actual Results:  
Import Wizard closes without asking which file to import.

Expected Results:  
Import Wizard should ask which file to import.

Comment 1

[hhh]

WFM

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2 ID:2006030804

Comment 2

[Brian Polidoro]

The error I get in the JS console is 
Error: this._migrator has no properties
Source File: chrome://browser/content/migration/migration.js
Line: 165

Comment 3

[Brett Wilson]

(In reply to comment #1)
> WFM
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308
> Firefox/1.5.0.2 ID:2006030804

You're using 1.5. This bug is in places (trunk).

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

See also bug 335588 - looks like this bug affects migrating from other apps too.

Comment 5

[Adam Guthrie]

The error in comment 2 is totally unrelated to this, and the result of some hackery in wizard.xul's cancel method.

migration.js is set to bail if the user chooses to import from a file: http://lxr.mozilla.org/seamonkey/source/browser/components/migration/content/migration.js#149

This is handled properly in bookmarks.js: http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#823

Once the wizard is closed it checks to see if the user chose to import from a file, then calls the importBookmarksFromFile method in bookmarks.js.

So basically, this needs to be handled similarly somewhere in places.js.

Comment 6

[Adam Guthrie]

*** Bug 346745 has been marked as a duplicate of this bug. ***

Comment 7

[Joey Minta]

Attachment: check .fromFile

I hit this while trying to find a nice way to test out some other patches.
Patch adds a check to make sure we really have a _migrator, and then watches for .fromFile, since the migration wizard assumes that its opener will take care of that type of migration.

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 252130 [details] [diff] [review]
check .fromFile

hey joey, thanks for working on this!

some comments:

1)

+    if (this._migrator) {
+      var sourceProfiles = this._migrator.sourceProfiles;
+      var count = sourceProfiles.Count();
+      for (var i = 0; i < count; ++i) {
+        var item = document.createElement("radio");
+        var str = sourceProfiles.QueryElementAt(i, Components.interfaces.nsISupportsString);
+        item.id = str.data;
+        item.setAttribute("label", str.data);
+        profiles.appendChild(item);
+      }

Can you add a comment to this code elaborate on when _migrator is not defined, and why that's ok?

2)

This looks like the importBookmarks() function from bookmarks.js

http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#913


   importBookmarks: function PO_import() {
     var features = "modal,centerscreen,chrome,resizable=no";
+    window.fromFile = false;
     openDialog("chrome://browser/content/migration/migration.xul",
                "", features, "bookmarks");
+    if (window.fromFile) {
+      this.importFromFile();
+    }
+  },

Can you port over this comment from bookmarks.js?

  // XXX: ifdef it to be non-modal (non-"sheet") on mac (see bug 259039)
  var features = "modal,centerscreen,chrome,resizable=no";

(hmm, should the 2nd argument to openDialog be "migration"?

From bookmarks.js:

918       window.openDialog("chrome://browser/content/migration/migration.xul", "migration", features, "bookmarks");

3)

+  importFromFile: function PO_importFromFile() {
+    var fp = Cc["@mozilla.org/filepicker;1"].
+            createInstance(Ci.nsIFilePicker);
+    fp.init(window, "Import", Ci.nsIFilePicker.modeOpen);
+    fp.appendFilters(Ci.nsIFilePicker.filterHTML | Ci.nsIFilePicker.filterAll);
+    if (fp.show() != Ci.nsIFilePicker.returnCancel) {
+      var ioService = Cc["@mozilla.org/network/io-service;1"].
+                      getService(Ci.nsIIOService);
+      var bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+                getService(Ci.nsINavBookmarksService);
+      if (fp.file) {
+        bms.importBookmarksHTML(ioService.newFileURI(fp.file));
+      }
+    }
   },

Are you passing in "Import" as the hard coded title?


comparing that to importBookmarksFromFile() in bookmarks.js, I think we want to do something more like:

933       const kTitle = BookmarksUtils.getLocaleString("SelectImport");
934       kFilePicker.init(window, kTitle, kFilePickerIID["modeOpen"]);

Comment 9

[(not reading, please use [e-mail] instead)]

joey, you will probably want to run this by asaf's or gavin's feedback, as they are browser code peers (http://www.mozilla.org/projects/firefox/review.html)

Comment 11

[Joey Minta]

Attachment: check .fromFile v2

This version incorporates all of seth's comments.

Comment 12

[Joey Minta]

Comment on attachment 254975 [details] [diff] [review]
check .fromFile v2

Shoot, ignore the bookmarks-panel changes.  I don't know how those got in there.

Comment 13

[Joey Minta]

Attachment: check .fromFile v3

Mano asked me to look more closely at the this._migrator error/comment.  Turns out this error exists in legacy bookmarks too on trunk.  This version updates the comment to properly reflect that (and removes the unneeded bookmarks-panel changes).

Comment 14

[Asaf Romano (gone)]

Comment on attachment 255038 [details] [diff] [review]
check .fromFile v3

>Index: browser/components/places/content/places.js
>===================================================================
>@@ -245,28 +245,59 @@ var PlacesOrganizer = {
>   getCurrentOptions: function PO_getCurrentOptions() {
>     return asQuery(this._content.getResult().root).queryOptions;
>   },
>   
>   /**
>    * Show the migration wizard for importing from a file.
>    */
>   importBookmarks: function PO_import() {
>+    // XXX: ifdef it to be non-modal (non-"sheet") on mac (see bug 259039)
>     var features = "modal,centerscreen,chrome,resizable=no";
>+    

nit: remove trailing whitespace.

>+    // The migrator window will set this to true when it closes, if the user
>+    // chose to migrate from a specific file.
>+    window.fromFile = false;
>     openDialog("chrome://browser/content/migration/migration.xul",
>-               "", features, "bookmarks");
>+               "migration", features, "bookmarks");
>+    if (window.fromFile) {
>+      try {
>+        this.importFromFile();
>+      } catch(ex) {
>+        alert(ex);
>+      }

Any reason for this to ever throw?

>+    }
>+  },
>+
>+  /**
>+   * Open a file-picker and import the selected file into the bookmarks store
>+   */
>+  importFromFile: function PO_importFromFile() {
>+    var fp = Cc["@mozilla.org/filepicker;1"].
>+            createInstance(Ci.nsIFilePicker);

nit: fix indentation.

>+    fp.init(window, PlacesUtils.getString("SelectImport"), Ci.nsIFilePicker.modeOpen);
>+    fp.appendFilters(Ci.nsIFilePicker.filterHTML | Ci.nsIFilePicker.filterAll);
>+    if (fp.show() != Ci.nsIFilePicker.returnCancel) {
>+      var ioService = Cc["@mozilla.org/network/io-service;1"].
>+                      getService(Ci.nsIIOService);
>+      var bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                getService(Ci.nsINavBookmarksService);

use PlacesUtils.bookmarks

>+      if (fp.file) {
>+        bms.importBookmarksHTML(ioService.newFileURI(fp.file));
>+      }

nit: remove braces.

Comment 15

[Joey Minta]

Attachment: addresses nits

Addresses all the nits from version 3

Comment 16

[Asaf Romano (gone)]

Comment on attachment 255043 [details] [diff] [review]
addresses nits

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

>+  /**
>+   * Open a file-picker and import the selected file into the bookmarks store
>+   */
>+  importFromFile: function PO_importFromFile() {
>+    var fp = Cc["@mozilla.org/filepicker;1"].
>+             createInstance(Ci.nsIFilePicker);
>+    fp.init(window, PlacesUtils.getString("SelectImport"), Ci.nsIFilePicker.modeOpen);

please try to keep lines no longer than 80 characters.

>   /**
>    * Allows simple exporting of bookmarks.
>    */
>   exportBookmarks: function PO_export() {
>     var fp = Cc["@mozilla.org/filepicker;1"].
>             createInstance(Ci.nsIFilePicker);
>-    fp.init(window, "", Ci.nsIFilePicker.modeSave);
>+    fp.init(window, PlacesUtils.getString("EnterExport"), Ci.nsIFilePicker.modeSave);

ditto.

>     fp.appendFilters(Ci.nsIFilePicker.filterHTML);
>     fp.defaultString = "bookmarks.html";
>     if (fp.show() != Ci.nsIFilePicker.returnCancel) {
>       var bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>                 getService(Ci.nsINavBookmarksService);

please change this to use PlacesUtils.bookmarks as well.

r=mano otherwise.

Comment 17

[Joey Minta]

Patch checked in.
Checking in browser/components/migration/content/migration.js;
/cvsroot/mozilla/browser/components/migration/content/migration.js,v  <--  migration.js
new revision: 1.34; previous revision: 1.33
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.78; previous revision: 1.77
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.17; previous revision: 1.16
done

Comment 18

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091411 Minefield/3.0a8pre ID:2007091411

VERIFIED

Comment 19

[Tracy Walker [:tracy]]

verified with - Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091404 Minefield/3.0a8pre

Comment 20

[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