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

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

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.

So how goes the progress?

(In reply to comment #8) > So how goes the progress? Feel free to make a patch. They are always welcome ;)

(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.

(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.

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 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.

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

(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 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

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

(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.

Asa, this bug is for cookies, history, and bookmarks. If you need others, please file a new bug or reopen bug 589589.

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.

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

(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.

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.

(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.

(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.

(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.

(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.

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?

(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.

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?

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

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

> > + // 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.

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

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

I updated the patch to address Mak's concerns.

With qref changes.

>@@ +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.

(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?

I did. It definitely transferred my bookmarks and history.

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

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

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.

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

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?

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.

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?

The opera migrator has been removed, see bug 539133.

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...

(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.

Does that work for Chromium also ?

Not yet, bug 706973 will add Chromium support.