505192 - Chrome profile migration for Import Wizard and Migration Assistant - cookies, history, and bookmarks

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[jobin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1

there is no option to import data from browsers like google chrome...

Reproducible: Always

Comment 1

[Jo Hermans]

That's because you need to export them first from Chrome. Firefox can't access the bookmarks directly from Chrome's profile. IMHO, the File->Import menu (which imports not only bookmarks, but all settings) is becoming less and less usable. 

https://support.mozilla.com/en-US/kb/Importing+bookmarks+from+Google+Chrome

Comment 2

[Chris Lawson (gone)]

This is a lot worse on the Mac, since Chrome on Mac doesn't have the ability to export bookmarks (yet?) at all.

The format looks clean enough; it might be worth writing an importer for this.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: WIP

Comment 4

[Makoto Kato [:m_kato]]

Attachment: WIP

Comment 6

[Makoto Kato [:m_kato]]

Attachment: WIP v2

Comment 8

[imradyurrad]

So how goes the progress?

Comment 9

[Frank Yan (:fryn)]

(In reply to comment #8)
> So how goes the progress?

Feel free to make a patch. They are always welcome ;)

Comment 10

[richardus]

(In reply to comment #2)
> This is a lot worse on the Mac, since Chrome on Mac doesn't have the ability to
> export bookmarks (yet?) at all.

If anyone CCed is still stuck on this bug Google Chrome on Mac now supports exporting bookmarks.

Comment 11

[Makoto Kato [:m_kato]]

(In reply to comment #10)
> (In reply to comment #2)
> > This is a lot worse on the Mac, since Chrome on Mac doesn't have the ability to
> > export bookmarks (yet?) at all.
> 
> If anyone CCed is still stuck on this bug Google Chrome on Mac now supports
> exporting bookmarks.

This isn't blocker for 4.0.  I don't post new patch for all platform support yet.
After post 4.0 tree is opened, I will send a review for all platform code.

Comment 12

[Dietrich Ayala (:dietrich)]

This is great work so far Makoto, thanks for working on it. It'd be great to see this as an extension, but from looking at the code, not sure that's possible :( Bummer, because I'd love to see Firefox 4 be able to do this.

Comment 13

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 519363 [details] [diff] [review]
fix v1


some drive-by comments:

>+function readJSONfile(path)

See https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm for ways to do this asynchronously.

>+function insertBookmarkItems(folderId, items) {
>+  items.forEach(function (item) {
>+    try {
...
>+    } catch (e) {
>+      // ignore error
>+    }
>+  });
>+}

Don't swallow the errors here. Should report via Component.utils.reportError maybe?

;
>+      try {
>+        PlacesUtils.history.runInBatchMode({
>+          _self : this,
>+          runBatched : function (aUserData) {

can you split the actual import out into a separate method?

>+        }, null);
>+      } catch (e) {
>+        // ignore error
>+      }

ditto wrt hiding errors

>+    if (aItems & this.COOKIES) { 
>+      // Copy cookie

ditto wrt to splitting it out into a separate method, and hiding errors.

switching to Marco for first review, since he's probably got a much smaller queue than Gavin. Can have Gavin do a final pass.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Thanks for doing this in JS - we really should port our other migrators too :)

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #15)
> Thanks for doing this in JS - we really should port our other migrators too :)

Or rip them out.  A few of them are ridiculously ancient.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 519363 [details] [diff] [review]
fix v1

Yes, doing this JS is awesome.

Migrators revise plan should probably consist of:
- kill old and useless migrators
- rewrite remaining migrators in js (use jsctypes where needed)
- fix browserProfileMigrator idl if needed for better async support
- fix UI to be more user-friendly and async

>diff --git a/browser/components/migration/src/Makefile.in b/browser/components/migration/src/Makefile.in

>+EXTRA_COMPONENTS = \
>+	nsChromeProfileMigrator.manifest \
>+	$(NULL)

Since there is this secret plan to convert other migrators to js, rather than having a manifest for each migrator, could you rename this to profileMigrators.manifest and we register all migrators in it?

>diff --git a/browser/components/migration/src/nsChromeProfileMigrator.js b/browser/components/migration/src/nsChromeProfileMigrator.js

>+ * The Initial Developer of the Original Code is Mozilla Foundation.

nit: is the Mozilla Foundation

>+function chromeTimeToTimeT(time) {
>+  // Google Chrome uses FILETIME / 10 as time.
>+  // FILETIME is based on same structure of Windows.

nit: convert this comment to a javadoc before the function, add a @param and a @return descriptions
The name of the function could be more js-like, maybe chromeTimeToMicroseconds? But looks like you are trying to return seconds here?
Notice that Places APIs use _microseconds_ from 1970, while nsICookieManager2 uses seconds, so to be more exact this should return microseconds and cookies call should round to seconds.

>+  return Math.floor((time * 10 -  0x19DB1DED53E8000 ) / 10000000);
>+}

wrong spacing around the magic number.
Btw, move this magic number to a named const please, like const 100NANOSECONDS_INTERVALS_FROM_1601_TO_1970 = 0x19DB1DED53E8000;
The second magic number as well should be a const 100NANOSECONDS_INTERVALS_PER_MICROSECOND = 10;

>+function readJSONfile(path)
>+{

As said, this should be async and have a callback.
The UI could not be ready to handle an async process, but that should not prevent us from proceding, we should fix that in next steps.

>+function insertBookmarkItems(folderId, items) {

maybe better insertBookmarksInFolder(items, folderId)

>+  items.forEach(function (item) {

based on latest research, using forEach is unefficient for the garbage collector, since here we could import hundreds of entries, I'd go for a simple for loop.

>+    try {
>+      if (item.type == "url") {
>+        let uri = Services.io.newURI(item.url, null, null);

Use NetUtil.newURI(item.url) directly in the Places API call

>+        PlacesUtils.bookmarks.insertBookmark(folderId, uri,
>+                                             PlacesUtils.bookmarks.DEFAULT_INDEX,
>+                                             item.name);
>+      } else if (item.type == "folder") {
>+        let newfolderId = PlacesUtils.bookmarks.createFolder(folderId, item.name,
>+                                                             PlacesUtils.bookmarks.DEFAULT_INDEX);
>+        insertBookmarkItems(newfolderId, item.children);
>+      }
>+    } catch (e) {
>+      // ignore error

as said, let's Cu.reportError

>+function importBookmark(items, folderId, folderName) {

>+  if (items.length > 0) {
>+    if (folderName) {
>+      folderId = PlacesUtils.bookmarks.createFolder(folderId, folderName,
>+                                                    PlacesUtils.bookmarks.DEFAULT_INDEX);
>+    }
>+    insertBookmarkItems(folderId, items);
>+  }
>+}

this function is confusing with the previous one, maybe they could be merged, the first one could get a third optional aCreateSubfolder aargument, if true it would create folder (move the code to create the title from below to here) and then proceed calling itself without the optional argument.
Or rename this one to createSubfolderAndImportBookmarks(aParentId, aNewFolderTitle, aItems)

>+function nsChromeProfileMigrator() {
>+}

nit: brace on new line (consistent with above)

>+
>+nsChromeProfileMigrator.prototype = {

>+  ALL : 0x0000,
>+  SETTINGS : 0x0001,
>+  COOKIES : 0x0002,
>+  HISTORY : 0x0004,
>+  FORMDATA : 0x0008,
>+  PASSWORDS : 0x0010,
>+  BOOKMARKS : 0x0020,
>+  OTHERDATA : 0x0040,

why are not these const rather than properties?

>+  migrate: function Chrome_migrate(aItems, aStartup, aProfile) {

nit: brace on new line (consistent with above)
nit: what about renaming aStartup to aProfileStartup, aStartup.doStartup() is funny :p
aItems is confusing since in the same file other methods are using the same name for an array of bookmarks, maybe aFeatures, or aMigratingFeatures?

As a global hint, in future (soon really) most of the pieces that I see in migrate() will be asynchronous, so would be really cool (but you can skip it for lack of time) if each piece would be a method with a callback, doing fake-async callbacks.
Then converting this to async would be incredibly easy.
Regardless this, Dietrich's suggestion to split into single helpers for each feature sounds great and is also moving toward the async direction.

>+    if (aItems & this.BOOKMARKS) {
>+      // Copy bookmark
>+      Services.obs.notifyObservers(null, "Migration:ItemBeforeMigrate",
>+                                   this.BOOKMARKS);
>+      try {
>+        PlacesUtils.bookmarks.runInBatchMode({
>+          _self : this,
>+          runBatched : function (aUserData) {
>+            let importTitle = null;
>+            if (!this._self._replace) {
>+              // get "import from google chrome" string for folder
>+              let strbundle = Services.strings.createBundle("chrome://browser/locale/migration/migration.properties");
>+              let sourceNameChrome = strbundle.GetStringFromName("sourceNameChrome");
>+              importTitle = strbundle.formatStringFromName("importedBookmarksFolder",
>+                                                           [sourceNameChrome],
>+                                                           1);

make importTitle a "bookmarksSubfolderTitle" lazyGetter in the component, and avoid the comment above too.

>+            if (this._self._bookmarks.roots.bookmark_bar.children)
>+              importBookmark(this._self._bookmarks.roots.bookmark_bar.children,
>+                             PlacesUtils.bookmarks.toolbarFolder,

Use PlacesUtils.toolbarFolderId, so you avoid a xpcom crossing

>+                             importTitle);
>+            if (this._self._bookmarks.roots.other.children)
>+              importBookmark(this._self._bookmarks.roots.other.children,
>+                             PlacesUtils.bookmarks.bookmarksMenuFolder,

Use PlacesUtils.bookmarksMenuFolderId, so you avoid a xpcom crossing

>+        }, null);
>+      } catch (e) {
>+        // ignore error

reportError here as well

>+    if (aItems & this.HISTORY) {
>+      // Copy hisotry

ok, here we have the first real problems:
- this is using synchronous Storage calls (createStatement, executeStep), it should use asynchronous storage
- the function should be made async, at this point doing what I said above about async callbacks seems needed :(
- adding to history should use the new updatePlaces asynchronous call, as Sync does: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/mozIAsyncHistory.idl

>+    if (aItems & this.COOKIES) { 
>+      // Copy cookie

Same synchronous storage use as above, we should avoid it
Also, I'm not sure if nsICookieManager2.add is synchrounous, I thought all cookies management was made asynchronous for FF4.
It should probably listen to cookie changes and notify when done, but most likely you could just put a TODO comment on that and don't care for now (cookies are far less than history or bookmarks)


>+  getMigrateData: function Chrome_getMigrateData(aProfile, aDoingStartup) {

nit: brace on newline for consistency (won't report anymore later, but please just be consistent in the file)
I see this method could be a problem to do async, but probably here you just want to check if files exist, and let the async json load for the real migration work, since it could even take some time to parse a large json (indeed sourceExist could be really slow as it is now)
Also, reportError in the catches here

As a side note, would be cool to be able to gather Chrome's version, since they could change format of the data across versions, we should have an import code able to adapt to different versions

Comment 18

[Dietrich Ayala (:dietrich)]

Hi Makoto, do you know if you can still work on this? It would be fantastic to get this patch into Firefox.

Comment 19

[Makoto Kato [:m_kato]]

(In reply to comment #18)
> Hi Makoto, do you know if you can still work on this? It would be fantastic to
> get this patch into Firefox.

I will update this in next week.

Comment 21

[Makoto Kato [:m_kato]]

Asa, this bug is for cookies, history, and bookmarks.

If you need others, please file a new bug or reopen bug 589589.

Comment 22

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 528550 [details] [diff] [review]
fix v2

Review of attachment 528550 [details] [diff] [review]:

This doesn't import keywords as part of the bookmarks import.
They are in Chrome history database, keyword_search_terms table.
I'm not asking to address this here, but a follow-up is needed, at least.
They should be imported as bookmarks in unsortedBookmarksFolderId using "lower_term keyord" as title and "lower_term" as keyword (I've not checked if they use %s as a replacement as we do though).

This is much better than the first iteration, I  think next review will be positive, great work!
Apart some nits and suggestions, there are a couple important problems, like history titles not being imported due to a typo, attribute possibly throwing, wrong expiration timestamp for cookies.

I think the plain review following will be unreadable (at least it is on the theme I use, will check the default theme), so I suggest to use the new Review link (Splinter).

::: browser/components/migration/src/Makefile.in
@@ +76,5 @@
            $(NULL)
+endif
+
+EXTRA_PP_COMPONENTS = \
+	nsChromeProfileMigrator.js \

you should drop the ns prefix, ChromeProfileMigrator.js is fine.

@@ +80,5 @@
+	nsChromeProfileMigrator.js \
+	$(NULL)
+
+EXTRA_COMPONENTS = \
+	nsChromeProfileMigrator.manifest \

I still think would be better to just call this ProfileMigrators.manifest and register all future migrators into it, unless I'm missing something.

::: browser/components/migration/src/nsChromeProfileMigrator.js
@@ +53,5 @@
+const MIGRATE_BOOKMARKS = 0x0020;
+const MIGRATE_OTHERDATA = 0x0040;
+
+const CONVERT_100NS_INTERVALS_FROM_1601_TO_1970 = 0x19DB1DED53E8000;
+const CONVERT_100NS_INTERVALS_PER_MS = 10;

the CONVERT_ prefix doesn't add value here, I'd drop it making the const names shorter.

@@ +62,5 @@
+Components.utils.import("resource://gre/modules/NetUtil.jsm");
+
+XPCOMUtils.defineLazyGetter(this, "bookmarksSubfolderTitle", function () {
+  // get "import from google chrome" string for folder
+  let strbundle = Services.strings.createBundle("chrome://browser/locale/migration/migration.properties");

nit: cut at 80 chars
let strbundle =
  Services.string...

@@ +73,5 @@
+/*
+ * Convert Chrome time format to Date object
+ *
+ * @param   aTime
+ *          Chrome time into SQLite database

nit: remove "into SQLite database", the helper has no clue about databases, it just gets a numeric value.

@@ +75,5 @@
+ *
+ * @param   aTime
+ *          Chrome time into SQLite database
+ * @returns
+ *          converted Date object

@return (with no trailing s) is the correct form, and put both on the same line

not going to repeat on @return, should be an easy search&replace in the patch.

@@ +80,5 @@
+ */
+function chromeTimeToDate(aTime)
+{
+  // Google Chrome uses FILETIME / 10 as time.
+  // FILETIME is based on same structure of Windows.

// Chrome uses FILETIME / 10 as time, where FILETIME has the same
// structure as Windows FILETIME: the number of 100-nanosecond intervals
// since January 1, 1601 (UTC).

@@ +92,5 @@
+ *          file path of JSON format file
+ * @returns
+ *          JSON object
+ */
+function readJSONfile(aPath)

I see that you cannot make this async due to sourceHomePageURL that wants the data synchronously.
This is bad but I don't think we can handle the problem here, please file a follow-up to make sourceHomePageURL an async method and add a "TODO (bug#######): Make this async." comment

@@ +113,5 @@
+/*
+ * Insert bookmark items into specific folder.
+ *
+ * @param   aFolderId
+ *          folder id that item is inserted

id of the folder where items will be inserted

@@ +115,5 @@
+ *
+ * @param   aFolderId
+ *          folder id that item is inserted
+ * @param   aItems
+ *          insertd bookmark items

bookmarks items to be inserted

@@ +124,5 @@
+    let item = aItems[i];
+
+    try {
+      if (item.type == "url") {
+        let uri = NetUtil.newURI(item.url, null, null);

newURI should allow optional arguments, so just NetUtil.newURI(item.url)

@@ +131,5 @@
+                                             item.name);
+      } else if (item.type == "folder") {
+        let newfolderId = PlacesUtils.bookmarks.createFolder(aFolderId, item.name,
+                                                             PlacesUtils.bookmarks.DEFAULT_INDEX);
+        insertBookmarkItems(newfolderId, item.children);

why don't you use createSubfolderAndImportBookmarks here?

@@ +141,5 @@
+}
+
+
+/*
+ * Create bookmarks items into aParentId's folder

Creates a new folder and inserts bookmarks into it

@@ +144,5 @@
+/*
+ * Create bookmarks items into aParentId's folder
+ 
+ * @param   aParentId
+ *          id of top folder

Let's make this aGrandParentId, id of the folder where a new subfolder will be created.

@@ +146,5 @@
+ 
+ * @param   aParentId
+ *          id of top folder
+ * @param   aNewFolderTitle
+ *          parent folder name.  if null, it isn't created.

I don't see a use-case for passing null here, at that point I'd just use insertBookmarkItems.
I think you should handle the fork in the calling code below, rather than polluting the helpers here.

@@ +148,5 @@
+ *          id of top folder
+ * @param   aNewFolderTitle
+ *          parent folder name.  if null, it isn't created.
+ * @param   aItems
+ *          inserted bookmark items

bookmarks items to be inserted

@@ +157,5 @@
+    if (aNewFolderTitle) {
+      aParentId = PlacesUtils.bookmarks.createFolder(aParentId, aNewFolderTitle,
+                                                     PlacesUtils.bookmarks.DEFAULT_INDEX);
+    }
+    insertBookmarkItems(aParentId, aItems);

always create the subfolder, as said above.

@@ +169,5 @@
+nsChromeProfileMigrator.prototype = {
+  _bookmarkPath : null,
+  _cookiePath : null,
+  _historyPath : null,
+  _prefsPath : null,

nit: the common code style is property: value, rather than property : value
should be an easy search&replace
here you could use a object like _paths: {bookmarks: null, cookies: null, history: null, prefs: null},
and later use this._paths.bookmarks and such.

@@ +184,5 @@
+
+  _notifyStart : function Chrome_notifyStart(aType)
+  {
+    Services.obs.notifyObservers(null, "Migration:ItemBeforeMigrate",
+                                 aType);

nit: don't see a reason to go on newline.

@@ +197,5 @@
+   */
+  _notifyCompleted : function Chrome_notifyIfCompleted(aType)
+  {
+    Services.obs.notifyObservers(null, "Migration:ItemAfterMigrate",
+                                 aType);

nit: don't see a reason to go on newline.

@@ +200,5 @@
+    Services.obs.notifyObservers(null, "Migration:ItemAfterMigrate",
+                                 aType);
+    if (--this._pendingCount == 0) {
+      Services.obs.notifyObservers(null, "Migration:Ended",
+                                   null);

ditto

@@ +207,5 @@
+
+  /*
+   * Migrating bookmark items
+   */
+  _migrateBookmark : function Chrome_migrateBookmark()

nit: _migrateBookmarks (plural) makes more sense

@@ +218,5 @@
+        runBatched : function (aUserData) {
+
+          let migrator = this._self;
+          let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
+          file.initWithPath(this._self._bookmarkPath);

use migrator rather than this._self since you defined it.

@@ +231,5 @@
+            let str = {};
+            let cstream = Cc[CONVERT_INPUT_STREAM_CID].createInstance(Ci.nsIConverterInputStream);
+            cstream.init(aInputStream, "UTF-8", aInputStream.available(), 0);
+            cstream.readString(-1, str);
+            cstream.close();

Can you use NetUtil.readInputStreamToString here?

@@ +247,5 @@
+                                                bookmarks.roots.bookmark_bar.children);
+            if (bookmarks.roots.other.children)
+              createSubfolderAndImportBookmarks(PlacesUtils.bookmarksMenuFolderId,
+                                                importTitle,
+                                                bookmarks.roots.other.children);

As said above, you should completely split the 2 replace paths, rather then relying on createSubfolderAndImportBookmarks doing the right thing.

@@ +275,5 @@
+          let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
+          file.initWithPath(this._self._historyPath);
+
+          let dbConn = Services.storage.openDatabase(file);
+          let stmt = dbConn.createAsyncStatement("SELECT url, title, last_visit_time, typed_count FROM urls");

I think you should add a WHERE hidden = 0. Looking at their db I see a bunch of useless (for our use-case) hidden entries, we don't mind of offering these entries in the awesomebar, so no reason to import them.

@@ +288,5 @@
+              while (row = aResults.getNextRow()) {
+                try {
+                  let title = row.getResultByName("title");
+                  if (title == "")
+                    title = row.getResultByName("url");

is this really needed? We should be able to handle empty and null titles, here you probably want to title = null;
If it doesn't work, please file a bug against Toolkit/Places.

@@ +296,5 @@
+                  if (row.getResultByName("typed_count") > 0)
+                    transType = PlacesUtils.history.TRANSITION_TYPED;
+
+                  places.push({
+                    uri: NetUtil.newURI(row.getResultByName("url"), null, null),

no need for ", null, null" on newURI

@@ +297,5 @@
+                    transType = PlacesUtils.history.TRANSITION_TYPED;
+
+                  places.push({
+                    uri: NetUtil.newURI(row.getResultByName("url"), null, null),
+                    titie: title,

important: typo in "titie", this is not going to set any title.

@@ +334,5 @@
+
+  /*
+   * Migrating cookies
+   */
+  _migrateCookie : function Chrome_migrateCookie()

_migrateCookies (plural) makes more sense

@@ +352,5 @@
+        _db : dbConn,
+        _migrator : this,
+        handleResult : function(aResults) {
+          let row = null;
+          while (row = aResults.getNextRow()) {

nit: fwiw, you can oneline this as for (let row = aResults.getNextRow(); row; row = aResults.getNextRow()) {

@@ +354,5 @@
+        handleResult : function(aResults) {
+          let row = null;
+          while (row = aResults.getNextRow()) {
+            let host_key = row.getResultByName("host_key");
+            if (host_key.match(/^\./)) {

use "if (/^\./.test(host_key)) {" since we don't need the match.

@@ +360,5 @@
+              host_key = host_key.substr(1);
+            }
+
+            try {
+              // TODO: async insertion of cookies

sigh, I thought they were made async, but it's only the read part. one day maybe :(

@@ +368,5 @@
+                          row.getResultByName("value"),
+                          row.getResultByName("secure"),
+                          row.getResultByName("httponly"),
+                          false,
+                          chromeTimeToDate(row.getResultByName("expires_utc")) * 1000);

nit: the indentation of the arguments is wrong

important: _cm.add takes SECONDS so this should be parseInt(chromeTimeToDate(...) / 1000);

@@ +427,5 @@
+
+    if (--this._pendingCount == 0) {
+      // async imports are immeditelly completed.
+      Services.obs.notifyObservers(null, "Migration:Ended",
+                                   null);

I see what's up here, one of the migrators could be immediately setting _pendingCount to 1, and then decrease it, causing Migration:Ended to be fired too early.
I think you need a better comment to explain this, since at first glance this was looking bogus.

@@ +439,5 @@
+   *          this is unused due to single profile support only
+   * @param   aDoingStartup
+   *          non-null if called during startup.
+   * @returns
+   *          supproted migration types

typo: supproted

@@ +496,5 @@
+
+  /*
+   * Whether we support migration of Chrome
+   *
+   * @returns return true if supported

nit: @return true if supported

@@ +514,5 @@
+   * Return home page URL
+   *
+   * @returns home page URL
+   */
+  sourceHomePageURL: function Chrome_sourceHomePageURL()

did you test this? I'm not sure but being a readonly attribute I had feeling it should have been a getter.

@@ +522,5 @@
+
+    if (!this._prefsPath)
+      this.getMigrateData(null, false);
+
+    this._homepageURL = readJSONfile(this._prefsPath).homepage;

This can arguably throw, if readJSONfile fails (corrupt json or such), in such a case you should return an empty string.

Comment 24

[Shawn Wilsher :sdwilsh]

We should be able to write an xpcshell tests that imports from a chrome database file, no?

Comment 25

[Marco Bonardo [:mak]]

(In reply to comment #24)
> We should be able to write an xpcshell tests that imports from a chrome
> database file, no?

yes, provided we add some helper to drive the migrator path to a custom one.

Comment 26

[Makoto Kato [:m_kato]]

Marco, thanks for reviewing.

(In reply to comment #23)
> @@ +131,5 @@
> +                                             item.name);
> +      } else if (item.type == "folder") {
> +        let newfolderId = PlacesUtils.bookmarks.createFolder(aFolderId,
> item.name,
> +                                                            
> PlacesUtils.bookmarks.DEFAULT_INDEX);
> +        insertBookmarkItems(newfolderId, item.children);
> 
> why don't you use createSubfolderAndImportBookmarks here?

If items is empty, it doesn't create folder.
 
> @@ +146,5 @@
> + 
> + * @param   aParentId
> + *          id of top folder
> + * @param   aNewFolderTitle
> + *          parent folder name.  if null, it isn't created.
> 
> I don't see a use-case for passing null here, at that point I'd just use
> insertBookmarkItems.
> I think you should handle the fork in the calling code below, rather than
> polluting the helpers here.

When calling from start-up, migration should not create "Import from xxxxx" folder.

> @@ +231,5 @@
> +            let str = {};
> +            let cstream =
> Cc[CONVERT_INPUT_STREAM_CID].createInstance(Ci.nsIConverterInputStream);
> +            cstream.init(aInputStream, "UTF-8", aInputStream.available(), 0);
> +            cstream.readString(-1, str);
> +            cstream.close();
> 
> Can you use NetUtil.readInputStreamToString here?

NetUtil.readInputStreamToString doesn't set character encoding.  It is no good when we need character conversion.

Comment 27

[Marco Bonardo [:mak]]

(In reply to comment #26)

> (In reply to comment #23)                                                 
> > PlacesUtils.bookmarks.DEFAULT_INDEX);
> > +        insertBookmarkItems(newfolderId, item.children);
> > 
> > why don't you use createSubfolderAndImportBookmarks here?
> 
> If items is empty, it doesn't create folder.

then you could just change the calling points like
if (bookmarks.roots.bookmark_bar.children && bookmarks.roots.bookmark_bar.children.length > 0)
  createSubfolderAndImportBookmarks(...

The createsubfolder helper is handy, it's a pity to have magic conditions in it.

> > + 
> > + * @param   aParentId
> > + *          id of top folder
> > + * @param   aNewFolderTitle
> > + *          parent folder name.  if null, it isn't created.
> > 
> > I don't see a use-case for passing null here, at that point I'd just use
> > insertBookmarkItems.

> 
> When calling from start-up, migration should not create "Import from xxxxx"
> folder.

I understand this, my suggestion is to handle this in the migrating code with a simple if/else, rather than with hidden magic in the helper, and have the helper just do what its name says.

something like:

if (bar.children && bar.children.length > 0) {
  if (!migrator._replace)
    createSubfolderAndImportBookmarks
  else
    nsertBookmarkI
}
if (menu.children && menu.children.length > 0) {
  if (!migrator._replace
    createSubfolderAndImportBookmarks
  else
    InsertBookmarkI
}

it's more verbose, but being explicit should prevent surprises. eventually you could make a new helper that will get migrator._replace, children and parent (PlacesUtils.toolbarFolderId or menu) but there should not be the need.
Small specialized helpers are less bug prone imo.

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #26)
> > Can you use NetUtil.readInputStreamToString here?
> 
> NetUtil.readInputStreamToString doesn't set character encoding.  It is no
> good when we need character conversion.

We could fix the NetUtil with an optional argument, I'm sure sdwilsh could review that change.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> We could fix the NetUtil with an optional argument, I'm sure sdwilsh could
> review that change.
Please do it in a separate bug though.

Comment 30

[Makoto Kato [:m_kato]]

(In reply to comment #29)
> (In reply to comment #28)
> > We could fix the NetUtil with an optional argument, I'm sure sdwilsh could
> > review that change.
> Please do it in a separate bug though.

I filed as bug 655658.

Comment 31

[Asa Dotzler [:asa]]

m_kato, this feature and the additions at bug 589589 have icreased in priority recently. You've done some good work here but it seems to have stalled. Are you going to be able to return to this any time soon? Is there any help we can offer in terms of additional design, developer, or review resources?

Comment 32

[Makoto Kato [:m_kato]]

(In reply to Asa Dotzler [:asa] from comment #31)
> m_kato, this feature and the additions at bug 589589 have icreased in
> priority recently. You've done some good work here but it seems to have
> stalled. Are you going to be able to return to this any time soon? Is there
> any help we can offer in terms of additional design, developer, or review
> resources?

I has new patch and I will set review flag for it next week after testing.

Comment 33

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 34

[Asa Dotzler [:asa]]

Marco, can you take a look at this patch? It'd be nice to try to get these migrators into 10 and we've only got about a week left. 

Makoto, are you going to be able to get to bug 589589 any time soon?

Comment 35

[Marco Bonardo [:mak]]

Please, just flag me for review if the patch is ready.

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 569950 [details] [diff] [review]
fix v3

Review of attachment 569950 [details] [diff] [review]:
-----------------------------------------------------------------

There are some things to fix, but I don't see anything preventing this from landing.
I'm giving a r+ on the assumption you tested this manually checking that bookmarks, history, cookies and the homepage have been correctly imported.
We should really put some ateam resources in making a migration test harness for the major browsers.

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +63,5 @@
> +XPCOMUtils.defineLazyGetter(this, "bookmarksSubfolderTitle", function () {
> +  // get "import from google chrome" string for folder
> +  let strbundle =
> +    Services.strings.createBundle(
> +      "chrome://browser/locale/migration/migration.properties");

nit: move "chrome://browser/locale/migration/migration.properties" to a BUNDLE_MIGRATION const, then you may oneline this

@@ +80,5 @@
> + */
> +function chromeTimeToDate(aTime)
> +{
> +  // Google Chrome uses FILETIME / 10 as time.
> +  // FILETIME is based on same structure of Windows.

move this to a @note in the javadoc

@@ +94,5 @@
> + *          bookmark items to be inserted
> + */
> +function insertBookmarkItems(aFolderId, aItems)
> +{
> +  let bookmarks = PlacesUtils.bookmarks;

hm, you can avoid this with better indentation below, it just makes the function longer with doubtful benefit

@@ +103,5 @@
> +    try {
> +      if (item.type == "url") {
> +        let uri = NetUtil.newURI(item.url);
> +        bookmarks.insertBookmark(aFolderId, uri, bookmarks.DEFAULT_INDEX,
> +                                 item.name);

just put newURI inside
PlacesUtils.bookmarks.insertBookmark(aFolderId,
                                     NetUTil.newURI(uri),
                                     PlacesUtils.bookmarks.DEFAULT_INDEX,
                                     item.name);

@@ +106,5 @@
> +        bookmarks.insertBookmark(aFolderId, uri, bookmarks.DEFAULT_INDEX,
> +                                 item.name);
> +      } else if (item.type == "folder") {
> +        let newFolderId =
> +          bookmarks.createFolder(aFolderId, item.name, bookmarks.DEFAULT_INDEX);

indent as above

@@ +129,5 @@
> +    prefs : null,
> +  },
> +
> +  _homepageURL : null,
> +  _replace : false,

rename to _replaceBookmarks since it seems to apply only to bookmarks...

@@ +163,5 @@
> +
> +  /*
> +   * Migrating bookmark items
> +   */
> +  _migrateBookmark : function Chrome_migrateBookmark()

nit: since there is migrateCookies, this should probably be plural, as well

@@ +177,5 @@
> +          file.initWithPath(migrator._paths.bookmarks);
> +
> +          NetUtil.asyncFetch(file, function(aInputStream, aResultCode) {
> +            if (!Components.isSuccessCode(aResultCode)) {
> +              this._self._notifyCompleted(MIGRATE_BOOKMARKS);

s/this._self/migrator/

@@ +178,5 @@
> +
> +          NetUtil.asyncFetch(file, function(aInputStream, aResultCode) {
> +            if (!Components.isSuccessCode(aResultCode)) {
> +              this._self._notifyCompleted(MIGRATE_BOOKMARKS);
> +              retrun;

typo retrun

@@ +185,5 @@
> +            // Parse Chrome bookmark file that is JSON format
> +            let roots = JSON.parse(
> +              NetUtil.readInputStreamToString(aInputStream,
> +                                              aInputStream.available(),
> +                                              { charset : "UTF-8" })).roots;

please assign NetUtils.readInput... to a local var, to improve readability.

@@ +235,5 @@
> +
> +    try {
> +      PlacesUtils.history.runInBatchMode({
> +        _self : this,
> +        runBatched : function (aUserData) {

since updatePlaces is async, runInBatchMode won't bring anything useful here, since it is unable to handle async stuff, so you can remove this complication for history migration.
We'll figure out in future if we should make updatePlaces fire the batch notifications.

@@ +240,5 @@
> +          // access sqlite3 database of Chrome's history
> +          let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
> +          file.initWithPath(this._self._paths.history);
> +
> +          let dbConn = Services.storage.openDatabase(file);

Please use openUnsharedDatabase, it is a bit more performant.

@@ +242,5 @@
> +          file.initWithPath(this._self._paths.history);
> +
> +          let dbConn = Services.storage.openDatabase(file);
> +          let stmt = dbConn.createAsyncStatement(
> +              "SELECT url, title, last_visit_time, typed_count FROM urls WHERE hidden = 0");

This is oversimplifying a bit, it's importing a single visit and ignoring the "visits" table, that means we may give a better frecency to our users, out of their data, but we don't.

On the other side, I don't think this should block us from shipping a first version of the migrator, so this may be addressed in a follow-up bug.

@@ +248,5 @@
> +          stmt.executeAsync({
> +            _asyncHistory : Cc["@mozilla.org/browser/history;1"]
> +                            .getService(Ci.mozIAsyncHistory),
> +            _db : dbConn,
> +            _self : this._self,

killing the batch should simplify this self redirection

@@ +265,5 @@
> +                    visits: [{
> +                      transitionType: transType,
> +                      visitDate: chromeTimeToDate(
> +                                   row.getResultByName(
> +                                     "last_visit_time")) * 1000,

move this time calculation to a local var, to improve readability

@@ +281,5 @@
> +              }
> +            },
> +
> +            handleError : function(aError) {
> +              Cu.reportError(aError);

this will likely report unreadable stuff, you need something like
Cu.reportError("Async statement execution returned with '" +
               aError.result + "', '" + aError.message + "'");

@@ +310,5 @@
> +      // Access sqlite3 database of Chrome's cookie
> +      let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
> +      file.initWithPath(this._paths.cookies);
> +
> +      let dbConn = Services.storage.openDatabase(file);

ditto for unshared

@@ +334,5 @@
> +                                   row.getResultByName("secure"),
> +                                   row.getResultByName("httponly"),
> +                                   false,
> +                                   parseInt(chromeTimeToDate(
> +                                     row.getResultByName("expires_utc")) / 1000));

ditto for local time var

@@ +342,5 @@
> +          }
> +        },
> +
> +        handleError : function(aError) {
> +          Cu.reportError(aError);

ditto for handleError

@@ +431,5 @@
> +    try {
> +      let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
> +      file.initWithPath(chromepath + "Bookmarks");
> +      this._paths.bookmarks = file.path
> +      result += MIGRATE_BOOKMARKS;

shouldn't we check if the file exists? And also for the other entries, below

@@ +492,5 @@
> +
> +      if (!this._paths.prefs)
> +        this.getMigrateData(null, false);
> +
> +      // XXX reading and parsing JSON is synchronized.

synchronized? you mean synchronous?

::: browser/components/migration/src/Makefile.in
@@ +73,5 @@
> +	$(NULL)
> +
> +EXTRA_COMPONENTS = \
> +	BrowserProfileMigrator.manifest \
> +	$(NULL)

nit: indent just 2 spaces

::: browser/components/migration/src/nsProfileMigrator.cpp
@@ +248,5 @@
>      return NS_OK;
>    }
> +  else if (internalName.LowerCaseEqualsLiteral(INTERNAL_NAME_CHROME)) {
> +    aKey = "chrome";
> +    return NS_OK;

OT: I like how this code converts strings to themselves... sigh, would be so nice to refactor all of this code...

::: browser/installer/package-manifest.in
@@ +357,5 @@
>  @BINPATH@/components/contentSecurityPolicy.js
>  @BINPATH@/components/contentAreaDropListener.manifest
>  @BINPATH@/components/contentAreaDropListener.js
> +@BINPATH@/components/BrowserProfileMigrator.manifest
> +@BINPATH@/components/ChromeProfileMigrator.js

nit: I'd probably call the manifest BrowserProfileMigrators.manifest, the plural form should make clearer to newcomers where they should put their new migrators

Comment 37

[Dão Gottwald [:dao]]

> > +            // Parse Chrome bookmark file that is JSON format
> > +            let roots = JSON.parse(
> > +              NetUtil.readInputStreamToString(aInputStream,
> > +                                              aInputStream.available(),
> > +                                              { charset : "UTF-8" })).roots;
> 
> please assign NetUtils.readInput... to a local var, to improve readability.

Please don't do this. It can break the call depending on what that method is doing.

Comment 38

[Marco Bonardo [:mak]]

I think my comment was misleading, i did not mean to shortcut the function, i meant to assign its result to a local var.

Comment 39

[Dietrich Ayala (:dietrich)]

r+, almost there! Makoto, are you going to be able to make this final set of changes anytime soon?

Comment 40

[Josh Matthews [:jdm]]

Attachment: Importing bookmarks from Google Chrome

I updated the patch to address Mak's concerns.

Comment 41

[Josh Matthews [:jdm]]

Attachment: Importing bookmarks from Google Chrome

With qref changes.

Comment 42

[Josh Matthews [:jdm]]

>@@ +431,5 @@
>> +    try {
>> +      let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
>> +      file.initWithPath(chromepath + "Bookmarks");
>> +      this._paths.bookmarks = file.path
>> +      result += MIGRATE_BOOKMARKS;
>
>shouldn't we check if the file exists? And also for the other entries, below

I don't believe this is necessary, since the migrator checks the success code of NetUtil.asyncFetch and skips the migration if the fetch fails.

Comment 43

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #42)
> I don't believe this is necessary, since the migrator checks the success
> code of NetUtil.asyncFetch and skips the migration if the fetch fails.

ok, did you have a chance to test the migrator?

Comment 44

[Josh Matthews [:jdm]]

I did. It definitely transferred my bookmarks and history.

Comment 45

[Makoto Kato [:m_kato]]

Jdm, do you land this?  I have updated patch by marco's review.

Comment 46

[Josh Matthews [:jdm]]

I have not done anything but upload my updated version accoding to marco's review.

Comment 47

[Matthew N. [:MattN]]

Comment on attachment 569950 [details] [diff] [review]
fix v3

Review of attachment 569950 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Nit: The mode line should be updated to reflect reality. (js & tab-width of 2). See https://mxr.mozilla.org/mozilla-central/search?string=mode%3A+js for examples.

Comment 48

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07d4c438a18b

Comment 49

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/07d4c438a18b

Comment 50

[Johnathan Nightingale [:johnath]]

Just wanted to say that this is great to see - a big thank you to Makoto and the various people helping out

Comment 51

[Marco Bonardo [:mak]]

Looking at jdm patch he correctly removed the runInBatchMode stuff from migrateHistory, but the landed version did not...

kato-san, could you please make an interdiff with jdm's version and figure out the missing review bits?

Comment 52

[Marco Bonardo [:mak]]

to give context, comment 36

@@ +235,5 @@
> +
> +    try {
> +      PlacesUtils.history.runInBatchMode({
> +        _self : this,
> +        runBatched : function (aUserData) {

since updatePlaces is async, runInBatchMode won't bring anything useful here, since it is unable to handle async stuff, so you can remove this complication for history migration.
We'll figure out in future if we should make updatePlaces fire the batch notifications.

Comment 53

[Paul Silaghi, QA [:pauly]]

Hi guys,

I noticed one thing on Firefox 11 and 12. 
Opera browser disappears from the import window, although I have it installed. Only Internet Explorer and Chrome are listed. In Firefox 10, IE and Opera are listed.
What do you think?

Comment 54

[Marco Bonardo [:mak]]

The opera migrator has been removed, see bug 539133.

Comment 55

[u434713]

Maybe a bug, I don't know. When I do: bookmarks>show all bookmarks>import and backup>import data from another browser, in firefox 10 it only shows 'internet explorer' and 'opera' and in firefox 11 beta shows only internet explorer. After creating new profiles for both firefoxs versions, in version 10 nothing changed, but in version 11 beta, a chrome option was added! Odd, I would say...

Comment 56

[Matthew N. [:MattN]]

(In reply to Erlis Dhima from comment #55)
This is expected since you can see from the "Target Milestone" field that this was only added in Firefox 11.

Comment 57

[antistress]

Does that work for Chromium also ?

Comment 58

[Matthew N. [:MattN]]

Not yet, bug 706973 will add Chromium support.