Closed Bug 954928 Opened 10 years ago Closed 3 years ago

Create an account import wizard - GSoC 2012

Categories

(Chat Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: wnayes, Assigned: wnayes)

References

(Depends on 1 open bug)

Details

Attachments

(7 obsolete files)

*** Original post on bio 1495 at 2012-06-08 15:46:00 UTC ***

This bug will be a location for various code reviews during the development of the Import Wizard functionality for the Google Summer of Code 2012. For more information on the project, please visit:

http://www.tc.umn.edu/~nayes006/gsoc2012/
*** Original post on bio 1495 as attmnt 1574 at 2012-06-08 16:18:00 UTC ***

This was the first patch reviewed in a more formal matter. Parts of the discussion on IRC are posted below, full log available: http://log.bezut.info/instantbird/120607/

> <flo> why is there an executeSoon call in pidgin.js's start method?

I assumed it would need to be there for the same reasons it was needed in the test importer (calls to observe() when the .purple directory is not found which are outside of the asynch file read).

> <flo> |for (var i = 0; i < accounts.length; ++i) {| var -> let. You seem to have lots of lines that could throw inside that loop if the xml file isn't structured strictly like you expect
> <flo> you may want to do some error checking, or add a try/catch

I could see some error catching for the name and protocol parsing, though the length in the loops should handle the rest of the potential NodeList being empty issues. I will add a try/catch as well.

> <flo> would "im-import-plugin" sound better than "im-importer-plugin"? (really not sure)
> <flo> I don't see you you use NetUtil.readInputStreamToString followed by xmlParser.parseFromString instead of something like xmlParser.parseFromStream(stream, null, stream.available(), "text/xml");
<flo> "existingAccount" -> "ExistingAccount" (upper case for the first later of a constructor name)
> <flo> this._autoLogin = false; (and the next 3 lines in the constructor) -> move that to the prototype
> <flo> only objects and array need to be initialized in the constructor to avoid issues when sharing them between instances
> <flo> the dateModified setter and getter seem useless. When you have both a getter and a setter (ie you aren't using the getter as a way to make the property read only) and the only thing they do is forward to a property of the object, that property could be used directly
> <flo> so instead of having |_time: 0, get dateModified() this._time, set dateModified(aDate) this._time = aDate,| you can have only |dateModified: 0,| in the prototype
> <flo> that comment applies for the dateModified, autoLogin, password and alias properties
> <flo> the code in existingAccount.setBool (and setInt and setString) is a bit complicated, and seems duplicated between the 3 methods. I haven't made the effort of fully understanding it because I think you will refactor it anyway if you start using the prplIPref interface directly
> <flo> you may want to use the ClassInfo helper from imXPCOMUtils.jsm instead of implementing getInterfaces yourself

I would agree with all of these, and will work on these changes today. Thanks for the review!
Attachment #8353329 - Flags: review-
Assignee: nobody → wnayes
Status: NEW → ASSIGNED
*** Original post on bio 1495 at 2012-06-08 16:35:55 UTC ***

(In reply to comment #1)

> > <flo> why is there an executeSoon call in pidgin.js's start method?
> 
> I assumed it would need to be there for the same reasons it was needed in the
> test importer (calls to observe() when the .purple directory is not found which
> are outside of the asynch file read).

OK. If we end up with this situation for all importers (this situation = there are cases where the code is much more readable if we return a result, typically an error, synchronously), maybe we should just more the executeSoon call to the importers service.
*** Original post on bio 1495 as attmnt 1630 at 2012-06-18 21:59:00 UTC ***

The main change from the last patch is the addition of the mIRC and XChat importers, along with their associated XPCShell tests.

I'm not sure if it would be easier to look over the files in the repository, as the patch is growing pretty large.

http://hg.instantbird.org/users/wnayes/file/af5930d287b9

The main place of interest would be the new folders in chat/importers.

Thanks!
Attachment #8353387 - Flags: feedback?(florian)
Comment on attachment 8353329 [details] [diff] [review]
First review - Importer Service and Pidgin Importing

*** Original change on bio 1495 attmnt 1574 at 2012-06-18 21:59:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353329 - Attachment is obsolete: true
Comment on attachment 8353387 [details] [diff] [review]
Second review - Pidgin, mIRC, and XChat importers

*** Original change on bio 1495 attmnt 1630 at 2012-06-19 17:44:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353387 - Flags: feedback+
*** Original post on bio 1495 at 2012-06-19 17:44:23 UTC ***

Comment on attachment 8353387 [details] [diff] [review] (bio-attmnt 1630)
Second review - Pidgin, mIRC, and XChat importers

Note that I'm skipping all test and UI code for now.

>diff --git a/chat/components/public/imIImportersService.idl b/chat/components/public/imIImportersService.idl
>+interface imIExistingAccount: nsISupports {
>+  /* This time is read from files associated with
>+     the existing accounts and is used to approximate which
>+     account is more relevant if a conflict occurs. A value
>+     of 0 indicates this is unknown. */
Couldn't this actually be implemented differently if a client actually writes when they last updated a time? Also, your comments seem to all have different formatting...which bothers me, but it isn't a big deal right now. :) (Some are /*\n*\n*/ others are /*\n\n*/.)

>diff --git a/chat/importers/mIRC/Makefile.in b/chat/importers/mIRC/Makefile.in
>@@ -0,0 +1,22 @@
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
Nit: I think these should all line up? (Check all the makefiles for this.)

>diff --git a/chat/importers/mIRC/mIRC.js b/chat/importers/mIRC/mIRC.js
>+function mIRCImporter() {
>+  // The XPCShell tests access the JS Object of an importer.
>+  this.wrappedJSObject = this;
Hmm...I wonder if there should be an GenericImporterPrototype, like what we use for JS protocols...either way, you should be able to put this in the prototype instead of the constructor, I think.

>+}
>+mIRCImporter.prototype = {
>+    /* executeSoon is used to prevent race conditions when calling
>+       observe() on the UI. */
>+    executeSoon(function() {
It's pretty gross doing this everytime, but I think you and flo discussed this already.

>+      // mIRC is a Windows only client. Testing is done under the OS "XPCShell".
>+      let supportedOS = [ "WINNT", "XPCShell" ];
This feels very wrong, I think in the test you'd want to code WINNT instead. (Then below you can directly check if Services.appinfo.OS == "WINNT".) I assume we don't care if someone is running mIRC under wine or something? ;)

>+      let optList = this._getINIValue(mircIniFile, "options", "n0");
>+      if (optList)
>+        foundAccount.autoLogin = (optList.charAt(0) == "1" ? true : false);
You shouldn't need the ternary operator here, you should just be able to do: foundAccount.autoLogin = optList[0] == "1";

>+  abort: function() {
>+    // Implement when using threading in start()/log parsing
Mark this as TODO?

>+  /* mIRC stores application settings in a "mIRC" directory
>+   * found in the Windows user's AppData directory.  */
>+  _getMircDirectory: function() {
>+    // Reroute the test directory for xpcshell tests.
>+    if (this.testDir)
>+      return this.testDir;
>+
>+    let dirService = Cc["@mozilla.org/file/directory_service;1"]
>+                     .getService(Components.interfaces.nsIProperties);
Nit: Line the period up with the [ from the line above.

>diff --git a/chat/importers/pidgin/pidgin.js b/chat/importers/pidgin/pidgin.js
>+/* The importer below contains methods to retrieve accounts
>+ * from the instant messaging client, Pidgin.
I think this will work for any libpurple client (e.g. Finch also).

>+  /* Pidgin stores profile information with a ".purple" directory.
>+   * In Windows this directory can be found in the Roaming AppData
>+   * directory. In Linux/OS X, this directory is found in the user
>+   * home directory. 
>+   *
>+   * For more information:
>+   *  http://developer.pidgin.im/wiki/Using%20Pidgin#Whereismy.purpledirectory
>+   */
>+  _getPurpleDirectory: function() {
>+    // For use with xpcshell testing to set an arbitrary .purple location.
>+    // testDir is set by the test to a nsIFile instance.
>+    if (this.testDir)
>+      return this.testDir;
This doesn't seem right. I think in the test you'd want to override this method to just return a test directory.

>+   * The names of these settings correspond to Instantbird's namings due to
>+   * the use of libpurple. */
Is this fully true for things like prpl-facebook / prpl-gtalk / prpl-irc?

>+  _parseSettings: function(aSettings, aExistingAccount) {
[...]
>+          case "bool":
>+            if (name == "auto-login")
>+              aExistingAccount.autoLogin = (value == "1" ? true : false);
>+            else
>+              aExistingAccount.setBool(name, (value == "1" ? true : false));
Again here, you don't need the ternary operator.

>diff --git a/chat/importers/xchat/xchat.js b/chat/importers/xchat/xchat.js

>+    There are several more entries in the default servlist_.conf, which
>+    follow the above pattern. This method parses this format by splitting
>+    by double newlines (\n\n) to get each server section, then splitting
>+    by the single newline (\n) to get each sections individual name=value
>+    pairs. The target key is "S=", which will give the server url. */
Do we know what these other keys are? Are they information we should be keeping?

>diff --git a/chat/modules/jsImporterHelper.jsm b/chat/modules/jsImporterHelper.jsm
>+const EXPORTED_SYMBOLS = [
>+  "ExistingAccount"
>+];
Nit: Put this on one line, please.

>+const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
Nit: We usually only want the variables we actually use in that file. I think most of your files have everything defined (Cc, Ci, Cr and Cu), check if they're necessary.

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
>@@ -41,17 +41,18 @@ const EXPORTED_SYMBOLS = [
>   "GenericAccountBuddyPrototype",
>   "GenericConvIMPrototype",
>   "GenericConvChatPrototype",
>   "GenericConvChatBuddyPrototype",
>   "GenericMessagePrototype",
>   "GenericProtocolPrototype",
>   "ForwardProtocolPrototype",
>   "Message",
>-  "TooltipInfo"
>+  "TooltipInfo",
>+  "purplePref"
I think these were in alphabetical order?
Blocks: 954309
*** Original post on bio 1495 as attmnt 1738 at 2012-07-12 21:49:00 UTC ***

This patch is my progress so far through the first half of GSoC 2012. The new importers since the last patch:

Google Talk: Accounts are read from the Windows registry. The CryptUnprotectData calls and password deobfuscation is complete. Tests are written checking the password entropy generation.

Windows Live Messenger: The Windows Credential Store module provides the password decryption for the account information.

AIM 6.x, 7.x: A JavaScript blowfish implementation was written to decrypt the AIM client passwords. The cipher has xpcshell tests written to verify against known test vectors.

Colloquy: The code is untested, but I was able to find some information on the property list files and write some code similar to the Safari migration code from Firefox.

The executeSoon calls have been removed in favor of a single call in the ImportersService. The importers have been removed from their subfolders and reside right in chat/importers (except for AIM).
Attachment #8353498 - Flags: feedback?(florian)
Comment on attachment 8353387 [details] [diff] [review]
Second review - Pidgin, mIRC, and XChat importers

*** Original change on bio 1495 attmnt 1630 at 2012-07-12 21:49:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353387 - Attachment is obsolete: true
Attachment #8353387 - Flags: feedback?(florian)
Attached patch GSoC Final Submission (obsolete) — Splinter Review
*** Original post on bio 1495 as attmnt 1814 at 2012-08-20 17:30:00 UTC ***

This is my final submission patch that contains the changes for my Instantbird Import Wizard GSoC 2012 project.

The notable change from the midterm patch is the inclusion of log importing functionality into the ImportersService, which occurs during idle after importing accounts. The UI is also fully functional.
Attachment #8353573 - Flags: review?(florian)
Comment on attachment 8353498 [details] [diff] [review]
GSoC Midterm Patch - Google Talk, Colloquy, WLM, and AIM Importers Added

*** Original change on bio 1495 attmnt 1738 at 2012-08-20 17:30:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353498 - Attachment is obsolete: true
Attachment #8353498 - Flags: feedback?(florian)
Depends on: 954943, 955079
*** Original post on bio 1495 at 2012-08-30 12:20:14 UTC ***

Comment on attachment 8353573 [details] [diff] [review] (bio-attmnt 1814)
GSoC Final Submission

A few random comments:
- The patch has Windows line endings. That makes hg import dislike it ;).
- The patch doesn't touch instantbird/installer/package-manifest.in, so if it landed as is, you would be quite disappointed. To test a packaged build, you can run "make package" in the objdir.
- The change in instantbird/content/tabbrowser.css looks like it wants to be reverted.

Some questions:
- Is Colloguy the only client that we can import from on Mac?
- Shouldn't we include in the imported logs some meta data indicating when it was imported, by which version of Instantbird, from which client, and maybe even from which original log file? I don't think we would want to display that in the UI, but it could be useful later if we ever actually implement log synchronization. It could also be useful if we fix bugs in some importers in the future and want to make them re-import and replace previously imported logs.
- Shouldn't the JS XPCOM components of each importer have a prefix in their names, so that they don't conflict in the future with implementation of these protocols? (names like aim.js seem particularly risky to me for instance)
- It seems you have included the patch from bug 954542 (bio 1108) but I don't see how it's related to the import wizard, can you clarify this?


Here is a screenshot of the summary step of the wizard for me on Mac:
http://i.imgur.com/cjrrE.png
As you can see there's an alignment issue between the account name and the "Will be created" text, and there's not enough horizontal space for the "Add another account" button.
Attached patch Changes based on review comments (obsolete) — Splinter Review
*** Original post on bio 1495 as attmnt 1882 at 2012-09-01 16:34:00 UTC ***

This patch should fix some of the observations made about the last one. The line endings should be correct this time.

> Is Colloguy the only client that we can import from on Mac?
Colloquy is the only Mac exclusive importer so far, but Pidgin and XChat should be included in every build.

> Shouldn't the JS XPCOM components of each importer have a prefix in their
> names, so that they don't conflict in the future with implementation of these
> protocols? (names like aim.js seem particularly risky to me for instance)
I've prefixed the importers in this patch. Each is named "importer-[client].js".

> It seems you have included the patch from bug 954542 (bio 1108) but I don't see how it's
> related to the import wizard, can you clarify this?
I've removed that patch code now. When I reviewed 1108 I was thinking it would be committed by this point, so it was left in.

> Shouldn't we include in the imported logs some meta data indicating when it
> was imported, by which version of Instantbird, from which client, and maybe
> even from which original log file?
I did not address this yet. I am thinking this could be done by adding a some new flags to the importer conversations, and then add them to the array of flags the logger checks.

> Here is a screenshot of the summary step of the wizard for me on Mac:
> http://i.imgur.com/cjrrE.png
> As you can see there's an alignment issue between the account name and the
> "Will be created" text, and there's not enough horizontal space for the "Add
> another account" button.
The label issue was addressed by changing the <description> elements to be <label>s, so the margins should be the same for all labels and OS styles. I am not sure why the wizard window does not have a normal dialog container around it. It seems like if there was equal left/right spacing around the content that the buttons would fit OK. The richlistbox sizing seems to suggest there should be more margin/padding of the dialog on the right as well. Maybe what I am seeing is due to how a screenshot tool rendered the window?
Attachment #8353640 - Flags: review?(florian)
Comment on attachment 8353573 [details] [diff] [review]
GSoC Final Submission

*** Original change on bio 1495 attmnt 1814 at 2012-09-01 16:34:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353573 - Attachment is obsolete: true
Attachment #8353573 - Flags: review?(florian)
*** Original post on bio 1495 at 2012-09-19 10:20:02 UTC ***

Comment on attachment 8353640 [details] [diff] [review] (bio-attmnt 1882)
Changes based on review comments

Sorry for the long delay here (I'm sure you understand that reviewing such a large patch takes a while though :)).
I reviewed the changes to chat/components/.
I have lots of comments, but I think they are all relatively easy to address, and otherwise things look quite good there :-).

I will review the changes to instantbird/ and chat/modules/, and for chat/importers/ I think I'll ask for some help from other developers.

Here are the comments I have about chat/components/:

>diff --git a/chat/components/public/imIImportersService.idl b/chat/components/public/imIImportersService.idl

>+interface imIExistingAccount: nsISupports {

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

>+  nsISimpleEnumerator getOptions();
>+
>+  // Set the protocol specific options

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?
My assumption here is that the imIExistingAccount defines that the account wizard can expect.

>+  void setBool(in string aName, in boolean aVal);
>+  void setInt(in string aName, in long aVal);
>+  void setString(in string aName, in AUTF8String aVal);
>+};


>+[scriptable, uuid(59fd4143-c8ab-4e55-b139-98b57e10bcee)]
>+interface imIImporter: nsISupports {
>+  readonly attribute AUTF8String name;
>+  readonly attribute AUTF8String id;
>+
>+  // Searches for accounts configured by the client software.
>+  // When found, the imIImportersService is notified.

So is it imIImportersService or aObserver that's notified?
(I suspect it's "aObserver, which will usually be the imIImportersService").

>+  //
>+  // Supported notifications:

"Supported notification" makes me think that the object supports receiving these notifications and doing something useful with them. I think you meant "Notifications fired:". Or just rephrase so that it's part of the same sentence as the one saying what will be notified.

>+  //  importer-found-account
>+  //    aSubject points to the imImportedAccount found.
>+  //  importer-search-finished
>+  void findAccounts(in nsIObserver aObserver);
>+
>+  // Handles the task of importing data for an account found from findAccounts.
>+  // This method should update its status after completing a task by notifying
>+  // 'import-status-updated' with any needed status information in aData.

"This method should update its status".

This makes me think:
- that something isn't implemented like it should be yet, or that it's not reliable.
- that 'status' is somehow an attribute either of this object, or of this method(?).

Also, you want to be more specific than "data".

What about something like:
"Asynchronously import data related to aAccount. This method handles the data that isn't required before connecting the account for the first time and can take a while to read/fetch/convert (eg. conversation logs). aObserver will receive an 'import-status-updated' notification when the import is finished."

>+  //
>+  // Supported notifications:
>+  //  import-status-updated
>+  //    aSubject is the aAccount which owns the data.
>+  //    aData is the preference string with updated status.
>+  void importData(in imIAccount aAccount, in string aPreference,

What's aPreference?
Why does it have the "string" type?


>+[scriptable, uuid(d1f32c53-2272-4852-9564-4ab00f92b4dd)]
>+interface imIImportersService: nsISupports {
>+  // Sets the service to observe idle notifications if pending importable data
>+  // remains.
>+  void initImporters();

This comment could be rephrased to be more readable, but I think it says what you mean, which is already a good thing :).


>+  // Closes any observation of idle notifications.
>+  void unInitImporters();

What is this going to do to the import jobs that are currently running?


>+  // Each importer checks for available accounts.

This isn't clear.
Maybe "Tell each importer to start checking asynchronously for existing accounts on the system." ?

>+  // The observer (UI) will be notified of new accounts discovered.

"observer (UI)" isn't clear. Maybe you meant "aObserver, typically implemented by the UI,"?


>+  // 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?
Can importers tell us immediately if there's nothing to import, so that we don't queue pointless import jobs?

>+  void queueAccount(in string aAccountId, in string aImporterId);

Again, why "string" rather than AUTF8String or ACString or AString?

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

>diff --git a/chat/components/public/imILogger.idl b/chat/components/public/imILogger.idl

>+  // Adds a conversation manually, optionally dated in the past.
>+  void logConversation(in prplIConversation aConversation,
>+                       [optional] in PRTime aDate);

What would you think of making startDate an attribute of prplIConversation?
(another benefit is it would let us finally have a correct implementation of timeOpened in headerFooterReplacements (chat/modiles/imThemes.jsm).

>diff --git a/chat/components/src/imImporters.js b/chat/components/src/imImporters.js
>+ImportersService.prototype = {
>+  // Called when the core initializes. If data importing has not been completed,
>+  // it will be scheduled in the idleService.
>+  initImporters: function() {
>+    let enabled = Services.prefs.getBoolPref("messenger.importWizard.enabled");
>+    if (this._pendingImport() && enabled)

- I don't think you want to call this._pendingImport if the pref says the import is disabled.
- I don't think you need the enabled variable, you can just inline the getBoolPref call.


>+  },
>+
>+  unInitImporters: function() {
>+    if (this._monitoringIdleness) {
>+      this._idleService.removeIdleObserver(this, kImportIdleSeconds);
>+      this._monitoringIdleness = false;
>+      this._idleService = null;

I would use "delete this._monitoringIdleness;" rather than setting it again to the default value. Deleting makes the value fall back to what's defined in the prototype. Same for this._idleService.

In this case, it's almost a coding style nit.

>+  // Determines whether there is data awaiting import.
>+  _pendingImport: function() {
>+    let accounts = Services.prefs.getCharPref("messenger.accounts").split(",");
>+    for each (let accountId in accounts) {
>+      let prefName = "messenger.account." + accountId + ".import";
>+      if (Services.prefs.prefHasUserValue(prefName))
>+        return true;
>+    }
>+    return false;

It's a bit sad that you are touching the messenger.accounts preferences directly, but I can understand that you don't want to deal with the nsISimpleEnumerator that Services.accounts.getAccounts returns.

This function looks a lot like you wanted to use the .some method of arrays:

_pendingImport: function()
  Services.prefs.getCharPref("messenger.accounts").split(",").some(function(accountId)
    Services.prefs.prefHasUserValue("messenger.account." + accountId + ".import")

(This compact way of writing it makes it exceed the 80 chars limit, so if you want to respect it you can keep the { return } and the accounts and prefName variables.)


>+  findAccounts: function(aObserver) {
>+    this._findAccObserver = aObserver;
>+
>+    // Locate the registered importers and create instances.
>+    this._importers = [];
>+    let importers = this.getImporters();
>+    while (importers.hasMoreElements())
>+      this._importers.push(importers.getNext().QueryInterface(Ci.imIImporter));
>+
>+    // If there are no importers registered, the observer still needs a signal.
>+    if (!this._importers.length) {
>+      this._findAccObserver.observe(this, "account-search-finished", null);

Firing a synchronous notification here defeats the purpose of the executeSoon thing explained in the next comment, doesn't it?

>+      return;
>+    }
>+
>+    // 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) {
>+      try {
>+        let findAccounts = importer.findAccounts;

What's the point of this variable?

>+        executeSoon(function() {
>+          try {
>+            findAccounts(this);
>+          } catch(e) {
>+            Cu.reportError("Error in importer findAccounts(): " + e.message);
>+            this.observe(importer, "account-search-finished", null);
>+          }
>+        }.bind(this));
>+      }
>+      catch (e) {
>+        Cu.reportError("Error calling findAccounts(): " + e.message);

In this case, is there more value in the string "Error calling findAccounts(): " are in the location information that e contains?
If the location information is more valuable, you should just do Cu.reportError(e);

>+      }
>+    }
>+  },
>+
>+  queueAccount: function(aAccountId, aImporterId) {
>+    let prefName = "messenger.account." + aAccountId + ".import";
>+    if (Services.prefs.prefHasUserValue(prefName)) {
>+      // Throw an error if the pref exists; maybe there is further action to
>+      // take here? 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);
>+      Cu.reportError(error);
>+      return;

The comment says you are throwing an error, but you are actually reporting the error and then continuing without throwing.
Please either change the comment to "Report an error" or actually throw (with the |throw| keyword) if this is something that really shouldn't happen, and should stop the execution if it ever happens.

>+  observe: function(aSubject, aTopic, aData) {

I wonder why you compare the value of the aTopic string with === and not just ==.

>+    else if (aTopic === "import-status-updated") {
>+      let [account, pref] = [aSubject, aData];
>+      let prefName = "messenger.account." + account.id + ".import";
>+
>+      // The import is finished for this account.
>+      if (!pref) {

Maybe swap these 2 lines so that it's obvious that the comment refers to what's executed if the test passes?
And please make it a bit more explicit, for example say that receiving the import-status-updated notification without data paramater means the import is done.

>+        Services.prefs.clearUserPref(prefName);
>+        return;
>+      }
>+      Services.prefs.setCharPref(prefName, pref);
>+
>+      // The user has returned from idle, yet importing remains. The
>+      // next task will be postponed until idle occurs again.
>+      if (!this._idle)
>+        return;
>+
>+      let importer = this.getImporterById(JSON.parse(pref).importer);
>+      let importData = importer.importData;
>+      executeSoon(function() {
>+        try {
>+          importData(account, pref, this);
>+        } catch(e) {
>+          Cu.reportError("Error in importer importData(): " + e.message);

Same comment as above about the value of the "Error in importer importData" string versus location information; and about the importData variable.

>+        }
>+      }.bind(this));
>+    }
>+    else if (aTopic === "idle") {
>+      this._idle = true;
>+
>+      // If we have received this notification and there is no pending import,
>+      // the idle observation can be removed;

I think I would write "the idle observer can be removed" or "the idle observation can stop" (with a slight preference for the former).

>+      let accts = Services.prefs.getCharPref("messenger.accounts").split(",");
>+      for each (let accountId in accts) {
>+        // Check accounts for the pref indicating awaiting importable data.
>+        let prefName = "messenger.account." + accountId + ".import";
>+        if (!Services.prefs.prefHasUserValue(prefName))
>+          continue;

.filter method of the array?

Also, it seems you are duplicating here what the _pendingImport method will do, so maybe you just want to check if the resulting array after .filter has a non-zero length instead of calling _pendingImport?

>+        let dataObj = Services.prefs.getCharPref(prefName);
>+        dataObj = JSON.parse(dataObj);

.map method of the array?

>+        // Distribute initial call to import the data. The service observer
>+        // will make future calls as necessary.
>+        let account = Services.accounts.getAccountById(accountId);
>+        let importer = this.getImporterById(dataObj.importer);
>+        let importData = importer.importData;
>+        executeSoon(function() {
>+          try {
>+            importData(account, JSON.stringify(dataObj), this);
>+          } catch(e) {
>+            Cu.reportError("Error in importer importData(): " + e.message);
>+          }
>+        }.bind(this));

Do you really want to start all the importers at once? Shouldn't they be queued instead?


>+  getImporters: function() {
>+    let importers = [];
>+    let entries = categoryManager.enumerateCategory(kImporterPluginCategory);
>+    while (entries.hasMoreElements()) {
>+      let id = entries.getNext().QueryInterface(Ci.nsISupportsCString).data;
>+      let importer = this.getImporterById(id);
>+      if (importer)
>+        importers.push(importer);
>+    }
>+    return new nsSimpleEnumerator(importers);
>+  },
>+
>+  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?).

>+    }
>+
>+    let importer = null;
>+    try {
>+      importer = Cc[cid].createInstance(Ci.imIImporter);
>+    } catch (e) {
>+      // This is a real error, the importer is registered and failed to init.
>+      let error = "failed to create an instance of " + cid + ": " + e;
>+      Cu.reportError(error);

I don't think you need an "error" variable here.
By the way, this is a case where the cid value added to the message is more valuable than the location information that e contained.

>+    }
>+    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)?

>+
>+    return importer;
>+  },

>diff --git a/chat/components/src/logger.js b/chat/components/src/logger.js

Changes to this file may suffer from some bitrot from https://bugzilla.mozilla.org/show_bug.cgi?id=787149
Sorry about this.

>-function getNewLogFileName(aFormat)
>+function getNewLogFileName(aFormat, aDate)
> {
>   let date = new Date();
>+  if (aDate)
>+    date = aDate;

You meant:
let date = aDate || new Date();

> /* Conversation logs stuff */
>-function ConversationLog(aConversation)
>+function ConversationLog(aConversation, aDate)

As mentioned earlier, I would prefer the date information to be contained in the prplIConversation object.

> {
>   this._conv = aConversation;
>+  this._date = aDate;
> }

>   _getHeader: function cl_getHeader()
>   {
>     let account = this._conv.account;
>     if (this.format == "json") {
>-      return JSON.stringify({date: new Date(),
>+      return JSON.stringify({date: (this._date ? this._date : new Date()),

this._date || new Date()

Although I would really prefer this._conv.startDate or something like that ;).

>                              name: this._conv.name,
>                              title: this._conv.title,
>                              account: account.normalizedName,
>                              protocol: account.protocol.normalizedName,
>                              isChat: this._conv.isChat
>                             }) + "\n";
>     }
>     return "Conversation with " + this._conv.name +
>-           " at " + (new Date).toLocaleString() +
>+           " at " + (this._date ? this._date : new Date()).toLocaleString() +

(this._date || new Date()).toLocaleString()

>            " on " + account.name +
>            " (" + account.protocol.normalizedName + ")" + kLineBreak;
>   },
>   _serialize: function cl_serialize(aString) {
>     // TODO cleanup once bio 102699 is fixed
>     let doc = getHiddenHTMLWindow().document;
>     let div = doc.createElementNS("http://www.w3.org/1999/xhtml", "div");
>     div.innerHTML = aString.replace(/\r?\n/g, "<br/>").replace(/<br>/gi, "<br/>");
>@@ -147,17 +166,17 @@ ConversationLog.prototype = {
>         flags: ["outgoing", "incoming", "system", "autoResponse",
>                 "containsNick", "error", "delayed",
>                 "noFormat", "containsImages", "notification",
>                 "noLinkification"].filter(function(f) aMessage[f])
>       };
>       let alias = aMessage.alias;
>       if (alias && alias != msg.who)
>         msg.alias = alias;
>-      this._log.writeString(JSON.stringify(msg) + "\n");
>+      this._log.writeMessage(JSON.stringify(msg) + "\n");

Why haven't you kept the writeString name for the new LogWriter objects?
The writeMessage method just forwards the call, so it would make sense to reuse the same name I think.

>@@ -167,17 +186,18 @@ ConversationLog.prototype = {
>         line += sender + " <AUTO-REPLY>: " + msg;
>       else {
>         if (/^\/me /.test(msg))
>           line += "***" + sender + " " + msg.replace(/^\/me /, "");
>         else
>           line += sender + ": " + msg;
>       }
>     }
>-    this._log.writeString(line + kLineBreak);
>+    //this._log.writeString(line + kLineBreak);
>+    this._log.writeMessage(line + kLineBreak);

Dead code ;).

> SystemLog.prototype = {
>   _log: null,
>   _init: function sl_init(aAccount) {

>+    this._log = new LogWriter(file);

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

> 
>+  // Adds a conversation not notifying the global observer service.

I don't understand this comment, please rephrase.

>+  logConversation: function(aConversation, aDate) {
*** Original post on bio 1495 at 2012-09-19 10:27:18 UTC ***

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?
*** Original post on bio 1495 at 2012-09-19 11:04:26 UTC ***

I just tested the UI.
There's something magical in seeing it find accounts that are on the system and being able to configure everything in just 2 clicks. It's exciting! :-)

The appearance needs some work on Mac, here are screenshots of what I saw:
http://i.imgur.com/X0PHJ.png
- The text says "press Next" but the button is labeled "Continue".
- The "Skip >" button on the left side seems strange.
- Why aren't we just showing the accounts that we have found?

http://i.imgur.com/zRHQc.png
- Not enough horizontal space for the "Add another account" button on the same line as the other buttons.
- I understood your argument earlier about the vertical space used by each account and the need for it because of the potential menulist if we have found the same account in more than one client. I'm wondering if we aren't optimizing for a corner case though.
- Wouldn't we want to show an icon for each program that we are importing from, next to the program name? Or are we concerned about copyright/licensing issues?

http://i.imgur.com/Ogeq0.png
- "1 existing account*s*" :-( See https://developer.mozilla.org/en-US/docs/Localization_and_Plurals

http://i.imgur.com/6ogNc.png
- I could only test the Colloguy importer as I'm on a Mac. There seems to be a problem with the import of the nickname, as my testcolloguy@irc.freenode.net account becomes florian@irc.freenode.net

After closing the wizard, I would have expected the new accounts to connect right away.
*** Original post on bio 1495 at 2012-09-19 12:16:50 UTC ***

(In reply to comment #9)
> >+  // 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?
> Can importers tell us immediately if there's nothing to import, so that we
> don't queue pointless import jobs?
Does this obey whether logging is disabled?

I'll try to look over some of this code soon (maybe on my red eye Friday night). (Probably the chat/modules and chat/importers changes.)
*** Original post on bio 1495 at 2012-09-19 16:03:11 UTC ***

Comment on attachment 8353640 [details] [diff] [review] (bio-attmnt 1882)
Changes based on review comments

Review comments for the changes to instantbird/

diff --git a/instantbird/content/accountWizard.css b/instantbird/content/accountWizard.css

+richlistitem[checked="false"] .showChecked,
+richlistitem[checked="true"] .hideChecked,

The descendant selector is very expensive (see https://developer.mozilla.org/en-US/docs/CSS/Writing_Efficient_CSS for details). I think you can avoid it with an xbl:inherit="checked" or two.

+.hiddenSource
+{

Nit: you used the "selector {" format for other rules in this file.


diff --git a/instantbird/content/accountWizard.js b/instantbird/content/accountWizard.js
--- a/instantbird/content/accountWizard.js
+++ b/instantbird/content/accountWizard.js
@@ -3,208 +3,422 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 Cu.import("resource:///modules/imServices.jsm");
 
 const PREF_EXTENSIONS_GETMOREPROTOCOLSURL = "extensions.getMoreProtocolsURL";
 
 var accountWizard = {
+  // As accounts are configured, the settings for each will be kept in a array

an array

+  observe: function am_observe(aObject, aTopic, aData) {

+    else if (aTopic == "existing-account-found") {
+      // Add a new item to the accountSummaryList with an existingAccount binding
+      let accountSummaryList = document.getElementById("accountSummaryList");
+      for (let i = 0; i < accountSummaryList.itemCount; ++i) {
+        let listedAccount = accountSummaryList.getItemAtIndex(i);
+        if (listedAccount.className !== "existingAccount")
+          return;

This needs a comment (why are we returning early if we find an account with the "existingAccount" class in the list?). And I'm wondering if you really meant return or if you meant continue here (hard to say for sure, as I'm not sure of what you are trying to do with this test).

+      accountSummaryList.appendChild(item);
+      item.build(aObject);
+      this.searchCount++;

Nit: ++this.searchCount;

+  showWelcomePage: function() {

+    // The import wizard functionality may be turned off (hidden).
+    if (Services.prefs.getBoolPref("messenger.importWizard.enabled"))
+      this.startImportSearch();

Do you really want to restart the search if the user has just pressed the back button?

+  skipAccountImport: function() {

Shouldn't this have a way to actually stop the import/search for accounts? (what will happen if we receive more existing-account-found notifications after hiding the welcome page?)

+  selectProtocol: function aw_selectProtocol() {
+    let pageId = document.getElementById("accountWizard").currentPage.pageid;
+    let listId = pageId == "accounttoplist" ? "topprotolist" : "protolist";
+    let protoList = document.getElementById(listId);
+
+    // Create an object to store the values for this account.
+    this.accounts.pop();

Why the .pop here?

What happens if you configure an account, then a second one, then press back to check something about the latest account you configured, then see it's fine and press next until you are on the summary page again. Wouldn't we drop an account (or at least its advanced options) in this case?


+  advanceUsernamePage: function aw_advanceUsernamePage() {
+    // Store the username in the accounts stack. We want to be able to retrieve
+    // both the complete name and the individual textbox values (if applicable)
+    this.getCurrentAccount().username.value = this.getUsername();
+    this.getCurrentAccount().username.sections = [];
+    for each (let box in this.userNameBoxes)
+      this.getCurrentAccount().username.sections.push(box.value);

Why so many this.getCurrentAccount().username calls?


+  showPasswordPage: function aw_showPasswordPage() {
+    let password = this.getCurrentAccount().password;
+    document.getElementById("password").value = (password ? password : "");

    document.getElementById("password").value =
      this.getCurrentAccount().password || "";


@showAdvanced: function aw_showAdvanced() {
     if (proxyVisible) {
-      this.proxy = Components.classes["@instantbird.org/purple/proxyinfo;1"]
-                             .createInstance(Ci.purpleIProxyInfo);
-      this.proxy.type = Ci.purpleIProxyInfo.useGlobal;
+      this.getCurrentAccount().proxy = Cc["@instantbird.org/purple/proxyinfo;1"]
+                                         .createInstance(Ci.purpleIProxyInfo);
+      this.getCurrentAccount().proxy.type = Ci.purpleIProxyInfo.useGlobal;

Use a proxy variable to avoid the duplicated this.getCurrentAccount().proxy?


+    let alias = this.getCurrentAccount().alias;
+    let aliasBox = document.getElementById("alias");
+    aliasBox.value = (alias ? alias : "");

alias || ""
(I'm sure you could guess this comment at this point :-))

 
@   populateProtoSpecificBox: function aw_populate() {
       case opt.typeBool:
-        var chk = document.createElement("checkbox");
+        let chk = document.createElement("checkbox");
         chk.setAttribute("label", text);
         chk.setAttribute("id", name);
-        if (opt.getBool())
-          chk.setAttribute("checked", "true");
+        let chkVal = (savedVal ? savedVal : (opt.getBool() ? "true" : "false"));

Why do you need to convert the boolean value to a string yourself?

+        chk.setAttribute("checked", chkVal);

Are you sure that checked="false" is the same as no checked attribute?
(When I look at http://hg.mozilla.org/mozilla-central/annotate/80499f04e875/toolkit/content/widgets/listbox.xml#l1057, the getter looks like it would accept a "false" value, but the setter uses removeAttribute rather than setting it to false.)

         box.appendChild(chk);
         break;
       case opt.typeInt:
-        box.appendChild(this.createTextbox("number", opt.getInt(),
-                                           text, name));
+        let intVal = (savedVal ? savedVal : opt.getInt());

savedVal || opt.getInt()
(and same comment for the next two preference types.)


+  clearSummaryList: function(aItemClass) {
+    let summaryList = document.getElementById("accountSummaryList");
+    for (let i = (summaryList.itemCount - 1); i >= 0 ; --i) {

Why the parens?

+      let entry = summaryList.getItemAtIndex(i);
+      if (aItemClass && entry.className === aItemClass)
+        summaryList.removeItemAt(i);
+      else if (!aItemClass)
+        summaryList.removeItemAt(i);

You meant:
      if (!aItemClass || entry.className === aItemClass)

+  createAccounts: function aw_createAccounts() {

+        let autologin = entry.autologin;
+        acc.autoLogin = autologin;

I don't see where this is in the current UI.

+        if (window.opener) {
+          let am = window.opener.gAccountManager;
+          if (am)
+            am.selectAccount(acc.id);

It seems like this code should be called only once when we are creating multiple accounts.
Do we want to select the first or the last account that we create?

+      else if (entry.className == "newAccount") {

The code here is almost the same. Can you find a way to reduce this duplication?

+    // This initiates the data importing process. Data will be imported in the
+    // background for the user during idle. The process will continue if the
+    // program is restarted.
+    if (Services.prefs.getBoolPref("messenger.importWizard.enabled"))
+      Services.importers.initImporters();

Should this be only if we have actually created an imported account?

 
   getValue: function aw_getValue(aId) {
-    var elt = document.getElementById(aId);
+    let elt = document.getElementById(aId);

All these trivial changes make the real changes to this file difficult to review.
To avoid this for the next iteration of the patch, I would suggest that you file a bug (or send me through whatever mean you like a patch) for the var->let change only, that I commit it quickly and that you do the hg diff for the next iteration of the patch after updating to a revision that already includes this change.

You shouldn't have any conflict to deal with: just use a separate clean tree to generate the var->let patch, and once it's commited, backup your current version of the file, hg up, ignore the conflicts and hg revert the file, and put your backup back in place.

+  openURL: function(aURL) {
+    let urlUri = Services.io.newURI(aURL, null, null);

urlUri doesn't make much sense as a variable name. Why not just "uri"? 

+    Cc["@mozilla.org/uriloader/external-protocol-service;1"]
+      .getService(Ci.nsIExternalProtocolService).loadUrl(urlUri);

Or why do you even need that intermediate variable instead of putting .loadUrl(...) on a third line?


diff --git a/instantbird/content/accountWizard.xml b/instantbird/content/accountWizard.xml

+  <binding id="existingAccount" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
+    <content>
+      <xul:vbox flex="1">
+        <xul:hbox flex="1" align="center">

Why do you need these 2 boxes?
Was <content flex="1" align="center"> not working?

+          <xul:vbox>
+            <xul:checkbox anonid="accountSelected"
+                          xbl:inherits="checked" class="summaryCheckbox"
+                          oncommand="document.getBindingParent(this).updateChecked();"/>
+          </xul:vbox>
+          <xul:vbox>
+            <xul:image xbl:inherits="src=prplicon" class="summaryIcon"/>
+          </xul:vbox>

These 2 vboxes contain only one child. Why are they needed?

+          <xul:vbox flex="1">
+            <xul:hbox align="baseline">
+              <xul:label xbl:inherits="value=username" class="summaryName"/>
+              <xul:separator flex="1" hidden="true"/>

Is there anything that unhides this separator?

+              <xul:checkbox label="&accountSummary.connectAutomatically.label;"
+                            dir="reverse" class="summaryAutologinCheckbox" hidden="true"
+                            anonid="autologin"/>

I don't see any code unhidding this.

+            </xul:hbox>
+            <xul:hbox align="center" class="textGroup">
+              <xul:label class="showChecked" value="&accountExisting.doImport.label;"/>
+              <xul:label class="hideChecked" value="&accountExisting.noImport.label;"/>
+              <xul:label class="summaryImporterText showChecked" anonid="singleClient"/>

This will be painful for screenreader users:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#553

I think you will need to either reimplement this with a single label (and change the value in updateChecked), or add a "label" getter to the implementation.


+  <binding id="newAccount" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
+    <content align="top">

My comment about vboxes that have a single child applies here too.

+      <xul:vbox flex="1">
+        <xul:hbox flex="1" align="center">
+          <xul:vbox>
+            <xul:checkbox anonid="accountSelected"
+                          xbl:inherits="checked" class="summaryCheckbox"
+                          oncommand="document.getBindingParent(this).updateChecked();"/>
+          </xul:vbox>
+          <xul:vbox>
+            <xul:image xbl:inherits="src=prplicon" class="summaryIcon"/>
+          </xul:vbox>
+          <xul:vbox flex="1">
+            <xul:hbox align="baseline">
+              <xul:label xbl:inherits="value=username" class="summaryName"/>
+              <xul:separator flex="1" hidden="true"/>
+              <xul:checkbox label="&accountSummary.connectAutomatically.label;"
+                            dir="reverse" class="summaryAutologinCheckbox" hidden="true"
+                            anonid="autologin" checked="false"/>

The comment about this being hidden applies too.

And more generally I wonder if we really needed to duplicate the binding. If we can't use the same for both cases, maybe one could inherit from the other, and we would have only one implementation of the common things?


diff --git a/instantbird/content/accountWizard.xul b/instantbird/content/accountWizard.xul

   <wizardpage id="accountsummary" pageid="accountsummary"
               label="&accountSummaryTitle.label;"
+              onextra2="accountWizard.addAnotherAccount();"

Do we have an extra2 button without an extra1 button?


diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties

+skipButton.label=Skip >

I'm not sure the '>' symbol applies if you can't be sure that button will be on the right side.

+searchStatus.success.label=Instantbird was able to find %S existing accounts on your computer. To view what was found, press Next.

As I said in my previous comment, this needs a localized plural.

+searchStatus.failure.label=Instantbird could not find any existing accounts to import. To configure a new account, press Next.

Why are we showing this rather than jumping directly to the top protocol page, or just hiding the additional elements of the welcome page that are related to importing accounts?


+summaryItem.clienttext.label=from %S

I think you wanted to write clientText, and this desperately needs a localization note to explain where it will be visible, and what %S will be replaced with.

diff --git a/instantbird/test/xpcshell.ini b/instantbird/test/xpcshell.ini

This hunk no longer applies after https://bugzilla.mozilla.org/show_bug.cgi?id=787984 landed. Trivial to fix though :).


diff --git a/instantbird/themes/accountWizard.css b/instantbird/themes/accountWizard.css

+richlistitem[checked="false"] .summaryName,
+richlistitem[checked="false"] .hideChecked
+{
+  color: GrayText;
+}

Why do you need to specify .summaryName and .hideChecked? Are there elements within richlistitem[checked="false"] items that shouldn't get that color rule?

The text color of an unchecked selected account in the summary richlistbox isn't correct on Mac (light grey on dark blue isn't readable). When the account is checked, it's correct (the text color is white). I assume we need to find a way to make the rule setting the color to GrayText less specific than the one setting the color to white when the item is selected.

Nit: remove the line break before {

+richlistitem[checked="false"] .summaryIcon {
+  opacity: 0.3;
+}

Any way to avoid the descendant selector here?
*** Original post on bio 1495 at 2012-09-21 14:22:20 UTC ***

Comment on attachment 8353640 [details] [diff] [review] (bio-attmnt 1882)
Changes based on review comments

Review comments for changes to chat/modules/:

diff --git a/chat/modules/blowfish.jsm b/chat/modules/blowfish.jsm

I won't pretend to understand this code, so my comments for this file are only coding style nits.

I wonder how the performance of |[xL, xR] = [xR, xL];| compares to |let tmp = xL; xL = xR; xR = tmp;|, but the data you encrypt and decrypt may not be big enough for performance to really matter. (This comment isn't requesting a change, I'm just sharing what I thought while reading the code).
[Update: some quick testing discussed on IRC (thanks Mic!) showed that the destructuring assignment solution is ~20 times slower. Still not sure if that matter for your use cases though.]

+function Blowfish(aKey) {

+  // Calculate the subkeys based on the provided key and initial P and S arrays.
+  // Pad the key to 56 bytes, cycling the given key values.
+  let j = 0;
+  let keyArray = [aKey[i] for (i in aKey)];
+  while (keyArray.length < 72) {
+    keyArray.push(keyArray[j++]);
+    if (j >= aKey.length)
+      j = 0;

What about:
  keyArray.push(keyArray[j]);
  j = (j + 1) % aKey.length;

+  }
+  aKey = Uint8Array(keyArray);
+
+  // XOR the P array with 32-bit sections of the key.
+  for (let i = 0; i < this._P.length; ++i) {
+    // TODO: Use DataView in Gecko 15.

Note: by the time this patch lands, Instantbird will likely be based on Gecko 15 or newer.

+    for (let j = 0; j < 4; ++j)
+      keyInt = ((keyInt << 8) | aKey[(i * 4) + j]) >>> 0;

What does >>> 0 do?

+  encrypt: function(aPlainBytes) {

+    for (let i = 0; i < blockArray.length; i += 2) {
+      let encryptedBlock = this._encryptBlock(blockArray.subarray(i, i+2));

i + 2 rather than i+2

+      result.push(encryptedBlock[0]);
+      result.push(encryptedBlock[1]);

result.push(encryptedBlock[0], encryptedBlock[1]);

+  decrypt: function(aEncryptedBytes) {

Same 2 comments as for the encrypt method.



diff --git a/chat/modules/jsImporterHelper.jsm b/chat/modules/jsImporterHelper.jsm

+const EXPORTED_SYMBOLS = [
+  "ExistingAccount",
+  "GenericImporterPrototype",
+  "ImporterConversation"

ImporterConversation or ImportedConversation? (probably doesn't matter)

+ExistingAccount.prototype = {

+  getOptions: function() {
+    if (!this._options)
+      return EmptyEnumerator;

You initialized this._options to [] in the constructor, so this test will always be false.

+  setString: function(aName, aVal) {
+    if (this._setOption(aName, aVal, Ci.prplIPref.typeString, "getString"))
+      return;

_setOption can return false either if the type is wrong, or if we are setting an option to its default value. Is this what this code expects?

+    this._setOption(aName, aVal, Ci.prplIPref.typeList, "getListDefault");
+  },
+  _setOption: function(aName, aVal, aType, aGetMethod) {
+    let protoId = this._protoId;

This is used only once, you can inline it.

+    let protoOptions = Services.core.getProtocolById(protoId).getOptions();
+    while (protoOptions.hasMoreElements()) {
+      let opt = protoOptions.getNext().QueryInterface(Ci.prplIPref);
+      if (opt.type == aType && opt.name == aName) {
+        // Create a new preference with the new value (in 'default' property)
+        let newOpt = new purplePref(aName, {label: opt.label, default: aVal});

I think I'm confused by this. Why do we need in the ExistingAccount implementation a getOptions method similar to what protocols (not accounts) have? You just want a list of key/value pairs, don't you?

Idea for another simpler solution:
this._options = {};
set*: function(aName, aVal) { this._options[aName] = aVal; } (we may still want to check that the new value is actually different from the existing default)

get options() this._options,

In the imIExistingAccount interface:
readonly attribute nsIVariant options;

I haven't tested this, so I'm not sure it actually works, but if it does I think the code would be simplified.


+const GenericImporterPrototype = {

+  // Methods defined in the imIImporter interface.
+  findAccounts: function(aObserver) {
+    this._observer = aObserver;
+    this._endAccountSearch();
+  },
+  importData: function(aAccount, aPreference, aObserver) {
+    this._observer = aObserver;
+    this._updateImportStatus(aAccount, null);
+  },

So these 2 implementations will cause the importer to do strictly nothing, and fire the notification saying it's done immediately, right?
Is there any case where this is actually the code we want to execute, or is this just here for easy copy/pasting?
I think this need at least a better comment saying that they need to be overridden.


+function ImporterConversation(aName, aAccount, aIsChat, aBuddy) {
+  this._name = aName;
+  this._account = aAccount;
+  this._isChat = aIsChat;
+  this.buddy = aBuddy;

Why the different treatment for buddy (it's a readonly attribute too in prplIConvIM)?

diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
@@ -319,19 +320,25 @@ const GenericMessagePrototype = {

-    if (aObject)
-      for (let i in aObject)
-        this[i] = aObject[i];
+    if (aObject) {
+      for (let i in aObject) {
+        if (i !== "conversation")
+          this[i] = aObject[i];
+      }
+      // The message should not be sent off until all properties are set.
+      if (aObject.conversation)
+        this.conversation = aObject.conversation;
+    }

I don't understand why you need this change.
In your ImporterConversation.prototype implementation of writeMessage, the conversation property is already set after all the others.

diff --git a/chat/modules/winCredentialStore.jsm b/chat/modules/winCredentialStore.jsm

+const LPBYTE = ctypes.PointerType(ctypes.jschar);
+const LPTSTR = ctypes.PointerType(ctypes.jschar);
+const LPCTSTR = ctypes.PointerType(ctypes.jschar);

Do these 3 pointer types really map to the same thing? (disclaimer: I've never used ctypes, so someone else should probably review this file :-)).

+const PVOID = ctypes.PointerType(ctypes.void_t);
+
+// "Contains a 64-bit value representing the number of 100-nanosecond intervals
+// since January 1, 1601 (UTC)."
+//  http://msdn.microsoft.com/en-us/library/ms724284.aspx
+const FILETIME = ctypes.StructType("FILETIME", [
+        {'dwLowDateTime': DWORD},

The 8-space indent is odd here.
And do you really need the quotes around the names used as object keys?

+CredentialStore.prototype = {

+  getCredentials: function(aFilter) {

+    let credArray = [];
+    for (let i = 0; i < count.value; ++i) {
+      let cred = new Credential(castCredentials.contents[i].contents);
+      credArray.push(cred);

You don't need the cred variable.
*** Original post on bio 1495 at 2012-09-22 04:41:50 UTC ***

>diff --git a/chat/importers/importer-aim.js b/chat/importers/importer-aim.js
>+function aim6Importer() { }
>+  findAccounts: function(aObserver) {
[...]
>+    // Close the handles on the Windows registry.
>+    passwordsKey.close();
>+    aimKey.close();
>+    aolKey.close();
>+    swKey.close();
>+    this._endAccountSearch();
Won't these not be closed if we fail in the middle of the search for the key? (I.e. if there's no passwords key, we bail out early).

>+aim6Importer.prototype.decryptPassword = decryptPassword;
Why add this to the aim6Importer? If it is used for both, can't we just call the global decryptPassword function?

>+function aim7Importer() { }
>+  findAccounts: function(aObserver) {
>+    this._observer = aObserver;
>+
>+    let aimxfile = this._getAIMXFile();
This is only called once, can we inline it?

>+    // A StreamListener implementation is needed to receive the asynchronously
>+    // decompressed string.
>+    let myListener = new StreamListener();
I dislike "myListener", can we come up with a better name?

>+    // The aimx.bin file is compressed with DEFLATE. The asynchronous method
>+    // of nsIStreamConverter must be used as the synchronous is not implemented.
>+    let converter = Cc["@mozilla.org/streamconv;1?from=deflate&to=uncompressed"]
>+                      .createInstance(Ci.nsIStreamConverter);
>+    converter.asyncConvertData("deflate", "uncompressed", myListener, null);
>+    converter.onStartRequest(null, null);
>+    converter.onDataAvailable(null, null, stream, 0, stream.available());
>+    converter.onStopRequest(null, null, 201);
>+
>+    // The result of the uncompression is a string containing an AIM username
>+    // and Base64 encoded password separated by '-'.
>+    let decompressed = myListener.data;
>+    let userData = decompressed.split("-");
Why not just:
let [username, encodedPass] = decompressed.split("-");
if (!username || !encodedPass)

>+    // Base64 decode the password string and Blowfish decrypt the blocks.
>+    let encodedPass = encodedPass.slice(0, encodedPass.length - 3);
What's with the - 3?

>+// AIM6 and AIM7 use the same Blowfish encryption techniques.
>+// This method handles calls to the Blowfish cipher module, and returns the
>+// user's plaintext password. It is called with the encrypted password text.
>+function decryptPassword(aPassword) {
>+  // Read the individual character codes from the encrypted password string.
>+  let passArray = [aPassword.charCodeAt(i) for (i in aPassword)];
>+
>+  // The first 8 bytes of the encrypted password string are a "salt". To
>+  // create the Blowfish key, this salt is prepended to a known static key,
>+  // creating a 448-bit key, the maximum allowed by a Blowfish implementation.
This known static key is an AOL specific thing, right?

>+// StreamListener implementation for AIM 7.x file decompression.
>+function StreamListener() {
>+  this.data = "";
Why not in the prototype?

This is an impressive bit of decryption, etc. Great job!

>diff --git a/chat/importers/importer-colloquy.js b/chat/importers/importer-colloquy.js
>+function colloquyImporter() { }
>+colloquyImporter.prototype = {
>+    let accounts = aPropertiesRoot.get("MVChatBookmarks");
>+    for each (let account in accounts) {
>+      // Colloquy website says there is IRC, SILC, and ICB support.
>+      // TODO: Are XMPP settings supported?
XMPP settings, you say above this it only supports IRC, SILC and ICB!? (Also, I'd rephrase that comment to be "Colloquy supports IRC...", but more confident. :))
Depends on: 955133
Depends on: 955277
Attached patch UI patch unbitrotted (obsolete) — Splinter Review
Attachment #8353640 - Attachment is obsolete: true
Attachment #8353640 - Flags: review?(florian)
Attachment #8392006 - Flags: review?(florian)
It looks like this already has some detailed unaddressed review comments above, or am I missing something?
Flags: needinfo?(wnayes)
Comment on attachment 8392006 [details] [diff] [review]
UI patch unbitrotted

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

::: im/content/accountWizard.js
@@ +108,5 @@
> +
> +  hideWelcomePage: function() {
> +    let wizard = document.getElementById("accountWizard");
> +    wizard.getButton('extra1').hidden = true;
> +    wizard.getButton('next').disabled = false;

Nit: Why are there single quotes here? I think the rest of the file uses double quotes.
Found some nits similar to the other reviews and fixed them. Fixed the account searching status icon CSS, which seemed to have been missing from the patches for some time. Also fixed the quote inconsistency mentioned above.

The UI may deserve its own bug separate from this one, which has been used mostly for tracking the entire project.
Attachment #8392006 - Attachment is obsolete: true
Attachment #8392006 - Flags: review?(florian)
Attachment #8393863 - Flags: review?(florian)
Flags: needinfo?(wnayes)
Depends on: 1057873
Attachment #8393863 - Flags: review?(florian)
Attachment #8393863 - Attachment is obsolete: true
Depends on: 1057874

I'm going to close this since it has been a while and this never landed.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: