*** Original post on bio 1705 at 2012-09-26 00:44:00 UTC *** This bug will be for discussion and reviews surrounding the backend code of the import wizard (Bug 954928 (bio 1495)). This includes changes to chat/components, as well as the other necessary changes for the core of the importing functionality to work.

*** Original post on bio 1705 at 2012-09-26 00:45:59 UTC *** Here is my reply to Comments 9 and 10 of Bug 954928 (bio 1495). >>+ // returns an enumerator of prplIPref containing default values set different >>+ // than the protocol defaults. >This comment doesn't make sense. >I think you meant something like "returns an enumerator of prplIPref containing >values that are different from the default of the protocol plugin." To use the prplIPref to store values, I had to set their "default" values as those are what get returned through getBool, getInt, etc. Either way, it makes more sense to word it how you suggested. > This comment needs to be extended. What's going to call these methods? > Why do these methods even need to be exposed publicly? Isn't this something > that should be internal? These methods are called on the ExistingAccount objects by the importers. I will look into the idea you suggested in the more recent review of jsImporterHelper.jsm for making these simpler. > My assumption here is that the imIExistingAccount defines the account that the > wizard can expect. Correct. > So is it imIImportersService or aObserver that's notified? > (I suspect it's "aObserver, which will usually be the imIImportersService"). I've rewritten this to be more specific. >>+ void importData(in imIAccount aAccount, in string aPreference, > What's aPreference? aPreference is the stringified JSON containing the information the importer uses to continue the search where it left off. But I'm guessing this was more of a suggestion to add a comment :) >>+ // Closes any observation of idle notifications. >>+ void unInitImporters(); > What is this going to do to the import jobs that are currently running? This is called in two places, on application exit and upon an idle notification when there are no pending imports. In the case of the latter, there should not be any running import jobs. If the application is exiting, I am not sure what could be done; I would think any running jobs would have terminated since the user would likely have become active again before closing the program. >>+ // When an account is created, it must be queued for importing data. This >>+ // creates the preference indicating an awaiting import. >Is this something that really has to be done, or only if the user wishes to >import logs? >Should we offer a preference for this in the UI? As of now, everything works assuming the user wants to import logs. There could be room for a checkbox on the final wizard page for allowing data import. Its visibility could be toggled based on whether ExistingAccounts are found in the summary list. >Can importers tell us immediately if there's nothing to import, so that we >don't queue pointless import jobs? I don't think it would be always trivial to determine that (such as with a possible importer that might need to reach online for some information). Perhaps a 'canImport' boolean could be created, but I'm not sure if there is enough of a need, an import that terminates immediately shouldn't be noticeable. >>+ // observe should only be called by the imIImporter implementations to report >>+ // changes. >>+ void observe(in nsISupports aObject, in string aTopic, >>+ [optional] in wstring aData); >If this is provided to each importer in an aObserver parameter, does this >really need to be exposed in the interface? I corrected the comment, as the idle service also uses observe() now. I think that would make it necessary to expose the method. > What would you think of making startDate an attribute of prplIConversation? I think that wouldn't hurt. Should I add that to this patch, or will that be added through another sometime? >>+ let findAccounts = importer.findAccounts; > What's the point of this variable? I can try testing the code without it, but I recall that there was a definite reason for it. There was something strange about calling importer.findAccounts within the try block within the executeSoon that was resolved when I made that change. I think it had to do with the reference to importer being undefined within the executeSoon (because it was defined in the loop?) >>+ else if (aTopic === "import-status-updated") { > I wonder why you compare the value of the aTopic string with === and not just ==. Whenever I've read about the usage of === vs. == online, it seems to be suggested that === should be used wherever possible. Wouldn't it be assumed that aTopic would be a string, or at least should be in this case? >>+ getImporterById: function(aImporterId) { >>+ let cid; >>+ try { >>+ cid = categoryManager.getCategoryEntry(kImporterPluginCategory, aImporterId); >>+ } catch (e) { >>+ return null; // no importer registered for this id. > Isn't there something to display in the error console in this case? > Am I understanding correctly that this means an import was started, but cannot > be finished because after a restart the importer is no longer there (disabled > add-on?). That could be one reason why it would fail. But getImporterById is used in quite a few places (basically anywhere the importers are needed). >>+ if (!importer) >>+ return null; > What's the point of these 2 lines (given that if importer is null, returning > importer at the next line will return null)? getImporterById() and getImporters() were based of getProtocolById() and getProtocols(), respectively, so I was assuming there was some reason for why those were written as they were. :) > I'm wondering if you really wanted to have this new LogWriter object, or if it > would be easier to just make it the __proto__ of SystemLog.prototype and > ConversationLog.prototype. I'm not sure :). The LogWriter wasn't needed in the end to allow the importers to log conversations, but at the time I thought it reduced some code duplication. I'll look into making that the prototype of ConversationLog and SystemLog. > I had an additional question while reviewing chat/components: Should the > importer service have a timeout to not let the user wait more than a few > seconds if an importer is broken and never notifies the importer service of > account-search-finished? That would probably be a good idea. I had written the current importers to (hopefully) always send a finish notification or encounter errors (caught by the try-catch in the service).

*** Original post on bio 1705 at 2012-10-02 21:34:32 UTC *** (In reply to comment #1) > > This comment needs to be extended. What's going to call these methods? > > Why do these methods even need to be exposed publicly? Isn't this something > > that should be internal? > > These methods are called on the ExistingAccount objects by the importers. The importers import jsImporterHelper.jsm as a JS module, so they can access everything on ExistingAccount, they aren't limited to what's exposed in the idl interfaces. > But I'm guessing this was more > of a suggestion to add a comment :) Correct! :) > >>+ // Closes any observation of idle notifications. > >>+ void unInitImporters(); > > > What is this going to do to the import jobs that are currently running? > > This is called in two places, on application exit and upon an idle notification > when there are no pending imports. In the case of the latter, there should not > be any running import jobs. If the application is exiting, I am not sure what > could be done; I would think any running jobs would have terminated since the > user would likely have become active again before closing the program. I'm willing to believe this :). > >Can importers tell us immediately if there's nothing to import, so that we > >don't queue pointless import jobs? > > I don't think it would be always trivial to determine that (such as with a > possible importer that might need to reach online for some information). Sure. There can be difficult cases. > Perhaps > a 'canImport' boolean could be created, but I'm not sure if there is enough of > a need, an import that terminates immediately shouldn't be noticeable. Maybe. It still feels like some overhead, especially for importers that don't implement any log importing logic (for example because the client doesn't even log conversations). > > What would you think of making startDate an attribute of prplIConversation? > > I think that wouldn't hurt. Should I add that to this patch, or will that be > added through another sometime? I haven't looked at this patch again, but IIRC this change would simplify this patch, so I think you can do it as part of this patch. (Or a separate patch applied before this one, if you are comfortable with patch queues :)). > > I wonder why you compare the value of the aTopic string with === and not just > ==. > > Whenever I've read about the usage of === vs. == online, it seems to be > suggested > that === should be used wherever possible. Wouldn't it be assumed that aTopic > would be a string, or at least should be in this case? It's a string, yes. > >>+ getImporterById: function(aImporterId) { > >>+ let cid; > >>+ try { > >>+ cid = categoryManager.getCategoryEntry(kImporterPluginCategory, aImporterId); > >>+ } catch (e) { > >>+ return null; // no importer registered for this id. > > > Isn't there something to display in the error console in this case? > > Am I understanding correctly that this means an import was started, but cannot > > be finished because after a restart the importer is no longer there (disabled > > add-on?). > > That could be one reason why it would fail. But getImporterById is used in > quite > a few places (basically anywhere the importers are needed). Not sure what you mean here. My point was: shouldn't we Cu.reportError(e); in the catch block? > > >>+ if (!importer) > >>+ return null; > > > What's the point of these 2 lines (given that if importer is null, returning > > importer at the next line will return null)? > > getImporterById() and getImporters() were based of getProtocolById() and > getProtocols(), respectively, so I was assuming there was some reason for why > those were written as they were. :) In getProtocolById there's a null check because we then call the init method on the protocol object. IIRC here you are returning the result directly so the null check doesn't make much sense.

*** Original post on bio 1705 as attmnt 2182 at 2013-01-04 18:41:00 UTC *** I've made changes based on the initial review of the import wizard code. Specifically, Comments #9 and #10 of Bug 954928 (bio 1495) were addressed (unless otherwise noted below). This patch is a separation of the backend code from the UI and importers. I am requesting feedback instead of review as this would not be a patch that could land; the Makefiles in particular have a lot of extra spaces which I added to allow the rest of the patch queue to apply correctly. > I had an additional question while reviewing chat/components: Should the > importer service have a timeout to not let the user wait more than a few > seconds if an importer is broken and never notifies the importer service of > account-search-finished? I have added a Timer for each importer. As of now this gives importers 10 seconds to finish before they are considered stalled. I had to add a nsITimer to the imImporter interface, which seemed necessary due to the observer usage. > > Perhaps a 'canImport' boolean could be created, but I'm not sure if > > there is enough of a need, an import that terminates immediately > > shouldn't be noticeable. > Maybe. It still feels like some overhead, especially for importers that don't > implement any log importing logic (for example because the client doesn't even > log conversations). I did not address this concern yet. There could be ways to keep imported accounts from being queued for importing data if it is known that the importer does not support importing to begin with. Otherwise I have made changes for the other review comments - to the best of my knowledge :).

I took the time to unbitrot this patch and split it up a bit more, it'd be great if we could get this landed before it bitrots more. I tested this briefly and it seemed to work. Note that this is prefed off by default. The UI will go in Bug 955133.

Unbitrotted version, the backend is in bug 955133. I'll attach importers next.

This is an importer for Pidgin (note that it doesn't work with Adium, I started hacking together a WIP for this though).

Comment on attachment 8392004 [details] [diff] [review] Unbitrotted patch (again) - UI part I just unfortunately had too many tabs open. This UI patch is going into bug 954928.

This is a dump of the other importers, I can break this up into smaller patches if we want.

Comment on attachment 8392005 [details] [diff] [review] Pidgin importer Review of attachment 8392005 [details] [diff] [review]: ----------------------------------------------------------------- How difficult would it be to use OS.File instead of nsIFile and/or to use a worker to process the log files? ::: chat/importers/importer-pidgin.js @@ +60,5 @@ > + > + let xmlParser = Cc["@mozilla.org/xmlextras/domparser;1"] > + .createInstance(Ci.nsIDOMParser); > + let accountXml > + = xmlParser.parseFromStream(stream, null, stream.available(), "text/xml"); Nit: the '=' should be on the previous line. @@ +63,5 @@ > + let accountXml > + = xmlParser.parseFromStream(stream, null, stream.available(), "text/xml"); > + let accounts = accountXml.documentElement.getElementsByTagName("account"); > + for (let i = 0; i < accounts.length; ++i) { > + let account = accounts[i]; for (let account of accounts) { @@ +141,5 @@ > + // any incorrect setting names passed to the set*() methods. > + _parseSettings: function(aSettings, aExistingAccount) { > + for (let i = 0; i < aSettings.length; ++i) { > + let eachSetting = aSettings[i].getElementsByTagName("setting") > + for (let j = 0; j < eachSetting.length; ++j) { for ... of (for both loops) @@ +157,5 @@ > + if (pidginSpecificSettings.indexOf(name) !== -1) > + continue; > + > + if (type === "bool") { > + if (name == "auto-login") value = value == "1"; And then you can remove the code duplication on the next 3 lines. @@ +201,5 @@ > + > + let conversation, sessionDatestring, username; > + let [line, more] = [{}, false]; > + do { > + more = logStream.readLine(line); Looks like main thread I/O :(. @@ +221,5 @@ > + let convHead = /with\s(.+?)\sat\s([\d\/:\s]+[AP]?M?)\son\s(.+?)\s\((.+?)\)/; > + let convMatches = val.match(convHead); > + if (convMatches) { > + let [target, date] = convMatches.slice(1); > + let isChat = (target.indexOf("#") === 0); target.startsWith("#") @@ +225,5 @@ > + let isChat = (target.indexOf("#") === 0); > + > + // The username is preserved to determine incoming/outgoing messages. > + username = convMatches[3]; > + if (username.indexOf("@") !== -1) username.contains("@")

Comment on attachment 8392003 [details] [diff] [review] Unbitrotted patch Review of attachment 8392003 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/src/imImporters.js @@ +69,5 @@ > + this._importers.push(importers.getNext().QueryInterface(Ci.imIImporter)); > + > + // If there are no importers registered, the observer still needs a signal. > + if (!this._importers.length) { > + executeSoon(function() { We can now use () => { here to save the .bind(this) call. @@ +78,5 @@ > + > + // Call the account search method on each importer. The use of executeSoon > + // is to prevent race conditions when calling observe() on the UI from the > + // importers. > + for each (let importer in this._importers) { for ... of @@ +89,5 @@ > + Cu.reportError("Importer " + importer.id + " has stalled."); > + executeSoon(function() { > + this.observe(importer, "account-search-finished", null); > + }.bind(this)); > + }.bind(this), kFindAccountsStallSeconds); More bind calls we no longer need. @@ +100,5 @@ > + this.observe(importer, "account-search-finished", null); > + } > + }.bind(this)); > + } > + catch (e) { Nit: we usually write } catch (e) { on a single line. ::: chat/importers/test/importerTestHelper.jsm @@ +10,5 @@ > + // Create a unique instance of an importer. > + getImporterJSInstance: function(aImporterName) { > + let cid; > + let importerName = aImporterName.toLowerCase(); > + if (importerName === "googletalk") Why is there a hard coded list of supported importers here? ::: chat/importers/test/xpcshell.ini @@ +1,5 @@ > +[DEFAULT] > +head = > +tail = > + > +[test_ImportersService.js] should be a lowercase i here: test_importersService.js (and also rename the file) @@ +4,5 @@ > + > +[test_ImportersService.js] > + > + > + More whitespace than needed here.

This adds fixes for the nits found in the previous review. The testing code was also removed, since there were actually no tests created for the backend itself (yet) and it complicated the patch needlessly.

Comment on attachment 8393839 [details] [diff] [review] Unbitrotted backend fixes from review Review of attachment 8393839 [details] [diff] [review]: ----------------------------------------------------------------- I'm not done reviewing yet, but my train is arriving at the station so I figured I would publish what I already have. ::: chat/components/public/imIImportersService.idl @@ +36,5 @@ > + > +[scriptable, uuid(59fd4143-c8ab-4e55-b139-98b57e10bcee)] > +interface imIImporter: nsISupports { > + readonly attribute AUTF8String name; > + readonly attribute AUTF8String id; Should we add a comment here specifying what a name looks like and what an id looks like? Is the name something that's for showing to the user? Any expectations about how the id should be constructed? @@ +39,5 @@ > + readonly attribute AUTF8String name; > + readonly attribute AUTF8String id; > + > + // Timer used to handle stalled importers. > + attribute nsITimer stallTimer; I don't see why this is exposed in the idl interface. @@ +54,5 @@ > + // Asynchronously import data related to aAccount. This method handles the > + // data that isn't required before connecting the account and can take awhile > + // to locate or convert. This method should be written in an iterative way so > + // that, when called, it can resume where it left off using information found > + // in aPreference. An 'import-status-updated' notification should be fired to What's aPreference here? Is it a full preference path? Just something used in a specific branch? What does "An 'import-status-updated' notification should be fired" mean? What's firing this notification? What's consuming it? ::: chat/components/src/imImporters.js @@ +12,5 @@ > + "nsICategoryManager"); > + > +const kImporterPluginCategory = "im-import-plugin"; > +const kImportIdleSeconds = 60; > +const kFindAccountsStallSeconds = 10000; You are using this in a setTimeout call, it's a value in ms, not seconds, right? @@ +26,5 @@ > + } > + }, > + > + unInitImporters: function() { > + if (this._monitoringIdleness) { Nit: we often do if (!foo) return; to avoid indenting the whole content of a function. @@ +35,5 @@ > + }, > + > + _idle: false, > + _monitoringIdleness: false, > + _monitorIdleness: function() { This is called only once; inline? @@ +45,5 @@ > + this._monitoringIdleness = true; > + }, > + > + // Determines whether there is data awaiting import. > + _pendingImport: function() { _pendingImport is called only once, and is a one liner; inline it? :-) @@ +52,5 @@ > + > + // Returns an array of account ids with pending import tasks. > + _pendingAccounts: function() { > + let accts = Services.prefs.getCharPref("messenger.accounts").split(","); > + return accts.filter(function(acct) { Nit: you don't need the accts variable here. return Services.prefs. ... .filter(... is as readable. @@ +58,5 @@ > + return Services.prefs.prefHasUserValue(prefName); > + }); > + }, > + > + findAccounts: function(aObserver) { Should this throw an error if there's already a previous findAccounts call in progress? @@ +59,5 @@ > + }); > + }, > + > + findAccounts: function(aObserver) { > + this._findAccObserver = aObserver; You probably don't want to keep a reference to aObserver forever. So it should likely be removed when you are done searching for accounts. And in the error/no importer case, you can likely just avoid saving it. @@ +63,5 @@ > + this._findAccObserver = aObserver; > + > + // Locate the registered importers and create instances. > + this._importers = []; > + let importers = this.getImporters(); This getImporters methods is creating an array of importer, and then creates an nsISimpleEnumerator from the array, so that you can then create an array from the enumerator here. And it's called only once. Inline and remove the iterator cruft? :) @@ +70,5 @@ > + > + // If there are no importers registered, the observer still needs a signal. > + if (!this._importers.length) { > + executeSoon(() => { > + this._findAccObserver.observe(this, "account-search-finished", null); I think you can replace "this._findAccObserver.observe" here by just aObserver (it's called through xpconnect so I think it will behave like a function here). @@ +80,5 @@ > + // is to prevent race conditions when calling observe() on the UI from the > + // importers. > + for (let importer of this._importers) { > + try { > + let findAccounts = importer.findAccounts; What's the point of this variable? @@ +83,5 @@ > + try { > + let findAccounts = importer.findAccounts; > + > + // Prevent stalled importers from going unnoticed. > + importer.stallTimer = setTimeout(() => { Why do we need a timer for each importer? It seems the timers all have the same length and aren't reset when the importer returns some data, so all the timers will finish at the same time. Could we have a single timer instead, that looks for remaining active importers in this._importers? Also, in case of a timeout, do we need a way to tell the importers to abort whatever they were doing? Is this something we also need if the user closes the account wizard while we were attempting to detect accounts? (a 'cancel', 'stop' or 'abort' method on both the importer service, and on each importer?) @@ +93,5 @@ > + }, kFindAccountsStallSeconds); > + > + executeSoon(() => { > + try { > + findAccounts(this); You are calling this as a function, not as a method, so the value of 'this' won't be set. But the implementation seems to use this. I think you really wanted |importer.findAccounts(this);| here.

Comment on attachment 8393839 [details] [diff] [review] Unbitrotted backend fixes from review Review of attachment 8393839 [details] [diff] [review]: ----------------------------------------------------------------- I wrote plenty of comments, but they are all mostly straight forward to address, and overall this looks pretty good! :-) ::: chat/components/public/imILogger.idl @@ +69,5 @@ > [optional] in boolean aGroupByDay); > nsISimpleEnumerator getSystemLogsForAccount(in imIAccount aAccount); > nsISimpleEnumerator getSimilarLogs(in imILog aLog, > [optional] in boolean aGroupByDay); > + void logConversation(in prplIConversation aConversation, Can we have a comment in the idl file about what this does? ::: chat/components/src/imImporters.js @@ +161,5 @@ > + if (!this._idle) > + return; > + > + let importer = this.getImporterById(JSON.parse(pref).importer); > + let importData = importer.importData; Why this variable? @@ +176,5 @@ > + > + // If we have received this notification and there is no pending import, > + // the idle observer can be removed; it would be added again if new > + // accounts were selected for import. > + let accts = this._pendingAccounts(); Almost all the code around here in the 'idle' case is duplicated from the 'import-status-updated' case. Do you need a 'doNextTask' method or something? @@ +212,5 @@ > + } > + return new nsSimpleEnumerator(importers); > + }, > + > + getImporterById: function(aImporterId) { Nit: this method can be 2 lines shorter and slightly easier to follow: let importer = null; try { let cid = categoryManager.getCategoryEntry(kImporterPluginCategory, aImporterId); try { importer = Cc[cid].createInstance(Ci.imIImporter); } catch (e) { // The importer is registered and failed to init. Cu.reportError("failed to create an instance of " + cid + ": " + e); } } catch (e) { // No importer registered for this id. Cu.reportError(e); } return importer; ::: chat/modules/jsImporterHelper.jsm @@ +69,5 @@ > + // The XPCShell tests access the importer JS Object. > + get wrappedJSObject() this, > + > + // Methods defined in the imIImporter interface. > + // These should be overridden by any importer implementations. If these methods need to be overridden by all implementations, shouldn't they throw instead of having dummy implementations? @@ +97,5 @@ > + this._account = aAccount; > + this._isChat = aIsChat; > + this._observers = []; > + // TODO: Method of assigning ID outside of ConversationService? > + this._id = Math.random() * 9999; Would this be a cause of random failures in tests later? @@ +117,5 @@ > + removeObserver: function(aObserver) { > + this._observers = this._observers.filter(function(o) o !== aObserver); > + }, > + notifyObservers: function(aSubject, aTopic, aData) { > + for each (let observer in this._observers) for ... of

I've addressed the above comments. Compatible UI and Pidgin importer patches are pasted at the following links to try the code out or reference. http://pastebin.instantbird.com/858971 http://pastebin.instantbird.com/858972 Overall the changes were straightforward unless noted below. I still have some concerns about the design of the data importing in general. The repeated "checking in" by the importers and saving status to prefs seems unorthodox, but the other alternative I've thought of (just letting the importers go until told to stop and save status) might be less robust in case of abrupt application exit. > ::: chat/components/src/imImporters.js > @@ +12,5 @@ > > + "nsICategoryManager"); > > + > > +const kImporterPluginCategory = "im-import-plugin"; > > +const kImportIdleSeconds = 60; > > +const kFindAccountsStallSeconds = 10000; I moved these time values to preferences to aid debugging, and because it seemed reasonable that they might need to be changed in some cases. You are using this in a setTimeout call, it's a value in ms, not seconds, right? > > @@ +83,5 @@ > > + try { > > + let findAccounts = importer.findAccounts; > > + > > + // Prevent stalled importers from going unnoticed. > > + importer.stallTimer = setTimeout(() => { > > Why do we need a timer for each importer? It seems the timers all have the > same length and aren't reset when the importer returns some data, so all the > timers will finish at the same time. Could we have a single timer instead, > that looks for remaining active importers in this._importers? Added a single timer and removed the unnecessary interface property for it. > Also, in case of a timeout, do we need a way to tell the importers to abort > whatever they were doing? Is this something we also need if the user closes > the account wizard while we were attempting to detect accounts? (a 'cancel', > 'stop' or 'abort' method on both the importer service, and on each importer?) Added an abort method to the importers and the ImportersService. Difficult to test for now since none of the current importers take long to finish finding accounts. > ::: chat/components/public/imILogger.idl > @@ +69,5 @@ > > [optional] in boolean > > aGroupByDay); > > nsISimpleEnumerator getSystemLogsForAccount(in imIAccount aAccount); > > nsISimpleEnumerator getSimilarLogs(in imILog aLog, > > [optional] in boolean aGroupByDay); > > + void logConversation(in prplIConversation aConversation, > > Can we have a comment in the idl file about what this does? The logger changes this past summer made this no longer necessary. Instead, I had to expose the observe() method on the Logger so that my conversations could notify `new-text' as each message is created. To avoid adding observe() to the interface, I would need a method like I had, but it would really just let the Logger add itself as an observer rather than my code doing that. > ::: chat/modules/jsImporterHelper.jsM > @@ +97,5 @@ > > + this._account = aAccount; > > + this._isChat = aIsChat; > > + this._observers = []; > > + // TODO: Method of assigning ID outside of ConversationService? > > + this._id = Math.random() * 9999; > > Would this be a cause of random failures in tests later? There was no existing way to get a safe ID for a conversation outside of adding it to the ConversationsService and consequently creating a UI conversation. My attempt to address this was creating a getNewPrplConversationId() method on the ConversationsService to expose access to the internal ID counter. My other thought was to create a method that wrapped up more of the first part of addConversation() that I need (the ID creation, and maybe sending the new-conversation notification) and use that when I create my conversation.

Comment on attachment 8477751 [details] [diff] [review] Importing backend review fixes, some unbitrotting Review of attachment 8477751 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the update! :-) ::: chat/components/public/imIConversationsService.idl @@ +51,5 @@ > void initConversations(); > void unInitConversations(); > > + // Retrieve an id to uniquely identify a conversation. > + long getNewPrplConversationId(); maybe "Unique" instead of "New" in the name? ::: chat/components/public/imILogger.idl @@ +96,5 @@ > jsval forEach(in imIProcessLogsCallback aCallback); > + > + // Allow the Logger to be added as an observer by outside code. > + void observe(in nsISupports aObj, in string aEvent, > + [optional] in wstring aData); I don't think you need this. ::: chat/components/src/imImporters.js @@ +41,5 @@ > + delete this._idleService; > + }, > + > + // Returns an array of account ids with pending import tasks. > + _pendingAccounts: function() { If it's a function, name it _getPendingAccounts @@ +71,5 @@ > + // If there are no importers registered, the observer still needs a signal. > + if (!this._importers.length) { > + executeSoon(() => { > + aObserver.observe(this, "account-search-finished", null); > + }); Should we reset _findAccountsPending here? @@ +105,5 @@ > + this._stallTimer = setTimeout(() => { > + delete this._stallTimer; > + for (let importer of this._importers) { > + Cu.reportError("Importer " + importer.id + " has stalled."); > + } nit: you don't need the {} here. @@ +127,5 @@ > + let prefName = "messenger.account." + aAccountId + ".import"; > + if (Services.prefs.prefHasUserValue(prefName)) { > + // The UI should not allow the user to add the same account twice. > + let error = "Account " + aAccountId + " has an existing importer " + > + "preference: " + Services.prefs.getCharPref(prefName); I'm not exactly sure why we have that restriction. I see no problem with importing logs from 2 different other clients for the same account. If the reason is because of some implementation details, that may be OK for a first version of the patch though :-). @@ +134,5 @@ > + } > + > + // Write the initial import preference. > + let pref = JSON.stringify({account: aAccountId, importer: aImporterId}); > + Services.prefs.setCharPref(prefName, pref); Should we ensure at this point that we have an idle observer in place? @@ +192,5 @@ > + executeSoon(() => { > + try { > + importer.importData(account, pref, this); > + } catch(e) { > + Cu.reportError("Error in importer importData(): " + e.message); should we clear the preference in this case? Won't we run into the same exception forever if we keep trying to import the same thing? ::: chat/modules/jsImporterHelper.jsm @@ +77,5 @@ > + abort: function() { > + throw "abort not implemented."; > + }, > + importData: function(aAccount, aStatus, aObserver) { > + throw "importData not implemented."; any reason why these 3 aren't throw Cr.NS_ERROR_NOT_IMPLEMENTED;? @@ +81,5 @@ > + throw "importData not implemented."; > + }, > + > + // Helper methods for observer notifications. > + _returnAccount: function(aExistingAccount) { what about naming this _notifyAccountFound @@ +84,5 @@ > + // Helper methods for observer notifications. > + _returnAccount: function(aExistingAccount) { > + this._observer.observe(aExistingAccount, "existing-account-found", null); > + }, > + _endAccountSearch: function() { and _notifySearchFinished? @@ +98,5 @@ > + this._account = aAccount; > + this._isChat = aIsChat; > + this._id = Services.conversations.getNewPrplConversationId(); > + this._observers = []; > + this.addObserver(Services.logs); These 2 lines could be merged into: this._observers = [Services.logs]; I suggested earlier that you don't expose the observe method in the imILogger interface. You can .QueryInterface(Ci.nsIObserver) here instead. I'm not entirely convinced using the new-text notification is the right thing to do here, as that will cause some significant overhead in the logger which will asynchronously open and close the log file for each message. For a version landing pref'ed off that's good enough, but I would suggest talking to nhnt11 to see if the two of you can find an efficient way to log a whole conversation at once. @@ +126,5 @@ > + // Called by the importers when the conversation has been fully imported. > + unInit: function() { > + delete this._account; > + delete this._observers; > + }, I guess I'll need to look at the importer example codes to figure this one, but I don't really see why the conversation can't just be garbage collected without any uninitialization. ::: im/app/profile/all-instantbird.js @@ +98,5 @@ > pref("messenger.globalProxy", "none"); > pref("messenger.warnOnQuit", true); > > +// Sets whether the account wizard searches for existing accounts to import. > +pref("messenger.importWizard.enabled", false); You need to set this preference to false in chat/ otherwise we'll have an exception when checking the pref at startup on Thunderbird.

(In reply to Will Nayes [:wnayes] from comment #14) > Overall the changes were straightforward unless noted below. I still have > some > concerns about the design of the data importing in general. The repeated > "checking in" by the importers and saving status to prefs seems unorthodox, > but > the other alternative I've thought of (just letting the importers go until > told > to stop and save status) might be less robust in case of abrupt application > exit. Might the new-ish AsyncShutdown help you here? http://dutherenverseauborddelatable.wordpress.com/2014/02/14/shutting-down-things-asynchronously/ http://dutherenverseauborddelatable.wordpress.com/2014/05/26/shutting-down-asynchronously-part-2/ https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/AsyncShutdown.jsm

Addressed the above review comments. As before, the corresponding compatible UI and pidgin importers have been pasted: http://pastebin.instantbird.com/861732 http://pastebin.instantbird.com/861733 > @@ +127,5 @@ > > + let prefName = "messenger.account." + aAccountId + ".import"; > > + if (Services.prefs.prefHasUserValue(prefName)) { > > + // The UI should not allow the user to add the same account twice. > > + let error = "Account " + aAccountId + " has an existing importer " + > > + "preference: " + Services.prefs.getCharPref(prefName); > > I'm not exactly sure why we have that restriction. I see no problem with > importing logs from 2 different other clients for the same account. > If the reason is because of some implementation details, that may be OK for > a first version of the patch though :-). The way the UI is currently implemented requires selecting which client to import from, which is why this was an error. It would be nice to have it merge accounts from different clients though! I rewrote the import pref to have an array of importer ids, so the backend should now be compatible with this change when the UI is changed. > @@ +134,5 @@ > > + } > > + > > + // Write the initial import preference. > > + let pref = JSON.stringify({account: aAccountId, importer: aImporterId}); > > + Services.prefs.setCharPref(prefName, pref); > Should we ensure at this point that we have an idle observer in place? The UI was calling initImporters after it finished all queueing, so the observer was being initialized. This didn't seem great so I now have queueAccount set up the observer if it needs to, which removes the UI's burden to do so. > @@ +192,5 @@ > > + executeSoon(() => { > > + try { > > + importer.importData(account, pref, this); > > + } catch(e) { > > + Cu.reportError("Error in importer importData(): " + e.message); > should we clear the preference in this case? Won't we run into the > same exception forever if we keep trying to import the same thing? I think this is reasonable. There might be a need to allow the importers to recover from resource availability issues too (if logs are on a server and it can't be reached now). This would need a different mechanism for the importers to let the service know they wish to be started up again at a later time. > ::: im/app/profile/all-instantbird.js > @@ +98,5 @@ > > pref("messenger.globalProxy", "none"); > > pref("messenger.warnOnQuit", true); > > > > +// Sets whether the account wizard searches for existing accounts to import. > > +pref("messenger.importWizard.enabled", false); > You need to set this preference to false in chat/ otherwise we'll > have an exception when checking the pref at startup on Thunderbird. Moved the prefs into chat-prefs.js since they relate to the importing backend itself.

Comment on attachment 8477927 [details] [diff] [review] Review fixes, backed supports multiple importers per account Review of attachment 8477927 [details] [diff] [review]: ----------------------------------------------------------------- Looks good :-). I only have a few comments that should be easy to address. ::: chat/components/src/imImporters.js @@ +18,5 @@ > + // Called when the core initializes. If data importing has not been completed, > + // it will be scheduled in the idleService. > + initImporters: function() { > + if (!Services.prefs.getBoolPref("messenger.importWizard.enabled")) > + return; Now that most of the code has been moved to _startIdleObserver, you don't really need the early return. (remove the ! and use &&) @@ +130,5 @@ > + // Record the intent to import data within a preference. > + let prefName = "messenger.account." + aAccountId + ".import"; > + let pref; > + if (Services.prefs.prefHasUserValue(prefName)) { > + pref = JSON.parse(Services.prefs.getCharPref(prefName)); If the user has hand-edited the pref and put something that isn't valid JSON, should we catch the error and overwrite the value as if it didn't exist yet? @@ +168,5 @@ > + Services.prefs.clearUserPref(prefName + ".status"); > + > + // Remove the importer that just finished, and the pref if none remain. > + let pref = JSON.parse(Services.prefs.getCharPref(prefName)); > + pref.importers.shift(); If you don't use the returned value here, you could as well just test .length == 1 on the next line. @@ +170,5 @@ > + // Remove the importer that just finished, and the pref if none remain. > + let pref = JSON.parse(Services.prefs.getCharPref(prefName)); > + pref.importers.shift(); > + if (!pref.importers.length) > + Services.prefs.clearUserPref(prefName); or did you mean to add an else here and save the resulting array after shifting?

(In reply to Will Nayes [:wnayes] from comment #17) > Created attachment 8477927 [details] [diff] [review] > Review fixes, backed supports multiple importers per account > > Addressed the above review comments. As before, the corresponding compatible > UI and pidgin importers have been pasted: > > http://pastebin.instantbird.com/861732 > http://pastebin.instantbird.com/861733 Could you please attach this on bugzilla (either here or in other bugs)? pastebin's UI isn't exactly reviewer-friendly ;-).

This should address the latest review comments. I'll create bugs for the UI and Pidgin patches now that the backend is close to ready.

Comment on attachment 8478010 [details] [diff] [review] Remaining review fixes Thanks!

Is there anything left that needs to be done to land this? I'd be happy to help if it's not *too* much work to finish this off, since the lack of account importing is one of the main things keeping me from using Instantbird at the moment (and I'd really like to).

(In reply to Jim Porter (:squib) from comment #22) > Is there anything left that needs to be done to land this? Not really, but it's useless without landing bug 1057873 and bug 1057874 too. I think help to finish this would be very appreciated :-).