Closed Bug 714733 Opened 13 years ago Closed 12 years ago

Instant messaging in Thunderbird

Categories

(Thunderbird :: Instant Messaging, enhancement)

Product:
Thunderbird
Component:
Instant Messaging
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 13.0

People

(Reporter: florian, Assigned: florian)

References

(Depends on 8 open bugs, Blocks 1 open bug,
URL
)

Details

Attachments

(6 files, 35 obsolete files)

AccountManager.js bugfix
4.79 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
Fix crash @nsMsgDBFolder::HandleAutoCompactEvent
886 bytes, patch
Bienvenu
: review+
Details | Diff | Splinter Review
Let Gloda noun definitions specify a tableName
1.24 KB, patch
protz
: review+
Details | Diff | Splinter Review
above patch in action
78.18 KB, image/png
Details
Screenshot of attachment 604280
260.64 KB, image/png
Details
Patch v12 unbitrotted
1.66 MB, patch
standard8
: review+
Details | Diff | Splinter Review
Attached patch WIP 1 (obsolete) — — Splinter Review 
I'm going to track my work on the "instant messaging in Thunderbird" feature here.

The discussion for the feature is summarized on the wiki in a feature page:
https://wiki.mozilla.org/Features/Thunderbird/Instant_messaging_in_Thunderbird

The attached patch is a work in progress. With it, it's already possible to use Google Talk, Facebook Chat and Twitter.

The UI isn't finished, we need to add at least:
- a way to change the user's status
- a way to add IM contacts
- a way to join a chat room.
- a way to access old (logged) conversations.

I'm still working on back-end stuff to integrate with the address book and gloda, but it may be a good time to start start looking at the code and playing with the work-in-progress UI (warning: the appearance is quite OK on Mac, but quite rough on Windows/Linux for now).

A try server build is available at http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-c22e6f1b4df9/
Some known issues:
- there are some really bad margins on some UI elements on the Windows build. You may not see the first character you type in the chat textbox because of that.
- Sometimes the "Account Settings..." dialog loses track of *all* IM accounts. The accounts are still listed in the "Instant messaging accounts status" dialog though. I've seen this only once, but I know Jb has also suffered from this problem. We do not know the steps to reproduce so I currently can't fix it, if anybody can find steps to reproduce, that would be awesome.
- this build will identify to the Twitter server as "Instantbird".
why would you use instant messaging in your mail client? Aren't there enough programs for IM that do their job well?
Why not? 
No same program works in windows, in Mac OS, in Gnu/Linux (this one could be,is still beta: http://jitsi.org/ and it's use java). 
Send an email or a tweet, or instant message is communication why Tb must only make an email ?
Jingle is the future, Thunderbird could help it to be a strong standard.
(In reply to leitl.christoph from comment #2)
> why would you use instant messaging in your mail client? Aren't there enough
> programs for IM that do their job well?

I think your questions are answered on the feature page on the wiki already, see section #2 there.
Attached patch WIP 2 (obsolete) — — Splinter Review 
New work in progress. The most significant change is that I changed the log format to something JSON-based that's easily parsable, and I taught gloda to index these logs. There are also various small bug fixes.

I'm going to start attaching smaller patches and request feedback/review on them.
Attachment #585368 - Attachment is obsolete: true
From a quick glance at your code, the changes in mailnews/db/gloda are really minimal, which makes me happy. Your indexer also reuses almost everything from Gloda (nouns, attributes, the logic for storing arbitrary objects in the database), so that's pretty cool. Here's a few remarks.
- I didn't figure out quite clearly how your "directory-sweep" was triggered, but I think you may want to reuse the indexing job mechanism with its "dont-thrash-the-cpu" logic. If you happen to be reindexing the entire log directory because, said, the user is a nightly tester and the gloda schema was bumped (and the database was blown away), I don't think you want to almost lock up Thunderbird doing the indexing.
- At this point, I think the global-messages-db.sqlite file size is going to become critical, and there's going to be terrible disk fragmentation, cache misses, and abysmal performance from sqlite. The situation is already bad for users with a lot of emails. If we index both IM logs and emails, users who use both are going to see a pretty sharp degradation of their Gloda performance.

Andrew had plans for a "splintered" gloda, meaning some stuff is split across multiple database files. Andrew, can you tell us more about these plans and possibly give a relevant bug#? I believe this is of utmost (!) importance to keep things reactive and responsive.

This looks pretty exciting!
Also splintering would allow you to bump your IM DB schemas, and trigger a reindexing of IM logs only. Right now we have a gloda-wide version number. If you bump that number, everything is re-indexed. One could imagine a database version number for gloda-classic, and another separate one for gloda-im.
(In reply to Jonathan Protzenko [:protz] from comment #6)

> - I didn't figure out quite clearly how your "directory-sweep" was
> triggered, but I think you may want to reuse the indexing job mechanism with
> its "dont-thrash-the-cpu" logic.

It's entirely driven by indexer.js so the whole logic should be the same as for emails.
First initialSweep is called, that queues a logsFolderSweep indexer job.
The logsFolderSweep worker will traverse all the directories in <profile dir>/logs and queue a convFolderSweep job for each folder containing conversation log that it finds.
The convFolderSweep worker will index all the log files of the given directory. It will yield after each file, so that should never lock the UI for more than a few ms.


> If you happen to be reindexing the entire
> log directory because, said, the user is a nightly tester and the gloda
> schema was bumped (and the database was blown away),

I still need to figure out how to do that. Currently I keep track of already indexed files with a JSON cache file, but if the gloda database is blown away, I need to delete that cache file too, otherwise nothing's going to be re-indexed.
Is there some kind of notification when the gloda database starts from scratch for some reason?

> - At this point, I think the global-messages-db.sqlite file size is going to
> become critical, and there's going to be terrible disk fragmentation

To dramatically reduce disk fragmentation, we could call setGrowthIncrement (http://mxr.mozilla.org/comm-central/source/mozilla/storage/public/mozIStorageConnection.idl#378) with a large value on the database connection, like Places does:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/Database.cpp#640

(A quick mxr search seemed to indicate Gloda currently doesn't use setGrowthIncrement at all, of course if it already does, this comment is irrelevant :)).

> cache misses, and abysmal performance from sqlite.

I think Places has some other tricks to improve performance of the SQLite connection. I don't remember them very well.

That being said, splitting the large database into several smaller files does sound like a good idea for the various reasons you explained :). I'm just not sure it needs to be done immediately.
This all is very exciting!  I have recently switched to using Instantbird for my IM/IRC uses and like it very much.

My preliminary efforts to re-do the gloda data store are in the "glodang" repo here:
http://hg.mozilla.org/users/bugmail_asutherland.org/glodang/file/498707aa91b5/lib/glodang

It mainly consists of speculative comments where I was still thinking things through.  Today I would be much more inclined to try and do as much as possible using LevelDB or IndexedDB backed by LevelDB with a single database, although some degree of sharding might still be in order for that.  (Because of the lexicographic ordering for storage, the need for sharding is greatly reduced.)

Although I agree gloda has some scale-up problems, I don't think it makes sense to require a massive gloda overhaul before allowing instant message integration.  I would be concerned about the logs for IRC channels funneling into the same database because they can be voluminous, but I expect a user's own 1:1 conversations should be of a manageable size, especially with protz's recent page size increases for the gloda database.

I only had a few minutes to skim the patch and did not make it all the way through.  It looks like there is synchronous database access off the main thread for part of the existing Instantbird codebase.  Unless the database is a pure in-memory database, that logic is going to need to be converted to using mozStorage's async database support.  I will try and take a look over the next few days.  (I am in the process of changing countries :)
Attached patch chat/ part (obsolete) — — Splinter Review 
This large part is Instantbird's IM core, with only 2 minor differences:
- Using the preference "mail.spellcheck.inline" for Thunderbird instead of "layout.spellcheckDefault" (that's ifdef'd). 
- A PRBool replaced to bool in an idl file.
Both of these changes can go in the instantbird repository eventually.

There may still be some changes in this part of the patch (especially in the XMPP protocol plugin, as there's a dozen or so not-yet-implemented features that I would like to have so that JS-XMPP isn't a regression compared to the libpurple xmpp plugin Instantbird used) so I'm not requesting review yet; but it's mostly stable so I think it's a good time for you to start getting familiar with this code and asking me questions about it.
Attachment #587848 - Flags: feedback?(dbienvenu)
(In reply to Andrew Sutherland (:asuth) from comment #9)
> It looks like there is synchronous database access off the main
> thread for part of the existing Instantbird codebase.  Unless the database
> is a pure in-memory database, that logic is going to need to be converted to
> using mozStorage's async database support.

The database used by the existing Instantbird code is used to store the contact list.
It's read only once when the IM core is initialized, then all the information is available in memory.
Changes are written from the main thread and don't use the async API. That caused performance issues when downloading the contact list of an account from the server and storing each contact in the database. I tried using the async API to resolve that, but it was difficult because the current code expects to be able to get lastInsertRowID immediately.
I've found an easier way to workaround the performance issue: all the database accesses are now automatically wrapped in a transaction that's commited automatically at the end of the even loop spin. This ensures that we write the database to disk at most once per even loop spin, and nobody has ever complained about buddy list storage performance since I added that.
Fix mailnews/base/prefs/content/AccountManager.js:
- handle accounts without any subpanel
- call onPreInit before getPageFormElements in the restorePage function so that the form elements can be dynamically generated by the panel's code when its onPreInit function is called.

This patch can land before the rest of the changes here, it only improves slightly the extensibility of the Account Settings dialog.
Attachment #587855 - Flags: review?(dbienvenu)
At some point my nsIMsgIncomingServer implementation lacked the msgStore getter and that made nsMsgDBFolder::HandleAutoCompactEvent crash because JS objects return undefined when attempting to get an undefined property, which is converted to null and NS_OK.
While there was obviously a problem in my JS component, I don't think it's a good behavior to crash because of a missing getter in an implementation of a scriptable interface, so here is a patch.
Attachment #587856 - Flags: review?(dbienvenu)
Without this change, it's impossible to define a gloda noun associated with a table defined using the schema property but that doesn't have an "ext_" prefix.

If the IM features are built-in, I don't think we will want the "ext_" prefix.
Attachment #587862 - Flags: review?(jonathan.protzenko)
I think this part is mostly stable now and is ready for getting some feedback, at least to see if your hackometer explodes :).

The code used to create an account from the account wizard is:

    var accountManager = Cc["@mozilla.org/messenger/account-manager;1"]
                           .getService(Ci.nsIMsgAccountManager);
    
    var inServer =
      accountManager.createIncomingServer(this.username,
                                          this.proto.id, // hostname
                                          "im");
    inServer.wrappedJSObject.imAccount = acc;

    var account = accountManager.createAccount();
    account.incomingServer = inServer;
    accountManager.notifyServerLoaded(inServer);

where acc is a variable containing an instance of imIAccount.

The two interfaces implemented here (nsIMsgProtocolInfo and nsIMsgIncomingServer) have lots of methods and attributes. I took the approach of implementing only those that are actually being called. This is ugly and may be a bit fragile, but not necessarily more than implementing lots methods I don't understand and so where I wouldn't be sure that I'm always returning correctly the value that would cause the least pointless/dangerous executions if somehow they ever got called. At least with methods not implemented at all, there's a clear warning in the terminal (with debug builds) when something is called unexpectedly.

Having to implement the rootFolder getter was a bit unfortunate, but lots of different parts of the existing code expect that it will always be there. The account manager even uses account.rootFolder.prettyName as the name to display in the UI!
Attachment #587869 - Flags: feedback?(dbienvenu)
(In reply to Florian Quèze from comment #13)
> Created attachment 587856 [details] [diff] [review]
> Fix crash @nsMsgDBFolder::HandleAutoCompactEvent
> 
> At some point my nsIMsgIncomingServer implementation lacked the msgStore
> getter and that made nsMsgDBFolder::HandleAutoCompactEvent crash because JS
> objects return undefined when attempting to get an undefined property, which
> is converted to null and NS_OK.

That's surprising, and bad. We rely on the xpcom rule that a method shouldn't return null and NS_OK throughout the code.
Attachment #587856 - Flags: review?(dbienvenu) → review+
(In reply to David :Bienvenu from comment #16)

> That's surprising, and bad. We rely on the xpcom rule that a method
> shouldn't return null and NS_OK throughout the code.

The rule is respected for methods, but this is an attribute getter.
Comment on attachment 587855 [details] [diff] [review]
AccountManager.js bugfix

looks OK - my one concern is whether this might break any existing extensions that add their own stuff to account settings. It doesn't seem like it should, but extensions tend to be fragile. If it seems likely to break extensions, a post to m.d.a.t. would be worthwhile, but if it seems highly unlikely, then don't bother.
Attachment #587855 - Flags: review?(dbienvenu) → review+
(In reply to Florian Quèze from comment #11)
> (In reply to Andrew Sutherland (:asuth) from comment #9)
> > It looks like there is synchronous database access off the main
> > thread for part of the existing Instantbird codebase.  Unless the database
> > is a pure in-memory database, that logic is going to need to be converted to
> > using mozStorage's async database support.
> 
> The database used by the existing Instantbird code is used to store the
> contact list.
> It's read only once when the IM core is initialized, then all the
> information is available in memory.
There's a difference between information being available in memory, and creating a pure in-memory database using ":memory:" as the table name (see http://www.sqlite.org/inmemorydb.html). Can you confirm that this is what you're doing? (Sorry, I don't have time to read the patch to make sure :)). We've been fighting in the Firefox area to remove any synchronous access, it would be shame to degrade Thunderbird performance when the entire toolkit has made sure they're not using any synchronous connections. (I won't refer your to the numerous MDN pages with big banners with red <h1>s, I guess you know about them :)).

> Changes are written from the main thread and don't use the async API. That
> caused performance issues when downloading the contact list of an account
> from the server and storing each contact in the database.
Can you elaborate on that?

> has ever complained about buddy list storage performance since I added that.
Of course, but Thunderbird demands much more from mozStorage, and there's already gloda issuing a lot of requests. We should make sure no one is intentionally making aggressive queries and doing a lot of IO, otherwise, with all other sqlite clients running on the same thread, this is likely to yield sizable performance issues :).

I think we should sort out the synchronous sqlite concern before landing this.
(In reply to Florian Quèze from comment #11)
> I tried using the
> async API to resolve that, but it was difficult because the current code
> expects to be able to get lastInsertRowID immediately.

gloda deals with this by determining the current maximum value at startup and then updating the value itself as it issues writes.  This should be pretty easy to do.

> I've found an easier way to workaround the performance issue: all the
> database accesses are now automatically wrapped in a transaction that's
> commited automatically at the end of the even loop spin. This ensures that
> we write the database to disk at most once per even loop spin, and nobody
> has ever complained about buddy list storage performance since I added that.

Yes, making sure to wrap things in a transaction so you only use 1 transaction for N statements rather than N transactions for N statements is definitely a good thing for performance.  If you are not going to consolidate your calls to executeAsync to occur in a single batch (which is to say, 1 call to executeAsync on the connection rather than the statements), you may want to 'manually' manage the async transactions by issuing a 'BEGIN' statement and a 'COMMIT' statement yourself.  This is how gloda does things (because it was written before the batching support existed, primarily).  This works because mozStorage only tries to wrap batches in transactions (and if it is automatically wrapping things in a transaction, it does not freak out if its attempt to open a transaction fails)).

In any event, I'm with protz that Thunderbird does not need any additional main-thread I/O.  If your database can survive with totally disabling fsyncs (and thereby increasing chances of corruption), it might be manageable, but otherwise it's a bad idea.
Florian, when you get a chance, a try server build with a more recent mailnews backend would be helpful - something from the last couple days would be good.
Attached patch WIP 3 (obsolete) — — Splinter Review 
Newer version of the patch. It's now possible to search the indexed messages.

(In reply to David :Bienvenu from comment #21)
> Florian, when you get a chance, a try server build with a more recent
> mailnews backend would be helpful - something from the last couple days
> would be good.

Here is a new try server build with this new patch + your patch from bug 715922:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-fad805960e3e/
Attachment #587812 - Attachment is obsolete: true
Comment on attachment 587869 [details] [diff] [review]
Integration with nsIMsgAccountManager

+    if (!id)
+      return null;
+    Services.core.init();
+    return (this._imAccount = Services.accounts.getAccountById(id));

Are IM services called Services? Won't that conflict with Mozilla Services, which we use quite a bit in the code. We use MailServices for our own services.

right, realHostName is not used in the folderPane UI - it's used in the back end to connect to the actual server. This allows the user to change the actual server w/o invalidating URI's.

constructedPrettyName is only used if server.name is not set.

I'm a little worried about fallout from including IM accounts in accountMgr.accounts; it might lead to further dummy stubs, but if it makes integration easier, it seems like a reasonable approach.
Attachment #587869 - Flags: feedback?(dbienvenu) → feedback+
Tools menu, instant messaging account status was the only way I could find to create a new IM account. I was able to create a twitter account, but no IRC account, so my testing of actual IM'ing is limited...

I can't find the options UI to control how the messages are displayed (I'm not a fan of the bubbles).
(In reply to David :Bienvenu from comment #23)
> Comment on attachment 587869 [details] [diff] [review]
> Integration with nsIMsgAccountManager
> 
> +    if (!id)
> +      return null;
> +    Services.core.init();
> +    return (this._imAccount = Services.accounts.getAccountById(id));
> 
> Are IM services called Services? Won't that conflict with Mozilla Services,
> which we use quite a bit in the code. We use MailServices for our own
> services.

imServices.jsm (http://lxr.instantbird.org/instantbird/source/chat/modules/imServices.jsm) imports the toolkit services.jsm module and reexports the Services symbol after adding a couple more services to it, this way we get both the IM and toolkit services with a single import.

This also worries me but not for the same reason: I'm afraid Services.accounts could be misleading inside Thunderbird (as a Thunderbird add-on author, I would expect that to be an nsIMsgAccountManager instance.

In parts of the code I'm adding here that are sharing the same scope as existing Thunderbird UI code, I've used this pattern:

var imServices = {};
Components.utils.import("resource:///modules/imServices.jsm", imServices);
imServices = imServices.Services;

I'm not sure if it's really good, but at least it's not confusing.
(In reply to David :Bienvenu from comment #24)
> Tools menu, instant messaging account status was the only way I could find
> to create a new IM account.

There's also Tools -> Account Settings -> Account Actions -> Add Instant Messaging Account...

> I was able to create a twitter account, but no
> IRC account, so my testing of actual IM'ing is limited...

IRC isn't supported yet, as Instantbird uses GPL'ed code for it. A JS implementation is in the work though (https://bugzilla.instantbird.org/show_bug.cgi?id=507), at this point it's mostly waiting for me to take time to review it.

Even if we had an IRC plugin, the prototype here doesn't have any UI to join a chat room so actually using IRC would be very difficult.

> I can't find the options UI to control how the messages are displayed (I'm
> not a fan of the bubbles).

If I understood correctly, we won't expose any UI to change message themes but the Bubbles theme won't remain the default if Andreas can provide a more Thunderbird-looking alternative :).
(In reply to Florian Quèze from comment #26)
> 
> There's also Tools -> Account Settings -> Account Actions -> Add Instant
> Messaging Account...

Something under file | new menu would be helpful, I think.
> 
> IRC isn't supported yet, as Instantbird uses GPL'ed code for it. A JS
> implementation is in the work though
> (https://bugzilla.instantbird.org/show_bug.cgi?id=507), at this point it's
> mostly waiting for me to take time to review it.
> 
> Even if we had an IRC plugin, the prototype here doesn't have any UI to join
> a chat room so actually using IRC would be very difficult.

Yeah, I guess this is more important for getting testing from Mozilla developers and almost completely unimportant for our user base.
Comment on attachment 587862 [details] [diff] [review]
Let Gloda noun definitions specify a tableName

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

Sorry I *completely* missed out on the part where you asked review, I don't even have bugmail for that. Was there a Bugzilla outage recently?
Attachment #587862 - Flags: review?(jonathan.protzenko) → review+
Comment on attachment 587848 [details] [diff] [review]
chat/ part

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

Overall, the code is extremely clean, and nice. Great job!

Where I've made style comments, I usually just comment on the first instance of the issue, but there are almost always multiple instances of the issue.

::: chat/chat-prefs.js
@@ +6,5 @@
> +
> +pref("messenger.accounts", "");
> +
> +// The intervals in seconds between automatic reconnection attempts
> +// The last value will be reused forever.

perhaps, "The last value will be used for the rest of the reconnection attempts."

::: chat/components/public/imIAccount.idl
@@ +107,5 @@
> +  /* Request more info on a buddy (typically a chat buddy).
> +   * The result (if any) will be provided by a user-info-received
> +   * notification dispatched through the observer service:
> +   *  - aSubject will be an nsISimpleEnumerator of prplITooltipInfo,
> +   *  Do NOT store aSubject. Use it right away. The memory it points to

this is a bit scary - is it because of lib purple?

@@ +210,5 @@
> +
> +  /* Delete the account (from the preferences, mozStorage, and call unInit). */
> +  void remove();
> +
> +  /* Cancel the timer that automatically reconnects the account that were

"the account that was" or "the accounts that were"

@@ +240,5 @@
> +  const short FIRST_CONNECTION_CRASHED = 4;
> +
> +  attribute short firstConnectionState;
> +
> +  // FIXME password should be in password manager

Yes, it should, before this lands, I think.

::: chat/components/public/imIAccountsService.idl
@@ +64,5 @@
> +       - after a loss of internet connectivity that disconnected all accounts.
> +  */
> +  void processAutoLogin();
> +
> +  imIAccount getAccountById(in AUTF8String aAccountId);

we prefer doxygen comment style for all methods, including documentation on each parameter and the return value, e.g.,

/**
 * Lookup the account by Id.
 *
 * @param aAccountId - Id of account we're looking for.
 * 
 * @returns Account for Id
 */

It's tedious, but it helps with documentation.

::: chat/components/public/imICommandsService.idl
@@ +62,5 @@
> +  //    0 is the default priority.
> +  //  < 0 is lower priority.
> +  //  > 0 is higher priority.
> +  // Commands registered by protocol plugins will usually use PRIORITY_PRPL.
> +  readonly attribute PRInt32 priority;

use signed int, if possible

@@ +104,5 @@
> +                         [optional] in prplIConversation aConversation);
> +};
> +
> +%{ C++
> +#define IM_COMMANDS_SERVICE_CONTRACTID \

it's odd to have instantbird contract ids in TB.

::: chat/components/public/imIContactsService.idl
@@ +162,5 @@
> +   * from the linked buddies (imIBuddy instances) and their account
> +   * buddies (imIAccountBuddy instances).
> +   */
> +
> +  // Exposed for add-on authors. All usage by Instantbird will come from

an other Instantbird reference.

::: chat/components/public/imIConversationsService.idl
@@ +52,5 @@
> +  attribute prplIConversation target;
> +
> +  // Number of unread messages (all messages, including system
> +  // messages are counted).
> +  readonly attribute PRUint32 unreadMessageCount;

prefer unsigned long - we're moving away from the NSPR types

::: chat/components/public/imITagsService.idl
@@ +75,5 @@
> +  // Get an existing tag by (numeric) id. Returns null if not found.
> +  imITag getTagById(in long aId);
> +  // Get an existing tag by name (will do an SQL query). Returns null
> +  // if not found.
> +  imITag getTagByName(in AUTF8String aName);

synchronously? Do we think that's OK?

::: chat/components/public/prplIMessage.idl
@@ +66,5 @@
> +  readonly attribute prplIConversation conversation;
> +
> +  /* Holds the sender color for Chats.
> +     Empty string by default, it is set by the conversation binding. */
> +           attribute AUTF8String color;

formatting/indentation is confusing here.

::: chat/components/src/imAccounts.js
@@ +92,5 @@
> +function UnknownProtocol(aPrplId)
> +{
> +  this.id = aPrplId;
> +}
> +UnknownProtocol.prototype = {

want a blank line after }

@@ +640,5 @@
> +    // For now, assume it's because of a crash.
> +    this.autoLoginStatus = Ci.imIAccountsService.AUTOLOGIN_CRASH;
> +    prefs.deleteBranch(kPrefAutologinPending);
> +
> +#ifdef MOZ_CRASHREPORTER

Why not use a dynamic way of figuring out if crash reporter is desired/present?

::: chat/components/src/imContacts.js
@@ +1251,5 @@
> +      let account = Services.accounts.getAccountByNumericId(accountId);
> +      let tag = TagsById[tagId];
> +      try {
> +        let ab = account.loadBuddy(buddy, tag);
> +        if (ab)

what does ab stand for here? A more meaningful name would be helpful.

::: chat/components/src/imCore.js
@@ +15,5 @@
> + * 2011.
> + *
> + * The Initial Developer of the Original Code is
> + * Florian QUEZE <florian@instantbird.org>.
> + * Portions created by the Initial Developer are Copyright (C) 2011

Need to decide what to do with the copyrights, and new MPL, but should use 2012 for starters

@@ +197,5 @@
> +  },
> +
> +  _getProfileDir: function()
> +    Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties)
> +      .get("ProfD" /*NS_APP_USER_PROFILE_50_DIR*/, Ci.nsIFile),

/*NS_APP...*/ isn't a useful comment, IMHO.

::: chat/components/src/logger.js
@@ +35,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#filter substitution
> +#ifdef XP_WIN

this can be done dynamically by checking if we're on windows, like this:

  var lineBreak = ("@mozilla.org/windows-registry-key;1" in Cc) ? "\r\n" : "\n";

@@ +483,5 @@
> +        log.logMessage(aSubject);
> +      }
> +      break;
> +    case "new-conversation":
> +      //XXX should we create the log file here?

I think creating it lazily on the first message is probably better. But I don't care much.

::: chat/components/src/smileProtocolHandler.js
@@ +39,5 @@
> +Components.utils.import("resource:///modules/imSmileys.jsm");
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const smileRegexp = /^smile:\/\//;

we usually use k for consts, so, kSmileRegexp

::: chat/content/browserRequest.js
@@ +168,5 @@
> +  var browser = document.getElementById("requestFrame");
> +  browser.addProgressListener(reporterListener,
> +                              Components.interfaces.nsIWebProgress.NOTIFY_ALL);
> +  var url = request.url;
> +  if (url != "") {

I prefer not to have unneeded braces in simple cases like this, which I think is consistent with the rest of the chat code I've seen.

::: chat/modules/imServices.jsm
@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +let EXPORTED_SYMBOLS = ["Services"];

having this be IMServices would be more consistent with what we do in TB

::: chat/modules/imStatusUtils.jsm
@@ +33,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +var EXPORTED_SYMBOLS = ["Status"];

maybe imStatus? or IMStatus

::: chat/modules/imThemes.jsm
@@ +321,5 @@
> +  let variants = getDirectoryEntries(aTheme.baseURI + "Variants/");
> +  let cssRe = /\.css$/;
> +  variants = variants.filter(function(v) cssRe.test(v))
> +                     .map(function(v) v.replace(cssRe, ""));
> +  return variants;

can just return the call to variants.filter, right?

@@ +676,5 @@
> +
> +/* This function is used to pretty print a selection inside a conversation area */
> +function serializeSelection(aSelection)
> +{
> +  // We have to kinds of selection serialization:

two kinds

::: chat/modules/imXPCOMUtils.jsm
@@ +48,5 @@
> +  "l10nHelper",
> +  "initLogModule"
> +];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

nice!

@@ +100,5 @@
> +}
> +
> +function setTimeout(aFunction, aDelay)
> +{
> +  var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

I prefer let to var where appropriate, though it's up to you.

::: chat/modules/jsProtoHelper.jsm
@@ +324,5 @@
> +  getTooltipInfo: function() EmptyEnumerator,
> +  createConversation: function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED; }
> +};
> +
> +// aUserName is required only if aBuddy is null (= we are adding a buddy)

(= ...) - i.e., we are adding a buddy would be more readable

::: chat/modules/socket.jsm
@@ +384,5 @@
> +  /*
> +   * nsIBadCertListener2
> +   */
> +  // Called when there's an error, return true to suppress the modal alert.
> +  notifyCertProblem: function(aSocketInfo, aStatus, aTargetSite) true,

just to be clear, because it has come up in security reviews, this means that the connection will be rejected, right? Which reminds me, has a security review been scheduled for IM in TB?

::: chat/protocols/twitter/twitter.js
@@ +280,5 @@
> +      entArray.sort(function(a, b) a.start - b.start);
> +      let offset = 0;
> +      for each (let entity in entArray) {
> +        let str = text.substring(offset + entity.start, offset + entity.end);
> +        if (str[0] == "ï¼ ")

maybe specifying this in octal with a comment would be nicer (up to you...it just makes me nervous) - or at least a comment as to what these magic characters mean.

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +154,5 @@
> +      // nsSAXXMLReader (inside XMPPParser) leaks if we don't clean up.
> +      // Unfortunately, calling onStopRequest on nsSAXXMLReader damages
> +      // something that causes a crash the next time we call onDataAvailable
> +      // on another parser instance for the same input stream buffer.
> +      // Workaround: keep references to all previous parsers used

yikes, is that a necko bug?

@@ +322,5 @@
> +                     _("connection.error." + errorMsg));
> +        return;
> +      }
> +
> +      let rv;

we tend to reserve rv for nsresult rv, i.e., return value. So a different var name will make the code more readable.

::: chat/protocols/xmpp/xmpp.jsm
@@ +78,5 @@
> + */
> +const kRoles = ["outcast", "visitor", "participant", "member", "moderator",
> +                "admin", "owner"];
> +
> +function MUCParticipant(aNick, aName, aStanza)

what's a MUC?
Comment on attachment 587848 [details] [diff] [review]
chat/ part

I didn't look at the UI parts of this patch, especially the strings and style/theme stuff.
Attachment #587848 - Flags: feedback?(dbienvenu) → feedback+
This small patch now gives the chat toolbar button an icon on Linux and Windows.
It seems like running this patch makes everything to tall to fit the main tb window, so I've been having some issues testing the chat.

Some quick things:
* Contacts show the protocol and the status, I don't think the protocol is that important. Most clients tends to hide this info until you click a contact.
* I agree with Bienvenu that new chat account would be good under file > new, I though I had done something wrong until I found it in the accounts dialog.
* Opening a conversation don't move it from Contacts to Conversation, making the one in Contacts unclickable.
* Might be good to be able to click the whole name for chat in contact list instead of the small chat bubble that appears on hover.
(In reply to Florian Quèze from comment #22)

> Here is a new try server build with this new patch + your patch from bug
> 715922:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.
> net-fad805960e3e/

Is there any reason why there is no Mac try build?
It might be a little bit late for this, but I was thinking: would it be wrong to implement this as an add-on that ships with Thunderbird (like Test Pilot)? That seems like it would be a good way of, say, letting enterprises disable IM support in Thunderbird, or for the people who love to complain about "bloat".

In the future, I could see Lightning being distributed as a default add-on as well.
(In reply to Neustradamus from comment #35)
> I found a bug, there is only "Facebook Chat", "Google Talk" or "Twitter".

An own XMPP implementation is used for Facebook Chat and Google Talk already. Plain XMPP will be available one day too.

> There is not "Jabber ID", an XMPP account: http://xmpp.org/.
> - http://xmpp.org/resources/public-services/
> - http://list.jabber.at/
> - http://www.jabberes.org/servers/
> -
> http://jabberworld.info/
> %D0%A1%D0%BF%D0%B8%D1%81%D0%BE%D0%BA_%D1%80%D0%B0%D0%B1%D0%BE%D1%82%D0%B0%D1%
> 8E%D1%89%D0%B8%D1%85_%D0%BF%D1%83%D0%B1%D0%BB%D0%B8%D1%87%D0%BD%D1%8B%D1%85_%
> D1%81%D0%B5%D1%80%D0%B2%D0%B5%D1%80%D0%BE%D0%B2_Jabber

It absolutely makes no sense to post random lists of XMPP servers here.
(In reply to Neustradamus from comment #35)

> There is not "Jabber ID", an XMPP account

Generic XMPP support isn't advertised in the new account wizard because we don't have support for DNS SRV queries yet (see bug 14328) and it's my understanding that lots of (if not most) generic XMPP services rely on DNS SRV.
Comment on attachment 587856 [details] [diff] [review]
Fix crash @nsMsgDBFolder::HandleAutoCompactEvent

Pushed as http://hg.mozilla.org/comm-central/rev/bed472157fd2
Comment on attachment 587862 [details] [diff] [review]
Let Gloda noun definitions specify a tableName

Pushed as http://hg.mozilla.org/comm-central/rev/d653fdc2972c
(In reply to David :Bienvenu from comment #29)
> Comment on attachment 587848 [details] [diff] [review]

> ::: chat/components/public/imIAccount.idl
> @@ +107,5 @@
> > +  /* Request more info on a buddy (typically a chat buddy).
> > +   * The result (if any) will be provided by a user-info-received
> > +   * notification dispatched through the observer service:
> > +   *  - aSubject will be an nsISimpleEnumerator of prplITooltipInfo,
> > +   *  Do NOT store aSubject. Use it right away. The memory it points to
> 
> this is a bit scary - is it because of lib purple?

Yes, this doesn't apply to the protocol plugins implemented in JavaScript of course :).
We could probably fix this by copying everything before firing the notification if this becomes an issue (I don't see any use case for storing this enumerator in the UI rather than using it immediately).

> @@ +240,5 @@
> > +  const short FIRST_CONNECTION_CRASHED = 4;
> > +
> > +  attribute short firstConnectionState;
> > +
> > +  // FIXME password should be in password manager
> 
> Yes, it should, before this lands, I think.

I've worked on this, it will land tonight in Instantbird's repository and will be in the next iteration of my patch here. :)


> ::: chat/components/public/imIAccountsService.idl
> @@ +64,5 @@
> > +       - after a loss of internet connectivity that disconnected all accounts.
> > +  */
> > +  void processAutoLogin();
> > +
> > +  imIAccount getAccountById(in AUTF8String aAccountId);
> 
> we prefer doxygen comment style for all methods, including documentation on
> each parameter and the return value, e.g.,
> 
> /**
>  * Lookup the account by Id.
>  *
>  * @param aAccountId - Id of account we're looking for.
>  * 
>  * @returns Account for Id
>  */
> 
> It's tedious, but it helps with documentation.

It's possible your example was shortened because it's well, just an example, but I hate these comments that don't document anything and only duplicate the information that was already obvious from the method prototype with the method name, parameter names and types and return value type. They are a waste of everybody's time because when they are there, one feels completed to read them before deciding they aren't helpful and it's better to just go look at the implementation.

Valuable doxygen style comments need to document the method enough that I won't want to look at its implementation. Things that I'll typically need to know:
 - how expensive a call to that API will be (should I call it only once, or can I call it in a loop?)
 - is it returning a copy of the object that I can freely modify or exposing some internal data structure that I shouldn't mess with?
 - how the error handling is done (if it's done at all)
...

And yes, that's very tedious, as you said :).
I'm willing to take the time to document correctly some of the key APIs that could be of interest to add-on developers, but I don't want to add pointless comments.


> @@ +104,5 @@
> > +                         [optional] in prplIConversation aConversation);
> > +};
> > +
> > +%{ C++
> > +#define IM_COMMANDS_SERVICE_CONTRACTID \
> > +  "@instantbird.org/purple/commands-service;1"
>
> it's odd to have instantbird contract ids in TB.

What would you like to see instead?
Should I do a mass replace @instantbird.org/purple/ -> @mozilla.org/chat/ ?

> ::: chat/components/public/imITagsService.idl
> @@ +75,5 @@
> > +  // Get an existing tag by (numeric) id. Returns null if not found.
> > +  imITag getTagById(in long aId);
> > +  // Get an existing tag by name (will do an SQL query). Returns null
> > +  // if not found.
> > +  imITag getTagByName(in AUTF8String aName);
> 
> synchronously? Do we think that's OK?

I will check how this can be changed. The only current use case seems to be internal to the tags service (inside the createTag method).

> 
> ::: chat/components/public/prplIMessage.idl
> @@ +66,5 @@
> > +  readonly attribute prplIConversation conversation;
> > +
> > +  /* Holds the sender color for Chats.
> > +     Empty string by default, it is set by the conversation binding. */
> > +           attribute AUTF8String color;
> 
> formatting/indentation is confusing here.

It's aligned with the other attribute keywords above, keeping the space for the "readonly" keyword. I agree it doesn't look good with a comment above it though :).


> ::: chat/components/src/imAccounts.js
> @@ +92,5 @@
> > +function UnknownProtocol(aPrplId)
> > +{
> > +  this.id = aPrplId;
> > +}
> > +UnknownProtocol.prototype = {
> 
> want a blank line after }

I think I've done this throughout the code: A blank line after the end of a function, but not between a constructor and its prototype. From my point of view it improves readability as this clearly shows that they work together and shouldn't be separated.

I would like to keep this pattern for the files in chat/. I don't mind doing like you said for code that will go in mail/ and mailnews/ as I also value consistency :).
 
> @@ +640,5 @@
> > +    // For now, assume it's because of a crash.
> > +    this.autoLoginStatus = Ci.imIAccountsService.AUTOLOGIN_CRASH;
> > +    prefs.deleteBranch(kPrefAutologinPending);
> > +
> > +#ifdef MOZ_CRASHREPORTER
> 
> Why not use a dynamic way of figuring out if crash reporter is
> desired/present?

Originally this was an ifdef in C++, where ifdef are quite common and less painful than in JS code (and maybe required to get the code to compile, if some XPCOM interface headers weren't available when this wasn't defined). I haven't reconsidered this when I rewrote this file in JS. I will :).


> ::: chat/components/src/imContacts.js
> @@ +1251,5 @@
> > +      let account = Services.accounts.getAccountByNumericId(accountId);
> > +      let tag = TagsById[tagId];
> > +      try {
> > +        let ab = account.loadBuddy(buddy, tag);
> > +        if (ab)
> 
> what does ab stand for here? A more meaningful name would be helpful.

ab = accountBuddy (an object implementing the prplIAccountBuddy xpcom interface)

> 
> ::: chat/components/src/imCore.js
> @@ +15,5 @@
> > + * 2011.
> > + *
> > + * The Initial Developer of the Original Code is
> > + * Florian QUEZE <florian@instantbird.org>.
> > + * Portions created by the Initial Developer are Copyright (C) 2011
> 
> Need to decide what to do with the copyrights, and new MPL, but should use
> 2012 for starters

I think this file was released as part of Instantbird (but maybe only in nightly builds) in 2011, so the copyright year sounds right.

I'm not sure of which email address to use for the license of files that I created for my Thunderbird work, so switching to MPL2 would clearly simplify things :).

> @@ +197,5 @@
> > +  },
> > +
> > +  _getProfileDir: function()
> > +    Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties)
> > +      .get("ProfD" /*NS_APP_USER_PROFILE_50_DIR*/, Ci.nsIFile),
> 
> /*NS_APP...*/ isn't a useful comment, IMHO.

I put it there to remember how to find with similar calls implemented in C++ with mxr.
That would probably be clearer if written as:
  const NS_APP_USER_PROFILE_50_DIR = "ProfD";


> @@ +483,5 @@
> > +        log.logMessage(aSubject);
> > +      }
> > +      break;
> > +    case "new-conversation":
> > +      //XXX should we create the log file here?
> 
> I think creating it lazily on the first message is probably better.

It's the current behavior :).


> ::: chat/modules/imServices.jsm
> @@ +34,5 @@
> > + * the terms of any one of the MPL, the GPL or the LGPL.
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +let EXPORTED_SYMBOLS = ["Services"];
> 
> having this be IMServices would be more consistent with what we do in TB

It's an extension of the toolkit Services object so that the code only has to import a single module to access both toolkit and IM services.

I'm not completely sure of what to do with this in the context of Thunderbird (mostly because Services.accounts to access the imIAccountsServices is confusing in this context), but renaming Services to IMServices would likely break compatibility with all current Instantbird add-ons, so I would rather avoid that.


> @@ +100,5 @@
> > +}
> > +
> > +function setTimeout(aFunction, aDelay)
> > +{
> > +  var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> I prefer let to var where appropriate, though it's up to you.

When doing Instantbird reviews, I require let for new files, and recommend it for new functions in existing files. When modifying an existing function already using var, I recommend using var too in new lines for consistency.
That said, I don't mind replacing lots of var with let in the existing code if that pleases you :). It's been done yet because it doesn't feel like a very productive use of time....



> ::: chat/modules/socket.jsm
> @@ +384,5 @@
> > +  /*
> > +   * nsIBadCertListener2
> > +   */
> > +  // Called when there's an error, return true to suppress the modal alert.
> > +  notifyCertProblem: function(aSocketInfo, aStatus, aTargetSite) true,
> 
> just to be clear, because it has come up in security reviews, this means
> that the connection will be rejected, right?

It only suppresses the modal dialog.

By the way, I haven't been able to find any way to prevent the connection from being rejected if NSS has already examined the certificate and is unhappy with it; nor to give NSS a different hostname than what was used to create the socket. This will be problematic when we will support DNS SRV queries, as we will connect to the hostname indicated in the SRV records but receive a certificate for the domain in the user's XMPP id (if the server is configured correctly).

> Which reminds me, has a
> security review been scheduled for IM in TB?

Not yet. I wanted to wait for you to become a bit familiar with this code before scheduling it; and fix the password storage :).
 

> ::: chat/protocols/xmpp/xmpp-session.jsm
> @@ +154,5 @@
> > +      // nsSAXXMLReader (inside XMPPParser) leaks if we don't clean up.
> > +      // Unfortunately, calling onStopRequest on nsSAXXMLReader damages
> > +      // something that causes a crash the next time we call onDataAvailable
> > +      // on another parser instance for the same input stream buffer.
> > +      // Workaround: keep references to all previous parsers used
> 
> yikes, is that a necko bug?

Seems so, but I haven't been able to produce a reduced testcase that I would need to file a bug that would be likely to get fixed.

> ::: chat/protocols/xmpp/xmpp.jsm
> @@ +78,5 @@
> > + */
> > +const kRoles = ["outcast", "visitor", "participant", "member", "moderator",
> > +                "admin", "owner"];
> > +
> > +function MUCParticipant(aNick, aName, aStanza)
> 
> what's a MUC?

Multi-User Chat
It's standard XMPP terminology.

I agree with all the review comments that I haven't mentioned in this reply. Thanks for the useful feedback!
(In reply to Florian Quèze from comment #40)
> > @@ +64,5 @@
> > > +       - after a loss of internet connectivity that disconnected all accounts.
> > > +  */
> > > +  void processAutoLogin();
> > > +
> > > +  imIAccount getAccountById(in AUTF8String aAccountId);
> > 
> > we prefer doxygen comment style for all methods, including documentation on
> > each parameter and the return value, e.g.,
> > 
> > /**
> >  * Lookup the account by Id.
> >  *
> >  * @param aAccountId - Id of account we're looking for.
> >  * 
> >  * @returns Account for Id
> >  */
> > 
> > It's tedious, but it helps with documentation.
> 
> It's possible your example was shortened because it's well, just an example,
> but I hate these comments that don't document anything and only duplicate
> the information that was already obvious from the method prototype with the
> method name, parameter names and types and return value type. They are a
> waste of everybody's time because when they are there, one feels completed
> to read them before deciding they aren't helpful and it's better to just go
> look at the implementation.
> 
> Valuable doxygen style comments need to document the method enough that I
> won't want to look at its implementation. Things that I'll typically need to
> know:
>  - how expensive a call to that API will be (should I call it only once, or
> can I call it in a loop?)
>  - is it returning a copy of the object that I can freely modify or exposing
> some internal data structure that I shouldn't mess with?
>  - how the error handling is done (if it's done at all)
> ...
> 
> And yes, that's very tedious, as you said :).

I understand that some methods are so simple that extra comments aren't helpful. But, in the idl, you have non doxygen style comments:

+  // observe should only be called by the imIAccount
+  // implementation to report user status changes that affect this account.

and comments like:

/* foobar
 * asdfafdsad
 */

which aren't quite doxygen either.

> >
> > it's odd to have instantbird contract ids in TB.
> 
> What would you like to see instead?
> Should I do a mass replace @instantbird.org/purple/ -> @mozilla.org/chat/ ?

Yes, that sounds good to me.
> I think I've done this throughout the code: A blank line after the end of a
> function, but not between a constructor and its prototype. From my point of
> view it improves readability as this clearly shows that they work together
> and shouldn't be separated.

OK.
> > 
> > what does ab stand for here? A more meaningful name would be helpful.
> 
> ab = accountBuddy (an object implementing the prplIAccountBuddy xpcom
> interface)

ok,thx, I'd really like that written out so that the code is more readable.
> 
> I put it there to remember how to find with similar calls implemented in C++
> with mxr.
> That would probably be clearer if written as:
>   const NS_APP_USER_PROFILE_50_DIR = "ProfD";

yes, I suppose - though we like to put k in front of our consts
> 
> 
> > @@ +483,5 @@
> > > +        log.logMessage(aSubject);
> > > +      }
> > > +      break;
> > > +    case "new-conversation":
> > > +      //XXX should we create the log file here?
> > 
> > I think creating it lazily on the first message is probably better.
> 
> It's the current behavior :).
Yes, I realized that; I was more saying that the comment should go away.
> 
> I'm not completely sure of what to do with this in the context of
> Thunderbird (mostly because Services.accounts to access the
> imIAccountsServices is confusing in this context), but renaming Services to
> IMServices would likely break compatibility with all current Instantbird
> add-ons, so I would rather avoid that.

Hmm, that's too bad.
> 
> 
> When doing Instantbird reviews, I require let for new files, and recommend
> it for new functions in existing files. When modifying an existing function
> already using var, I recommend using var too in new lines for consistency.
> That said, I don't mind replacing lots of var with let in the existing code
> if that pleases you :). It's been done yet because it doesn't feel like a
> very productive use of time....
My rule of thumb is that new code uses let; fixing old code to use let is just a bonus. I guess this code is somewhere in between old and new. The less code there is for people to copy that uses var, the better. I'm definitely not insisting, just letting you know my preference.

> 
> It only suppresses the modal dialog.

We want the connection to be rejected, because we don't want to allow connections with bad certs w/o telling the user. This will probably come up in the security review.
> 
> > what's a MUC?
> 
> Multi-User Chat
> It's standard XMPP terminology.

OK, cool. I buy that, but not "ab" :-)
(In reply to Florian Quèze from comment #40)
> > > +let EXPORTED_SYMBOLS = ["Services"];

Why not const instaed of let?

> > > +
> > > +function MUCParticipant(aNick, aName, aStanza)
> > 
> > what's a MUC?
> 
> Multi-User Chat
> It's standard XMPP terminology.

For someone new to the code, this is where doxygen comments would have helped ;)
(In reply to Magnus Melin from comment #42)
> (In reply to Florian Quèze from comment #40)
> > > > +let EXPORTED_SYMBOLS = ["Services"];
> 
> Why not const instaed of let?

Usage of const/var/let or just nothing before EXPORTED_SYMBOLS isn't consistent throughout mozilla-central and comm-central (http://mxr.mozilla.org/comm-central/search?string=EXPORTED_SYMBOLS), so there was no particular reason for let instead of const here; anyway I agree that const seems to make more sense, so my next patch will use only const in that case.

> > > > +
> > > > +function MUCParticipant(aNick, aName, aStanza)
> > > 
> > > what's a MUC?
> > 
> > Multi-User Chat
> > It's standard XMPP terminology.
> 
> For someone new to the code, this is where doxygen comments would have
> helped ;)

This function is an internal detail of a file, not an exposed/exported API in any way, so it's probably not the best example of the usefulness of doxygen comments.
Attached patch WIP 4 (obsolete) — — Splinter Review 
Most easy review comments have been addressed + this now stores passwords in the password manager.
Attachment #589154 - Attachment is obsolete: true
Keywords: sec-review-needed
Whiteboard: [secr:ptheriault]
+kw: sec-review-needed
wb: [secr:ptheriault]

We need to do a code level review of the sanitization code and possibly other areas. This task will be done by ptheriault.
Results of security review are in 727216, since they may impact InstantBird.

I'm new here so just let me know if I'm doing it wrong ;)
Attached patch WIP 4 (unbitrotted) (obsolete) — — Splinter Review 
The WIP4 patch had bitrotted slightly and I've hit this on two different systems now, so I thought it was better to upload the patch here instead of unbitrotting it by hand every time.
Depends on: 729069
Attached patch Aero style fixes (obsolete) — — Splinter Review 
(this patch also includes the Linux and Windows toolbar icons)
Some initial style fixes for Chat under Aero. Still WIP.
Attachment #589462 - Attachment is obsolete: true
Is there a nightly release channel yet or is this still too early (besides downloading the code manually)?  

Thanks!
Attached patch aero style fixes (v2) (obsolete) — — Splinter Review 
Makes it look like the rest of the Aero TB ui. Now onto the fun part, the chat styling!
Attachment #599208 - Attachment is obsolete: true
Attached patch WIP 5 (obsolete) — — Splinter Review 
This attachment is an updated version of attachment 593063 [details] [diff] [review], now with IRC support. It also includes Andreas' attachment 600385 [details] [diff] [review].

Try server builds with this attachment are at https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-0ea9ece75584/

I'm working on UI stuff these days, so the changes in the next sets of builds will likely be more visible.
Attachment #593063 - Attachment is obsolete: true
Attachment #598818 - Attachment is obsolete: true
Attachment #600385 - Attachment is obsolete: true
Attached patch WIP 6 (obsolete) — — Splinter Review 
The left pane of the Chat tab now works mostly like I intend it to work. I also fixed some bitrot caused by bug 723081 so gloda indexing of conversations works again.
Attachment #602032 - Attachment is obsolete: true
Attached patch WIP 7 (obsolete) — — Splinter Review 
Most of the UI now works as intended.

I still need to add:
- a Chat tab in the preference window
- a context menu on the conversation content
- a status selector in the chat toolbar and in the Instant messaging status dialog.
Attachment #602406 - Attachment is obsolete: true
Comment on attachment 603751 [details] [diff] [review]
WIP 7

David said during the product council meeting today that he wants to look at the IRC code. It may also be a good idea to look at the interdiff between what you already reviewed and the current WIP (mostly handing your feedback and fixing a few bugs here and there).
Attachment #603751 - Flags: feedback?(dbienvenu)
Comment on attachment 603751 [details] [diff] [review]
WIP 7

the number of XXX comments in ircBase.jsm makes me uncomfortable - those should be cleaned up somehow. But in general, the code looks quite reasonable. Looking forward to trying it out!
Attachment #603751 - Flags: feedback?(dbienvenu) → feedback+
(In reply to David :Bienvenu from comment #56)
> Comment on attachment 603751 [details] [diff] [review]
> WIP 7
> 
> the number of XXX comments in ircBase.jsm makes me uncomfortable - those
> should be cleaned up somehow.

Do you mean the lines with only "// XXX" and no additional comment, or all the XXX comments in general?
These are all related to not-very-useful or non standard messages that we can receive from the server and haven't felt the need to handle yet (or don't even know what they mean / what their syntax is).

> Looking forward to trying it out!

There's a try server build with attachment 603751 [details] [diff] [review] at http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-faae3ee1390a/
(In reply to Florian Quèze from comment #57)
> (In reply to David :Bienvenu from comment #56)
> > Comment on attachment 603751 [details] [diff] [review]
> > WIP 7
> > 
> > the number of XXX comments in ircBase.jsm makes me uncomfortable - those
> > should be cleaned up somehow.
> 
> Do you mean the lines with only "// XXX" and no additional comment, or all
> the XXX comments in general?

Both - XXX generally means there's significant work left to do or something seriously potentially wrong. I'd be happy if you removed the ones that don't fit into those two categories. Generally, XXX comments need to be addressed before landing, and I'm presuming that none or very few of these need to be addressed before landing.
(In reply to David :Bienvenu from comment #58)

> Generally, XXX comments need to be addressed before landing,

Ah? My understanding (or at least what I do) is:
FIXME -> needs to be fixed, preferably before landing; can wait but is sure to hurt us in someway or another eventually
TODO -> not implemented and needs be eventually, but can wait
XXX (often followed by the irc nick of the author) -> reflects the thoughts or mood of the author of the code at the time of writing that specific piece of code. A typical example would be "XXX This doesn't seem exactly right, but the spec is inconsistent on this point anyway."

> and I'm presuming that none or very few of these need to be
> addressed before landing.

Right. This has already landed for Instantbird and went through lots of review iterations during the last few weeks (the JS-IRC code has actually been in development for over 2 years!). Details are in https://bugzilla.instantbird.org/show_bug.cgi?id=507
(In reply to David :Bienvenu from comment #56)
> the number of XXX comments in ircBase.jsm makes me uncomfortable - those
> should be cleaned up somehow. But in general, the code looks quite
> reasonable. Looking forward to trying it out!

David, I wrote almost all of that code, I'm glad you think it is reasonable! Florian's definition of my XXX comments is right: it means there's something left to do (which might just be researching what we need to do still), but IRC seemed quite usable without it. I'm OK changing these to TODO comments or something else, but I do not think it's appropriate to just remove them (as they represent SOME work to be done, even if it's not important).

Please let me know if you have any specific questions about the internal workings of the IRC code!
(In reply to Patrick Cloke [:clokep] from comment #60)
> (In reply to David :Bienvenu from comment #56)
> > the number of XXX comments in ircBase.jsm makes me uncomfortable - those
> > should be cleaned up somehow. But in general, the code looks quite
> > reasonable. Looking forward to trying it out!
> 
> David, I wrote almost all of that code, I'm glad you think it is reasonable!
> Florian's definition of my XXX comments is right: it means there's something
> left to do (which might just be researching what we need to do still), but
> IRC seemed quite usable without it. I'm OK changing these to TODO comments
> or something else, but I do not think it's appropriate to just remove them
> (as they represent SOME work to be done, even if it's not important).
yes, please don't remove them if they're still valid; I'd just rather not use XXX. I'd use TODO if there's something to do; for things where you're not sure, perhaps a more specific comment to that effect.
Attached patch chat messages styling (obsolete) — — Splinter Review 
Took it from multicolored crazy bubbles to something more moderate (and twitter-like)
Attached patch gnomestripe toolbar icons (obsolete) — — Splinter Review 
winstripe and qute coming up!
(In reply to Andreas Nilsson (:andreasn) from comment #63)
> Created attachment 604281 [details] [diff] [review]
> gnomestripe toolbar icons
> 
> winstripe and qute coming up!

I assume you meant pinstripe :).

In this attachment, are the files join-chat.png and join-chat-small.png used anywhere?
(In reply to Andreas Nilsson (:andreasn) from comment #63)
> Created attachment 604281 [details] [diff] [review]
> gnomestripe toolbar icons
> 
> winstripe and qute coming up!

Andreas, would you mind posting screenshots for those of us who can't rebuild the beast ? ;-)

Tx
(In reply to Jb Piacentino who requested a screenshot in comment #65)
(In reply to Florian Quèze from comment #64)
> (In reply to Andreas Nilsson (:andreasn) from comment #63)
> > Created attachment 604281 [details] [diff] [review]
> > gnomestripe toolbar icons
> > 
> > winstripe and qute coming up!
> 
> I assume you meant pinstripe :).

Heh, yes. 

> In this attachment, are the files join-chat.png and join-chat-small.png used
> anywhere?

Sorry, these are leftovers that I put there by mistake. Will fix.
Attached patch Patch v8 (obsolete) — — Splinter Review 
This seems ready for a string freeze! (except for the strings you will want to change during the review of course)
Attachment #603751 - Attachment is obsolete: true
Attachment #604461 - Flags: review?(dbienvenu)
Attachment #604461 - Flags: review?(bwinton)
Attached patch toolbar icons (v2) (obsolete) — — Splinter Review 
Also includes pinstripe and qute icons
Attachment #604281 - Attachment is obsolete: true
Attached patch icons (v3) (obsolete) — — Splinter Review 
With preferences icons (wip)
Attachment #604465 - Attachment is obsolete: true
Comment on attachment 604461 [details] [diff] [review]
Patch v8

In general, I'm fine with this, modulo my comments about XXX comments. You can simply remove the XXX if you want, and file follow up bugs.

I assume once we get our new twitter api key, logging onto twitter won't tell me that Instantbird is trying to logon?
Attachment #604461 - Flags: review?(dbienvenu) → review+
Oh yes, one other comment to muddy the water - this is a UI point, but a source of a bit of pain for me as a user - it would be nice if I could reconnect directly from the IM tab, e.g., by right clicking on the account and clicking connect/disconnect, instead of having to go through the accounts window, which I don't otherwise need.
(In reply to David :Bienvenu from comment #71)

> I assume once we get our new twitter api key, logging onto twitter won't
> tell me that Instantbird is trying to logon?

Right, it will say Thunderbird and show a Thunderbird logo (even when running Daily or Earlybird; the alternative would be to have a different key for each of them and to put the key in the branding, but that would require the user to re-authentify when switching between channels).
(In reply to David :Bienvenu from comment #72)
> it would be nice if I could
> reconnect directly from the IM tab, e.g., by right clicking on the account
> and clicking connect/disconnect

Where do you expect to be able to right click exactly?

To connect or disconnect all your accounts at once, you should be able to use the status selector of the Chat toolbar (switching between an offline and a non-offline status); although that's neither as easy to use, nor as easy to discover as I intended :(.
Comment on attachment 604461 [details] [diff] [review]
Patch v8

Some comments:

"Please enter the username for your IRC account" should be "Please enter the username and server for your IRC account".

I see "JS Test" and "Override Test" chat networks.  I'm guessing they should be removed.

The error text for a selected account is dark red on a dark blue background.  It's really hard to read.

If we get a bad password, I think we should re-prompt, but I think you mentioned that that might be hard?

Sending a message to my wife (who isn't logged in), I get:
JavaScript error: chrome://messenger/content/messenger.xul, line 137: setColors is not defined

We shouldn't have the (Facebook, or any other) password box in the settings dialog, I think.

I tried to add a new Twitter account from the Instant Messaging Status dialog, and it never showed the OAuth window.

>+++ b/chat/chat-prefs.js
>@@ -0,0 +1,93 @@
>+// Setting the loglevel to a value smaller than 2 will cause messages
>+// with an INFO or MISC severity to be displayed as warnings so that
>+// their file URL is clickable
>+#ifndef DEBUG
>+// By default, show only warning and errors
>+pref("purple.debug.loglevel", 3);
>+#else
>+// On debug builds, show warning, errors and debug information.
>+pref("purple.debug.loglevel", 2);
>+#endif
>+
>+pref("purple.logging.format", "json");
>+pref("purple.logging.log_chats", true);
>+pref("purple.logging.log_ims", true);
>+pref("purple.logging.log_system", true);

Do we still need the "purple.*" prefs?  I thought we were getting rid of libpurple for some reason…

For the rest of this review, I'm going to concentrate on the strings…

>+++ b/chat/locales/en-US/accounts.properties
>@@ -0,0 +1,3 @@
>+passwordPromptTitle=Password for %S
>+passwordPromptText=Please enter your password for your account %S in order to connect it.

I don't think we need the extra "your account" here.
"Please enter your password for S in order to connect." seems good enough to me.

>+passwordPromptSaveCheckbox=Use Password Manager to remember this password.

These also need localization notes about what %S is.

>+++ b/chat/locales/en-US/irc.properties
>@@ -0,0 +1,148 @@
>+# LOCALIZATION NOTE (connection.*)
>+#   These will be displayed in the account manager in order to show the progress
>+#   of the connection.
>+#   (These will be displayed in account.connection.progress from
>+#    accounts.properties, which adds ⦠at the end, so do not include
>+#    periods at the end of these messages.)
>+connection.quitting=Sending the QUIT message

This seems a little technical to me.  Could we just say "Quitting" instead?

>+# LOCALIZATION NOTE (tooltip.*):
>+#    These are the descriptions given in a tooltip with information received
>+#    from a whois response.
>+#    The username is set by the user's IRC client, usually to the client's name
>+#    but the user can change it.
>+tooltip.user=Username
>+#    The host name that the user connects from (usually based on the
>+#    reverse DNS of the user's IP, but often mangled by the server to
>+#    protect users).
>+tooltip.host=Host name
>+#    The real name is a description of the user (including spaces).
>+tooltip.realname=Real name

If we're going to have "Host name" and "Real name", I think we should also use "User name".

>+++ b/chat/locales/en-US/twitter.properties
>@@ -0,0 +1,101 @@
>+# LOCALIZATION NOTE
>+#   This is the title of the conversation tab, %S will be replaced by
>+#   @<username>.
>+timeline=%S timeline

That sounds a little strange to me.  "%S's timeline" would be better, but I think that doesn't translate well…  We might have to just live with it, but what do you think about just "Timeline"?

>+++ b/chat/locales/en-US/xmpp.properties
>@@ -0,0 +1,66 @@
>+connection.error.failedToCreateASocket=Failed to create a socket (offline?)

I think we can expand this out to:
Failed to create a socket (Are you offline?)

>+connection.error.noCompatibleAuthMec=None of the authentication mechanisms offered by the server is supported

None of the authentication mechanisms offered by the server are supported

>+connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in clear

The server only supports authentication by sending the password in the clear
or
The server only supports authentication by sending the password in cleartext

>+connection.error.notAuthorized=Not authorized (wrong password?)

Not authorized (Did you enter the wrong password?)

>+options.connectionSecurity.allowUnencryptedAuth=Accept sending the password unencrypted

Allow sending the password unencrypted

>+++ b/mail/base/content/mailWindowOverlay.xul
>@@ -412,16 +412,17 @@
>   <key id="key_collapseAllThreads" key="&collapseAllThreadsCmd.key;" oncommand="goDoCommand('cmd_collapseAllThreads')"/>
>   <key key="&collapseAllThreadsCmd.key;" modifiers="shift"           oncommand="goDoCommand('cmd_collapseAllThreads')"/>
>   <key id="key_nextUnreadThread" key="&nextUnreadThread.key;"        oncommand="goDoCommand('cmd_nextUnreadThread')"/>
>   <key id="key_previousMsg" key="&prevMsgCmd.key;"                   oncommand="goDoCommand('cmd_previousMsg')"/>
>   <key id="key_previousUnreadMsg" key="&prevUnreadMsgCmd.key;"       oncommand="goDoCommand('cmd_previousUnreadMsg')"/>
>   <key id="key_archive" key="&archiveMsgCmd.key;"                    oncommand="goDoCommand('cmd_archive')"/>
>   <key id="key_goForward" key="&goForwardCmd.commandKey;"            oncommand="goDoCommand('cmd_goForward')"/>
>   <key id="key_goBack" key="&goBackCmd.commandKey;"                  oncommand="goDoCommand('cmd_goBack')"/>
>+  <key id="key_goChat" key="&goChatCmd.commandKey;"                  modifiers="accel,shift"/>

no oncommand="goDoCommand('cmd_goChat')" ?

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -0,0 +1,721 @@
>+    if (displayName != userName)
>+      displayName += " (" + userName + ")";

I think we might want to use a localizable string for this, to handle rtl languages better…

>+++ b/mail/locales/en-US/chrome/messenger/chat.dtd
>@@ -0,0 +1,40 @@
>+<!ENTITY chat.noPreviousConv.description       "&brandShortName; currently doesn't have any previous conversation stored for this contact.">

Should be "conversations".

>+++ b/mail/locales/en-US/chrome/messenger/chat.properties
>@@ -0,0 +1,64 @@
>+#LOCALIZATION NOTE
>+# The first parameter of this string will by the name of a buddy (either
>+# the alias followed by the username between parenthesis if an alias
>+# is set, or only the username otherwise).
>+# The second parameter will be the name of the protocol on which this
>+# buddy is removed (for example: AIM, MSN, Google Talk).
>+#
>+# Please find a wording that will keep the username as close as
>+# possible to the beginning of the string, because this is the
>+# important information that an user should see when looking quickly
>+# at this prompt.
>+buddy.deletePrompt.message=%S will be permanently removed from your %S buddy list if you continue.

For these, I think we really prefer to use %1$S and %2$S, so that French can re-order them if necessary.  ;)

>+chat.isTyping=is typingâ¦
>+chat.hasStoppedTyping=has stopped typing.
>+chat.contactIsTyping=%S is typing.
>+chat.contactHasStoppedTyping=%S has stopped typing.

Why do we have two of these?  What is chat.isTyping used for that couldn't use chat.contactIsTyping instead?

>+today=Today
>+yesterday=Yesterday

What do you think about adding "Last week" and "Last month", too?

>+++ b/mail/locales/en-US/chrome/messenger/imAccountWizard.dtd
>@@ -0,0 +1,28 @@
>+<!ENTITY accountProtocolTitle.label   "Chat service">
>+<!ENTITY accountProtocolInfo.label    "Please choose the network of your chat account.">
>+<!ENTITY accountProtocolField.label   "Protocol:">

If we're saying "choose the network", shouldn't the field also be named "Network", and the title be "Chat network"?

>+<!ENTITY accountAliasInfo.label       "This will only be displayed in your conversations when you talk, remote buddies won't see it.">

Is "buddies" the right term here?  We seem to use "contacts" in other places…

>+++ b/mail/locales/en-US/chrome/messenger/imAccounts.dtd
>@@ -0,0 +1,26 @@
>+<!-- This title must be short, displayed with a big font size -->
>+<!ENTITY accountManager.noAccount.title      "No account configured yet!">

I think we should remove this "!"

>+<!ENTITY account.cancelReconnection.label         "Cancel reconnection">
>+<!ENTITY account.cancelReconnection.accesskey     "A">

Should that be a capital "A", or a lowercase "a"?

>+++ b/mail/locales/en-US/chrome/messenger/imAccounts.properties
>@@ -0,0 +1,32 @@
>+# %S is replaced by the name of a protocol
>+protoOptions=%S Options
>+accountUsername=Username:
>+accountColon=%S:
>+accountUsernameInfo=Please enter the username for your %S account.
>+accountUsernameInfoWithDescription=Please enter the username (%S) for your %S account.

Are both these "%S"s really replaced by the name of a protocol?

>+++ b/mail/locales/en-US/chrome/messenger/preferences/chat.dtd
>@@ -0,0 +1,14 @@
>+<!ENTITY  reportIdleAfter.label         "Let my contacts know that I am Idle after">
>+<!ENTITY  reportIdleAfter.accesskey     "I">
>+<!ENTITY  idleTime                      "minutes of inactivity">

I wonder how much the localizers will like this…

>+++ b/mail/test/mozmill/migration/test-toolbar.js
>@@ -41,31 +41,31 @@
> // Use the Windows/Linux settings as the default, but check out setupModule.
> var DEFAULT_TB2_SET = "button-getmsg,button-newmsg,button-address,separator,button-reply,button-replyall,button-replylist,button-forward,separator,button-tag,button-delete,button-junk,button-print,separator,button-goback,button-goforward,spring,gloda-search";
>-var DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-address,separator,button-tag,qfb-show-filter-bar,spring,gloda-search"
>+var DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-chat,button-address,separator,button-tag,qfb-show-filter-bar,spring,gloda-search";

I think I might put chat after address book…

[snip…]
>   // The Mac has different settings for the toolbar, so adjust for that.
>   if (Application.platformIsMac) {
>     DEFAULT_TB2_SET = "button-getmsg,button-newmsg,button-address,spacer,button-reply,button-replyall,button-replylist,button-forward,spacer,button-tag,button-delete,button-junk,button-print,spacer,button-goback,button-goforward,spring,gloda-search,throbber-box";
>-    DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-address,spacer,button-tag,qfb-show-filter-bar,spring,gloda-search";
>+    DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-chat,button-address,spacer,button-tag,qfb-show-filter-bar,spring,gloda-search";

Ditto…

So, I guess that's an r=me for the strings with those changes made, but I explicitly haven't reviewed the code, nor done a ui-review, so this shouldn't be checked in yet.

Thanks,
Blake.
Attachment #604461 - Flags: review?(bwinton) → review+
Comment on attachment 604461 [details] [diff] [review] [diff] [review]
Patch v8

mail/themes/qute/mail/chat.css has an deprecated -moz-border-radius. Can you change it to only border-radius? Then #IMSearchInput has also a radius under Windows TB 13.
> If we're going to have "Host name" and "Real name", I think we should also use "User name".

How about hostname in one word and username in one? User name in two words always sounds to me like it might be the user's name, which it usually isn't.
I tried a IM build from try server under windows and found this issues:

- The IM toolbar under Win7 has a negative margin of 3px on the left because of a definition from .chromeclass-toolbar. This toolbar has no ID and this makes it difficult to apply a -moz-margin-start: 0px. Please can you add a ID?

- With a ID under XP and Win7 a bottom border can be applied. The border is missing on both systems. Normally this border is applied with the toolbox but the toolbar is made without a toolbox.

- The toolbar isn't customizable to switch between big and small icons or only text, only icon, icons and text or the normally default of icons beside text.
(In reply to Richard Marti [:paenglab] from comment #78)
> I tried a IM build from try server under windows and found this issues:
> 
> - The IM toolbar under Win7 has a negative margin of 3px on the left because
> of a definition from .chromeclass-toolbar. This toolbar has no ID and this
> makes it difficult to apply a -moz-margin-start: 0px. Please can you add a
> ID?
> 
> - With a ID under XP and Win7 a bottom border can be applied. The border is
> missing on both systems. Normally this border is applied with the toolbox
> but the toolbar is made without a toolbox.
> 
> - The toolbar isn't customizable to switch between big and small icons or
> only text, only icon, icons and text or the normally default of icons beside
> text.

Thanks for trying it and thanks for the feedback! I think most (if not all) of your concerns expressed here have been addressed in attachment 604461 [details] [diff] [review] (especially, the toolbar is now customizable, it has an id, and a toolbox is used).
I assume you tried the try server build from the link I gave in comment 57, which was based on attachment 603751 [details] [diff] [review].

I pushed to the try server again after attaching attachment 604461 [details] [diff] [review], but the build seems to have failed because of an unrelated bustage. I'll probably push to try again tomorrow after making changes requested in comment 75.
Attached patch icons (v4) (obsolete) — — Splinter Review 
Toolbar icons now displays correctly on Aero.
Attachment #604484 - Attachment is obsolete: true
Attached patch Patch v9 (obsolete) — — Splinter Review 
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #75)
> Comment on attachment 604461 [details] [diff] [review]

Thanks for the feedback!
I'm replying only to the comments that I haven't addressed with the new patch or that asked a question. 


> "Please enter the username for your IRC account" should be "Please enter the
> username and server for your IRC account".

This string is generic, with only the protocol name which is replaced. Do you really want us to introduce an IRC special case?

We have a second strings that is "Please enter the username (<user name hint>) for your <protocol name> account". Would you like us to display the string "and server" as a hint for IRC? That would be a bit hackish, but save us the special case in the wizard.

By the way, for Google Talk, I think we will want to add "email address" as a username hint (that's way easier to do than adding an empty text in the focused username box).


> I see "JS Test" and "Override Test" chat networks.  I'm guessing they should
> be removed.

Except if I broke something recently, these are built only in debug builds, and never packaged. You were testing a local debug build, right?
 
> The error text for a selected account is dark red on a dark blue background.
> It's really hard to read.

Maybe Andreas can propose a CSS fix for this?
 
> If we get a bad password, I think we should re-prompt, but I think you
> mentioned that that might be hard?

That may not be very difficult actually; we already prompt for a password when the user attempts to connect a password without any password saved in the password manager. I'll try to look into this soon!

> Sending a message to my wife (who isn't logged in), I get:
> JavaScript error: chrome://messenger/content/messenger.xul, line 137:
> setColors is not defined

This JS error isn't related to sending something to someone who isn't logged in. Any message bubble causes this error; it's a consequence of applying attachment 604280 [details] [diff] [review] which removed the setColors function, but not it's caller.


> We shouldn't have the (Facebook, or any other) password box in the settings
> dialog, I think.

Hmm... I agree this is surprising, especially for Facebook/Gtalk. But for IRC (where the password is optional) we will never prompt for a password if there isn't one, so having it in the settings sounds right in that case.

If you want us to add a check to show the password box only for accounts that support a password but don't require it (so will never prompt), I think that's possible.

> I tried to add a new Twitter account from the Instant Messaging Status
> dialog, and it never showed the OAuth window.

I haven't tried to reproduce this yet.


> >+++ b/chat/chat-prefs.js
> >@@ -0,0 +1,93 @@
> >+// Setting the loglevel to a value smaller than 2 will cause messages
> >+// with an INFO or MISC severity to be displayed as warnings so that
> >+// their file URL is clickable
> >+#ifndef DEBUG
> >+// By default, show only warning and errors
> >+pref("purple.debug.loglevel", 3);
> >+#else
> >+// On debug builds, show warning, errors and debug information.
> >+pref("purple.debug.loglevel", 2);
> >+#endif
> >+
> >+pref("purple.logging.format", "json");
> >+pref("purple.logging.log_chats", true);
> >+pref("purple.logging.log_ims", true);
> >+pref("purple.logging.log_system", true);
> 
> Do we still need the "purple.*" prefs?  I thought we were getting rid of
> libpurple for some reason…

When we rewrote the logging code in JS to replace the libpurple logger, we kept the existing preferences to avoid having to write migration code, so the logger actually uses these prefs.



> >+++ b/chat/locales/en-US/irc.properties
> >@@ -0,0 +1,148 @@
> >+# LOCALIZATION NOTE (connection.*)
> >+#   These will be displayed in the account manager in order to show the progress
> >+#   of the connection.
> >+#   (These will be displayed in account.connection.progress from
> >+#    accounts.properties, which adds ⦠at the end, so do not include
> >+#    periods at the end of these messages.)
> >+connection.quitting=Sending the QUIT message
> 
> This seems a little technical to me.  Could we just say "Quitting" instead?

I was surprised by this message and searched for when it is displayed: it isn't! I've just removed it (and the localization note about it was just wrong, it's the result of a copy/paste from xmpp.properties).

> >+# LOCALIZATION NOTE (tooltip.*):
> >+#    These are the descriptions given in a tooltip with information received
> >+#    from a whois response.
> >+#    The username is set by the user's IRC client, usually to the client's name
> >+#    but the user can change it.
> >+tooltip.user=Username
> >+#    The host name that the user connects from (usually based on the
> >+#    reverse DNS of the user's IP, but often mangled by the server to
> >+#    protect users).
> >+tooltip.host=Host name
> >+#    The real name is a description of the user (including spaces).
> >+tooltip.realname=Real name
> 
> If we're going to have "Host name" and "Real name", I think we should also
> use "User name".

I really dislike these strings. This tooltip is IMHO currently quite confusing and unfriendly. We started discussing replacing them in https://bugzilla.instantbird.org/show_bug.cgi?id=1293

 
> >+++ b/chat/locales/en-US/twitter.properties
> >@@ -0,0 +1,101 @@
> >+# LOCALIZATION NOTE
> >+#   This is the title of the conversation tab, %S will be replaced by
> >+#   @<username>.
> >+timeline=%S timeline
> 
> That sounds a little strange to me.  "%S's timeline" would be better, but I
> think that doesn't translate well…

Hmm, not sure (I don't see an issue to translate it into French; I don't know for other languages).

> what do you think about just "Timeline"?

The point of showing the username, and wanting it at the beginning of the title is to distinguish Twitter timelines from different twitter accounts (a user can have an arbitrary number of twitter accounts configured in the same Thunderbird instance).


> >+++ b/mail/base/content/mailWindowOverlay.xul
> >@@ -412,16 +412,17 @@
> >   <key id="key_collapseAllThreads" key="&collapseAllThreadsCmd.key;" oncommand="goDoCommand('cmd_collapseAllThreads')"/>
> >   <key key="&collapseAllThreadsCmd.key;" modifiers="shift"           oncommand="goDoCommand('cmd_collapseAllThreads')"/>
> >   <key id="key_nextUnreadThread" key="&nextUnreadThread.key;"        oncommand="goDoCommand('cmd_nextUnreadThread')"/>
> >   <key id="key_previousMsg" key="&prevMsgCmd.key;"                   oncommand="goDoCommand('cmd_previousMsg')"/>
> >   <key id="key_previousUnreadMsg" key="&prevUnreadMsgCmd.key;"       oncommand="goDoCommand('cmd_previousUnreadMsg')"/>
> >   <key id="key_archive" key="&archiveMsgCmd.key;"                    oncommand="goDoCommand('cmd_archive')"/>
> >   <key id="key_goForward" key="&goForwardCmd.commandKey;"            oncommand="goDoCommand('cmd_goForward')"/>
> >   <key id="key_goBack" key="&goBackCmd.commandKey;"                  oncommand="goDoCommand('cmd_goBack')"/>
> >+  <key id="key_goChat" key="&goChatCmd.commandKey;"                  modifiers="accel,shift"/>
> 
> no oncommand="goDoCommand('cmd_goChat')" ?

There's no <command> for it, and I don't see how adding one would be useful here.


> >+++ b/mail/locales/en-US/chrome/messenger/chat.properties

> >+buddy.deletePrompt.message=%S will be permanently removed from your %S buddy list if you continue.
> 
> For these, I think we really prefer to use %1$S and %2$S, so that French can
> re-order them if necessary.  ;)

I've changed the %S to %n$S in a few places, but this is just a remainder to translators that they can change the order(it's possible anyway even when we just write %S in the en-US file).


> >+chat.isTyping=is typingâ¦
> >+chat.hasStoppedTyping=has stopped typing.
> >+chat.contactIsTyping=%S is typing.
> >+chat.contactHasStoppedTyping=%S has stopped typing.
> 
> Why do we have two of these?  What is chat.isTyping used for that couldn't
> use chat.contactIsTyping instead?

The isTyping and hasStoppedTyping strings are used instead of the contact's status message, so there's <username> with a big font on a first line, and then either "is typing" or "has stopped typing" displayed with a smaller font on a second line.
The formatted variant is used for a tooltip displayed when hovering the status icon.


> >+today=Today
> >+yesterday=Yesterday
> 
> What do you think about adding "Last week" and "Last month", too?

I'm not exactly sure of what you mean here. Are you talking about a "Last week" string (I don't know where it would be used exactly) or about special casing the conversations that happened during the last 7 days to show "Monday", "Tuesday", etc... instead of the date?
In any case, I think that this feels like more work ;).


> >+++ b/mail/locales/en-US/chrome/messenger/imAccountWizard.dtd
> >@@ -0,0 +1,28 @@
> >+<!ENTITY accountProtocolTitle.label   "Chat service">
> >+<!ENTITY accountProtocolInfo.label    "Please choose the network of your chat account.">
> >+<!ENTITY accountProtocolField.label   "Protocol:">
> 
> If we're saying "choose the network", shouldn't the field also be named
> "Network", and the title be "Chat network"?

The field should definitely not be named "protocol:" (I changed it to "Network:"). I'm not sure for the title, Chat service seems ok to me (it's what Jb suggested) but if you want to change it, that's ok with me too.



> >+++ b/mail/locales/en-US/chrome/messenger/imAccounts.properties

> >+accountUsernameInfoWithDescription=Please enter the username (%S) for your %S account.
> 
> Are both these "%S"s really replaced by the name of a protocol?

The first one is a hint for the expected format of the username. I added a localization note.

> >+++ b/mail/locales/en-US/chrome/messenger/preferences/chat.dtd
> >@@ -0,0 +1,14 @@
> >+<!ENTITY  reportIdleAfter.label         "Let my contacts know that I am Idle after">
> >+<!ENTITY  reportIdleAfter.accesskey     "I">
> >+<!ENTITY  idleTime                      "minutes of inactivity">
> 
> I wonder how much the localizers will like this…

I added a localization note.


> >+++ b/mail/test/mozmill/migration/test-toolbar.js
> >@@ -41,31 +41,31 @@
> > // Use the Windows/Linux settings as the default, but check out setupModule.
> > var DEFAULT_TB2_SET = "button-getmsg,button-newmsg,button-address,separator,button-reply,button-replyall,button-replylist,button-forward,separator,button-tag,button-delete,button-junk,button-print,separator,button-goback,button-goforward,spring,gloda-search";
> >-var DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-address,separator,button-tag,qfb-show-filter-bar,spring,gloda-search"
> >+var DEFAULT_TB3_SET = "button-getmsg,button-newmsg,button-chat,button-address,separator,button-tag,qfb-show-filter-bar,spring,gloda-search";
> 
> I think I might put chat after address book…

That's where I had it first. I recently moved to between the "Write" and the "Address book" button, as "Chat" seemed related to "Write" to me, as they are both about sending messages, a opposed to "Address book" which is more about consulting or editing locally stored data.
Of course if you use the address book as the place to start writing your emails because you open the address book, select your contact, and then click the "Write" button of the address book toolbar, this point is moot.

If you really want it after the address book, I can change it; and will also need to change the toolbar migration code.
Attachment #604461 - Attachment is obsolete: true
Comment on attachment 604802 [details] [diff] [review]
Patch v9

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

Hey Florian,

chat-messenger-overlay.js looks pretty reasonable.  Just a few comments, nothing major.

-Mike

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

This license block should probably be MPL 2'd. That goes for all new code.

@@ +43,5 @@
> +
> +var gBuddyListContextMenu = null;
> +
> +function buddyListContextMenu(aXulMenu) {
> +  this.target  = document.popupNode;

It looks like there was some attempt here to space the assignment operators so that they all line up.  And then the attempt was abandoned.

We might as well just remove the extra padding after this.target and this.menu.

Also, according to MDN, document.popupNode is deprecated in favour of menupopup's "triggerNode" instead.  (https://developer.mozilla.org/en/DOM/document.popupNode)

@@ +46,5 @@
> +function buddyListContextMenu(aXulMenu) {
> +  this.target  = document.popupNode;
> +  this.menu    = aXulMenu;
> +  let localName = this.target.localName;
> +  this.onContact = localName == "imcontact";

Where are these magic strings imcontact / imconv used?  I wonder if it would be better to have them be consts somewhere, to pull out the hardcoded strings.

@@ +51,5 @@
> +  this.onConv = localName == "imconv";
> +  this.shouldDisplay = this.onContact || this.onConv;
> +
> +  let hide = !this.onContact;
> +  [ "context-openconversation",

In the Javascript I'm familiar with, we don't generally have a unit of whitespace after the opening brace of an Array.  Maybe let's leave that out.

@@ +55,5 @@
> +  [ "context-openconversation",
> +    "context-edit-buddy-separator",
> +    "context-alias",
> +    "context-delete"
> +  ].forEach(function (aId) {

Why the space after "function"?  That's inconsistent with the other function definitions I'm seeing around.

@@ +63,5 @@
> +  document.getElementById("context-close-conversation").hidden = !this.onConv;
> +  document.getElementById("context-openconversation").disabled =
> +    !hide && !this.target.canOpenConversation();
> +}
> +buddyListContextMenu.prototype = {

Each of the functions in this object prototype needs documentation.

@@ +90,5 @@
> +    if (displayName != userName) {
> +      displayName = bundle.getFormattedString("buddy.deletePrompt.displayName",
> +                                              [displayName, userName]);
> +    }
> +    let proto = buddy.protocol.name; // FIXME build a list

I don't think I understand this FIXME - regardless, something in here probably needs fixing...

@@ +106,5 @@
> +    this.target.remove();
> +  }
> +};
> +
> +

Why so many newlines?

@@ +181,5 @@
> +    let selectedItem = list.selectedItem;
> +    let shouldSelect =
> +      gChatTab && gChatTab.tabNode.selected &&
> +      (!selectedItem || (selectedItem == convs &&
> +                        convs.nextSibling.localName != "imconv"));

"imconv" and "imcontact" have shown up a few times in this file now.  I think they should definitely be replaced with constants.

@@ +228,5 @@
> +    }
> +
> +    let status = target.getAttribute("status");
> +    if (!status)
> +      return; // is this really possible?

What's this comment all about?

@@ +268,5 @@
> +    let time = dts.FormatTime("", dts.timeFormatNoSeconds,
> +                              aDate.getHours(), aDate.getMinutes(),0);
> +    let bundle = document.getElementById("chatBundle");
> +    if (aDate >= today)
> +      return bundle.getString("today") + " " + time;

Would it not be better to have the time be embedded in the translation string, like %1$S, to give the localizers some more flexibility?

@@ +382,5 @@
> +      this.observedContact = null;
> +      return;
> +    }
> +    if (item.getAttribute("id") == "searchResultConv") {
> +      let path = "logs/" + item.log.path;

Should this path be hardcoded, or be a pref?

@@ +473,5 @@
> +  },
> +
> +  _openDialog: function(aType) {
> +    let features = "chrome,modal,titlebar,centerscreen";
> +    window.openDialog("chrome://messenger/content/" + aType + ".xul", "",

Hm...the dialogs have all been put under messenger/content?  Would it not be better to have a "namespace", like messenger/content/chat?

@@ +492,5 @@
> +
> +    // Compute the color based on the nick
> +    var nick = aName.match(/[a-zA-Z0-9]+/);
> +    nick = nick ? nick[0].toLowerCase() : nick = aName;
> +    var weight = 10;

Hm.  The magic numbers in this function should be re-assigned to constants.

@@ +508,5 @@
> +
> +  _updateNoConvPlaceHolder: function() {
> +    let connected = false;
> +    let hasAccount = false;
> +    for (let account in fixIterator(imServices.accounts.getAccounts())) {

Purely optional here - but how about we set hasAccount to true if the cardinality of imServices.accounts.getAccounts() > 0, and then use Array.some (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/some) to set "connected"?

@@ +523,5 @@
> +  },
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic == "browser-request") {
> +      imServices.ww.openWindow(null,
> +                               "chrome://chat/content/browserRequest.xul",

Hm.   So some dialogs are at chat/content/*, and others are at messenger/content/*?
Attached patch icons v5 (obsolete) — — Splinter Review 
now builds on top of Florians v9 patch
Attachment #604799 - Attachment is obsolete: true
Attached patch icons v6 (obsolete) — — Splinter Review 
Now with native status icons for all 3 platforms
Attachment #604880 - Attachment is obsolete: true
Attached patch icons v6 (obsolete) — — Splinter Review 
(just removing a piece of extra commented out code I forgot in there)
Attachment #604889 - Attachment is obsolete: true
Attached patch icons v7 (obsolete) — — Splinter Review 
More status icons functionality (for the classes too now, so it works in the sidebar on the right)
Attachment #604890 - Attachment is obsolete: true
Comment on attachment 604802 [details] [diff] [review]
Patch v9

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

I've just been through the build config & L10n aspects of this. Comments follow.

The L10n comments need fixing before this lands. Please also ask me for review prior to landing, so that I can check through the updates.

I think the Build Config changes are largely performance improvements for building, and possibly for TB itself, and so may be fixed in a follow-up bug post landing.

::: chat/Makefile.in
@@ +41,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +PROTOCOLS = \
> +		facebook \

nit: we've moved to using two-space indent rather than tabs for these type of continuations in Makefiles.

@@ +56,5 @@
> +PREF_JS_EXPORTS = $(srcdir)/chat-prefs.js
> +
> +PARALLEL_DIRS	= \
> +		components/public \
> +		components/src \

Please consider putting these into one directory, maybe mixed with modules as well. We're generally trying to cut down on the amount of separate directories that make has to enter.

@@ +61,5 @@
> +		modules \
> +		content \
> +		themes \
> +		locales \
> +		$(foreach proto,$(PROTOCOLS),protocols/$(proto)) \

Could we restructure this so that we only have one Makefile in protocols, and everything in the sub-directories is included from that? I'd really prefer not to have a multitude of directories added.

::: chat/components/src/Makefile.in
@@ +41,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +EXTRA_COMPONENTS = \
> +		imAccounts.js imAccounts.manifest \

Unless you've got a good reason to do so, I'd suggest incorporating all the .manifest files into one, this will help avoid individual file processing during the make and packaging stages. Possibly during startup as well, though I can't quite remember how much they are already grouped together.

::: chat/locales/Makefile.in
@@ +37,5 @@
> +DEPTH		= ../..
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH		= @srcdir@
> +relativesrcdir	= chat/locales

You need to update mail/locales/l10n*.ini and mail/locales/filter.py otherwise l10n won't see these additions.

@@ +50,5 @@
> +
> +libs-%: AB_CD=$*
> +libs-%:
> +	# XXX: it would be nice if we could just do nothing if no langpack is being done
> +	#  currently, we just go and (re)build en-US if called with a non-supported locale

I don't see why you can't get away without this, e.g. see /editor/ui/locales/Makefile.in

::: chat/locales/en-US/accounts.properties
@@ +1,1 @@
> +# LOCALIATION NOTE

LOCALIZATION (include the Z).

Also, you should make it like:

LOCALIZATION NOTE (passwordPromptTitle, passwordPromptText):

as per other instances.

::: chat/locales/en-US/conversations.properties
@@ +27,5 @@
> +autoReply=Auto-reply - %S
> +
> +#LOCALIZATION NOTE
> +# ellipsis is used when copying a part of a message to show that the message was cut
> +messenger.conversations.selections.ellipsis=[â¦]

Please specify the string name in a localization note.

@@ +29,5 @@
> +#LOCALIZATION NOTE
> +# ellipsis is used when copying a part of a message to show that the message was cut
> +messenger.conversations.selections.ellipsis=[â¦]
> +
> +#LOCALIZATION NOTE

Need to specify the string names here.

::: chat/locales/en-US/irc.properties
@@ +1,1 @@
> +# LOCALIZATION NOTE (connection.error.*)

nit: Add a colon on the end.

@@ +4,5 @@
> +connection.error.lost=Lost connection with server
> +connection.error.timeOut=Connection timed out
> +connection.error.certError=Certification error when connecting to server
> +
> +# LOCALIZATION NOTE

nit: Add string names.

@@ +9,5 @@
> +#   These show up on the join chat menu. An underscore is for the access key.
> +joinChat.channel=_Channel
> +joinChat.password=_Password
> +
> +# LOCALIZATION NOTE

nit: add string names.

@@ +20,5 @@
> +options.quitMessage=Quit message
> +options.partMessage=Part message
> +options.showServerTab=Show messages from the server
> +
> +# LOCALIZATION NOTE:

bit: string names required here.

@@ +23,5 @@
> +
> +# LOCALIZATION NOTE:
> +#   %1$S is the nickname of the user who was pinged.
> +#   %2$S is the delay (in seconds).
> +ctcp.ping=Ping reply from %1$S in %2$S seconds.

This needs to be adapted for plural form.

See https://developer.mozilla.org/en/Localization_and_Plurals#Developing_with_PluralForm for more details.

::: chat/locales/en-US/status.properties
@@ +1,2 @@
> +#LOCALIZATION NOTE
> +# This used to be in instantbird.properties, you don't need to retranslate it.

That's not applicable here is it?

@@ +7,5 @@
> +offlineStatusType=Offline
> +invisibleStatusType=Invisible
> +idleStatusType=Idle
> +mobileStatusType=Mobile
> +#LOCALIZATION NOTE

Add the string name.

@@ +11,5 @@
> +#LOCALIZATION NOTE
> +# the status of a buddy is unknown when it's in the list of a disconnected account
> +unknownStatusType=Unknown
> +
> +#LOCALIZATION NOTE

Add the string name.

::: chat/locales/en-US/twitter.properties
@@ +9,5 @@
> +error.general=An error %S occurred while sending: %S
> +error.retweet=An error %S occurred while retweeting: %S
> +error.delete=An error %S occurred while deleting: %S
> +
> +# LOCALIZATION NOTE

Missing string names throughout this file on Localization notes.

::: mail/components/im/content/am-im.xul
@@ +44,5 @@
> +  id     = "account"
> +  title  = "&accountWindow.title;"
> +  buttons= "accept,cancel"
> +  width  = "&accountWindow.width;"
> +  height = "400"

Please use style with width: and height: here. doing it this way will let localisers adapt, but also it'll make sure that they use the right units and get the notation right.

Talking about units, shouldn't that be in em or ch these days?

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +1,3 @@
> +chatTabTitle=Chat
> +goBackToCurrentConversation.button=Back to current conversation
> +startAConversationWith.button=Start a conversation with %S

This needs a localization note for the %S

@@ +1,5 @@
> +chatTabTitle=Chat
> +goBackToCurrentConversation.button=Back to current conversation
> +startAConversationWith.button=Start a conversation with %S
> +
> +#LOCALIZATION NOTE

Please include the string names (various places in this file).

@@ +42,5 @@
> +
> +#LOCALIZATION NOTE
> +# the & symbol indicates the position of the character that should be
> +# used as the accesskey for this button.
> +buddy.deletePrompt.button=&Delete

Please separate this out into .label and .accesskey, as is the standard for L10n (I believe some locales can't specify a key in the string for the access key, but we should stick to the standard anyway).

::: mail/locales/en-US/chrome/messenger/imAccounts.dtd
@@ +1,2 @@
> +<!ENTITY accountsWindow.title                "Instant messaging status">
> +<!ENTITY accountManager.width                "450">

Please use style, as mentioned previously.

::: mail/locales/en-US/chrome/messenger/imAccounts.properties
@@ +1,1 @@
> +# %S is replaced by the name of a protocol

In this file, please ensure localization notes for all %S values, and include the LOCALIZATION NOTE header lines.

@@ +8,5 @@
> +# %2$S is the name of a protocol
> +accountUsernameInfoWithDescription=Please enter the username (%1$S) for your %2$S account.
> +
> +account.connection.error=Error: %S
> +account.connection.errorUnknownPrpl= No '%S' protocol plugin.

Is the space at the beginning intentional? If so, you should replace it with the html escaped value and add a localization note about it.

@@ +20,5 @@
> +account.reconnectInDouble=Reconnection in %S %S and %S %S.
> +account.reconnectInSingle=Reconnection in %S %S.
> +
> +requestAuthorizeTitle=Authorization request
> +requestAuthorizeAllow=&Allow

What's the & doing?

If this is to set the access key, then you need to separate this out into two strings, a .label and a .accesskey. Ditto for requestAuthorizeDeny.

If not, this definitely needs a localization note to explain it.

@@ +30,5 @@
> +accountsManager.notification.userDisabled.label=You have disabled automatic connections.
> +accountsManager.notification.safeMode.label=Automatic Connection Settings have been ignored because the application is currently running in Safe-Mode.
> +accountsManager.notification.startOffline.label=Automatic Connection Settings have been ignored because the application was started in Offline Mode.
> +accountsManager.notification.crash.label=The last run exited unexpectedly while connecting. Automatic Connections have been disabled to give you an opportunity to Edit your Settings.
> +accountsManager.notification.singleCrash.label=A previous run exited unexpectedly while connecting a new or edited account. It has not been connected so that you can Edit its Settings.;A previous run exited unexpectedly while connecting #1 new or edited accounts. They have not been connected so that you can Edit their Settings.

Please add an l10n note explaining that #1 is replaced by a number and this is using plural form.
Comment on attachment 604802 [details] [diff] [review]
Patch v9

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

There are several occurrences (at least 10 of them) of synchronous SQL being performed in your code, including statements that write to the database. The Tag service and the Contacts object seem to be designed to work in a synchronous fashion. While I understand that it may be painful to rewrite your code to work in an asynchronous fashion, I do think that such cleanups must be performed before landing. With a lot of database activity occurring in Thunderbird already, esp. with Gloda doing its indexing and whatnot, if your synchronous statement happens to trigger an fsync() of the underlying filesystem, and if I'm not mistaken, the UI is liable to lock up for several minutes in the worst case. I don't think we want to have that happening in Thunderbird.

I mentioned this several times already; I understand my comments may not have much value, since I do not have the "performance expert" hat :). Taras, Yoric, could you please chime in and tell us if it's okay landing code that performs synchronous IO? I may be exaggerating the situation, so I'd be happy if you could share your thoughts on performing synchronous calls to mozStorage on the main thread :).

::: chat/components/src/imContacts.js
@@ +136,5 @@
> +      return tag;
> +
> +    let statement = DBConn.createStatement("INSERT INTO tags (name, position) VALUES(:name, 0)");
> +    statement.params.name = aName;
> +    statement.executeStep();

This is a synchronous SQL statement. If you're ok with freezing the main UI for potentially long periods of time (I think the order of magnitude in the worst case is several minutes), that's fine. Otherwise, you must switch to asynchronous IO.
(In reply to Jonathan Protzenko [:protz] from comment #88)

> I mentioned this several times already; I understand my comments may not
> have much value, since I do not have the "performance expert" hat :).

For what is worth, I haven't ignored your comments. I've been asked to do whatever I could to present a patch that would be ready to land before the next aurora merge (tomorrow), so the top priority has been to finish ASAP the UI parts that require localizable strings; and it was my understanding that investigating potential performance issues and rewriting (hopefully small) parts of the code to be asynchronous could land on aurora (although that's obviously not ideal).

Also, if that makes you feel better, I added an about:config preference to completely turn off the Chat feature, so that we can still change our mind in a few weeks if we discovered a serious issues that can't reasonably be fixed on aurora and that we aren't comfortable shipping with.
Attached patch icons v8 (obsolete) — — Splinter Review 
Corrected odd image-regions on OSX.
Attachment #604891 - Attachment is obsolete: true
(In reply to Florian Quèze from comment #81)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #75)
> > "Please enter the username for your IRC account" should be "Please enter the
> > username and server for your IRC account".
> This string is generic, with only the protocol name which is replaced. Do
> you really want us to introduce an IRC special case?

Ideally, yes, but I could live without it.

> We have a second strings that is "Please enter the username (<user name
> hint>) for your <protocol name> account". Would you like us to display the
> string "and server" as a hint for IRC? That would be a bit hackish, but save
> us the special case in the wizard.

Hmm, perhaps that could work.

> By the way, for Google Talk, I think we will want to add "email address" as
> a username hint (that's way easier to do than adding an empty text in the
> focused username box).

That sounds like a reasonable compromise.

> > I see "JS Test" and "Override Test" chat networks.  I'm guessing they should
> > be removed.
> Except if I broke something recently, these are built only in debug builds,
> and never packaged. You were testing a local debug build, right?

Ah, yes, I was.  Nevermind, then.  :)

> > The error text for a selected account is dark red on a dark blue background.
> > It's really hard to read.
> Maybe Andreas can propose a CSS fix for this?

Sure.

> > Sending a message to my wife (who isn't logged in), I get:
> > JavaScript error: chrome://messenger/content/messenger.xul, line 137:
> > setColors is not defined
> This JS error isn't related to sending something to someone who isn't logged
> in. Any message bubble causes this error; it's a consequence of applying
> attachment 604280 [details] [diff] [review] which removed the setColors
> function, but not it's caller.

Okay, so it's fixed in the latest patch?

> > We shouldn't have the (Facebook, or any other) password box in the settings
> > dialog, I think.
> Hmm... I agree this is surprising, especially for Facebook/Gtalk. But for
> IRC (where the password is optional) we will never prompt for a password if
> there isn't one, so having it in the settings sounds right in that case.
> 
> If you want us to add a check to show the password box only for accounts
> that support a password but don't require it (so will never prompt), I think
> that's possible.

There aren't any password boxes in the other account settings.
I think people should be going to the Password Manager to set or change their passwords.

> > Do we still need the "purple.*" prefs?  I thought we were getting rid of
> > libpurple for some reason…
> When we rewrote the logging code in JS to replace the libpurple logger, we
> kept the existing preferences to avoid having to write migration code, so
> the logger actually uses these prefs.

Okay.

> > >+++ b/chat/locales/en-US/irc.properties
> > >+connection.quitting=Sending the QUIT message
> > This seems a little technical to me.  Could we just say "Quitting" instead?
> I was surprised by this message and searched for when it is displayed: it
> isn't! I've just removed it (and the localization note about it was just
> wrong, it's the result of a copy/paste from xmpp.properties).

Heh.  Even better!  :)

> > >+# LOCALIZATION NOTE (tooltip.*):
> > >+#    These are the descriptions given in a tooltip with information received
> > >+#    from a whois response.
> > >+#    The username is set by the user's IRC client, usually to the client's name
> > >+#    but the user can change it.
> > >+tooltip.user=Username
> > >+#    The host name that the user connects from (usually based on the
> > >+#    reverse DNS of the user's IP, but often mangled by the server to
> > >+#    protect users).
> > >+tooltip.host=Host name
> > >+#    The real name is a description of the user (including spaces).
> > >+tooltip.realname=Real name
> > If we're going to have "Host name" and "Real name", I think we should also
> > use "User name".
> I really dislike these strings. This tooltip is IMHO currently quite
> confusing and unfriendly. We started discussing replacing them in
> https://bugzilla.instantbird.org/show_bug.cgi?id=1293

Sure, but in the meantime, what's your plan here?  I would support removing it here, too, if that's your decision.

> > >+++ b/mail/base/content/mailWindowOverlay.xul
> > >@@ -412,16 +412,17 @@
> > >   <key id="key_collapseAllThreads" key="&collapseAllThreadsCmd.key;" oncommand="goDoCommand('cmd_collapseAllThreads')"/>
> > >   <key key="&collapseAllThreadsCmd.key;" modifiers="shift"           oncommand="goDoCommand('cmd_collapseAllThreads')"/>
> > >   <key id="key_nextUnreadThread" key="&nextUnreadThread.key;"        oncommand="goDoCommand('cmd_nextUnreadThread')"/>
> > >   <key id="key_previousMsg" key="&prevMsgCmd.key;"                   oncommand="goDoCommand('cmd_previousMsg')"/>
> > >   <key id="key_previousUnreadMsg" key="&prevUnreadMsgCmd.key;"       oncommand="goDoCommand('cmd_previousUnreadMsg')"/>
> > >   <key id="key_archive" key="&archiveMsgCmd.key;"                    oncommand="goDoCommand('cmd_archive')"/>
> > >   <key id="key_goForward" key="&goForwardCmd.commandKey;"            oncommand="goDoCommand('cmd_goForward')"/>
> > >   <key id="key_goBack" key="&goBackCmd.commandKey;"                  oncommand="goDoCommand('cmd_goBack')"/>
> > >+  <key id="key_goChat" key="&goChatCmd.commandKey;"                  modifiers="accel,shift"/>
> > no oncommand="goDoCommand('cmd_goChat')" ?
> There's no <command> for it, and I don't see how adding one would be useful
> here.

First, it would make it the same as the rest of the commands.  Second, having commmands lets us track things much easier in Test Pilot.  But I think we can push that change off to a later patch, since it's not user-facing.

> > >+chat.isTyping=is typingâ¦
> > >+chat.hasStoppedTyping=has stopped typing.
> > >+chat.contactIsTyping=%S is typing.
> > >+chat.contactHasStoppedTyping=%S has stopped typing.
> > Why do we have two of these?  What is chat.isTyping used for that couldn't
> > use chat.contactIsTyping instead?
> The isTyping and hasStoppedTyping strings are used instead of the contact's
> status message, so there's <username> with a big font on a first line, and
> then either "is typing" or "has stopped typing" displayed with a smaller
> font on a second line.
> The formatted variant is used for a tooltip displayed when hovering the
> status icon.

Ah, okay.

> > >+today=Today
> > >+yesterday=Yesterday
> > What do you think about adding "Last week" and "Last month", too?
> I'm not exactly sure of what you mean here. Are you talking about a "Last
> week" string (I don't know where it would be used exactly) or about special
> casing the conversations that happened during the last 7 days to show
> "Monday", "Tuesday", etc... instead of the date?
> In any case, I think that this feels like more work ;).

Yeah, maybe we leave it the way it is for now.

> > >+++ b/mail/locales/en-US/chrome/messenger/imAccountWizard.dtd
> > >@@ -0,0 +1,28 @@
> > >+<!ENTITY accountProtocolTitle.label   "Chat service">
> > >+<!ENTITY accountProtocolInfo.label    "Please choose the network of your chat account.">
> > >+<!ENTITY accountProtocolField.label   "Protocol:">
> > If we're saying "choose the network", shouldn't the field also be named
> > "Network", and the title be "Chat network"?
> The field should definitely not be named "protocol:" (I changed it to
> "Network:"). I'm not sure for the title, Chat service seems ok to me (it's
> what Jb suggested) but if you want to change it, that's ok with me too.

As long as we use the same words in both places, I'm happy with either "chat network" or "chat service".

> > >+++ b/mail/test/mozmill/migration/test-toolbar.js
> > I think I might put chat after address book…
> That's where I had it first. I recently moved to between the "Write" and the
> "Address book" button, as "Chat" seemed related to "Write" to me, as they
> are both about sending messages, a opposed to "Address book" which is more
> about consulting or editing locally stored data.
> Of course if you use the address book as the place to start writing your
> emails because you open the address book, select your contact, and then
> click the "Write" button of the address book toolbar, this point is moot.
> 
> If you really want it after the address book, I can change it; and will also
> need to change the toolbar migration code.

No, you've brought up a good point.  Let's leave it where it is.

Thanks,
Blake.
Attached patch icons v9 (obsolete) — — Splinter Review 
properly working mac dropdown icons now.
Attachment #604970 - Attachment is obsolete: true
Attached patch icons v10 (obsolete) — — Splinter Review 
removed unused junk
Attachment #604980 - Attachment is obsolete: true
Attached patch Patch v10 (obsolete) — — Splinter Review 
I addressed some review comments from comment 82 and some more from comment 75. I also included Andreas' attachment 604990 [details] [diff] [review].
Attachment #604280 - Attachment is obsolete: true
Attachment #604802 - Attachment is obsolete: true
Attachment #604990 - Attachment is obsolete: true
(In reply to Mike Conley (:mconley) from comment #82)
> Comment on attachment 604802 [details] [diff] [review]

> Hey Florian,
> 
> chat-messenger-overlay.js looks pretty reasonable.  Just a few comments,
> nothing major.

Thanks for the feedback. I'm going to reply only to questions and things that I haven't addressed.

> @@ +46,5 @@
> > +function buddyListContextMenu(aXulMenu) {
> > +  this.target  = document.popupNode;
> > +  this.menu    = aXulMenu;
> > +  let localName = this.target.localName;
> > +  this.onContact = localName == "imcontact";
> 
> Where are these magic strings imcontact / imconv used?

They are the tag names, defined in mail/components/im/content/chat.css.
I don't think making them consts would really clarify the code, and I'm running out of time, so I haven't addressed this comment.

> @@ +63,5 @@
> > +  document.getElementById("context-close-conversation").hidden = !this.onConv;
> > +  document.getElementById("context-openconversation").disabled =
> > +    !hide && !this.target.canOpenConversation();
> > +}
> > +buddyListContextMenu.prototype = {
> 
> Each of the functions in this object prototype needs documentation.

I really don't see what I could tell more than what the method name says :-/.

> @@ +90,5 @@
> > +    if (displayName != userName) {
> > +      displayName = bundle.getFormattedString("buddy.deletePrompt.displayName",
> > +                                              [displayName, userName]);
> > +    }
> > +    let proto = buddy.protocol.name; // FIXME build a list
> 
> I don't think I understand this FIXME - regardless, something in here
> probably needs fixing...

In Instantbird a contact (representing a person you can contact) can be composed of several buddies (for example one linked to your AIM account, one linked to your facebook account, etc...). Deleting a contact deletes the buddies for all accounts, so it would be good in this dialog to show that the contact will be removed from the contact list of all these accounts.
Thunderbird shares the same back-end so it's technically possible that a contact has several buddy, but there's currently absolutely no UI to group contacts, so this situation is not actually possible right now.

> @@ +228,5 @@
> > +    }
> > +
> > +    let status = target.getAttribute("status");
> > +    if (!status)
> > +      return; // is this really possible?
> 
> What's this comment all about?

If an add-on adds a menuitem in that menupopup that doesn't has the "status" attribute, the following code wouldn't work, so the function returns early if such a menuitem has been clicked. I rephrased the comment; hopefully it's clearer now.


> @@ +268,5 @@
> > +    let time = dts.FormatTime("", dts.timeFormatNoSeconds,
> > +                              aDate.getHours(), aDate.getMinutes(),0);
> > +    let bundle = document.getElementById("chatBundle");
> > +    if (aDate >= today)
> > +      return bundle.getString("today") + " " + time;
> 
> Would it not be better to have the time be embedded in the translation
> string, like %1$S, to give the localizers some more flexibility?

Probably good for rtl locales. I've also added a formatted string for the case where we just show the date and time; so that localizers can swap the order.

> @@ +382,5 @@
> > +      this.observedContact = null;
> > +      return;
> > +    }
> > +    if (item.getAttribute("id") == "searchResultConv") {
> > +      let path = "logs/" + item.log.path;
> 
> Should this path be hardcoded, or be a pref?

I don't see a use case of changing that path that wouldn't be addressed with a symbolic link; and anyway, it's something we can address in a follow-up if the feedback tells us this is really wanted.


> @@ +508,5 @@
> > +
> > +  _updateNoConvPlaceHolder: function() {
> > +    let connected = false;
> > +    let hasAccount = false;
> > +    for (let account in fixIterator(imServices.accounts.getAccounts())) {
> 
> Purely optional here - but how about we set hasAccount to true if the
> cardinality of imServices.accounts.getAccounts() > 0, and then use
> Array.some
> (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/
> some) to set "connected"?

getAccounts return an nsISimpleEnumerator, not an array.

> @@ +523,5 @@
> > +  },
> > +  observe: function(aSubject, aTopic, aData) {
> > +    if (aTopic == "browser-request") {
> > +      imServices.ww.openWindow(null,
> > +                               "chrome://chat/content/browserRequest.xul",
> 
> Hm.   So some dialogs are at chat/content/*, and others are at
> messenger/content/*?

chat/ is shared with Instantbird. messenger/ is Thunderbird specific.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #91)

> > > Sending a message to my wife (who isn't logged in), I get:
> > > JavaScript error: chrome://messenger/content/messenger.xul, line 137:
> > > setColors is not defined
> > This JS error isn't related to sending something to someone who isn't logged
> > in. Any message bubble causes this error; it's a consequence of applying
> > attachment 604280 [details] [diff] [review] which removed the setColors
> > function, but not it's caller.
> 
> Okay, so it's fixed in the latest patch?

Sure.

> > > We shouldn't have the (Facebook, or any other) password box in the settings
> > > dialog, I think.
> > Hmm... I agree this is surprising, especially for Facebook/Gtalk. But for
> > IRC (where the password is optional) we will never prompt for a password if
> > there isn't one, so having it in the settings sounds right in that case.
> > 
> > If you want us to add a check to show the password box only for accounts
> > that support a password but don't require it (so will never prompt), I think
> > that's possible.
> 
> There aren't any password boxes in the other account settings.
> I think people should be going to the Password Manager to set or change
> their passwords.

The password manager UI can only show or remove a password, not set it.

> > > >+# LOCALIZATION NOTE (tooltip.*):
> > > >+#    These are the descriptions given in a tooltip with information received
> > > >+#    from a whois response.
> > > >+#    The username is set by the user's IRC client, usually to the client's name
> > > >+#    but the user can change it.
> > > >+tooltip.user=Username
> > > >+#    The host name that the user connects from (usually based on the
> > > >+#    reverse DNS of the user's IP, but often mangled by the server to
> > > >+#    protect users).
> > > >+tooltip.host=Host name
> > > >+#    The real name is a description of the user (including spaces).
> > > >+tooltip.realname=Real name
> > > If we're going to have "Host name" and "Real name", I think we should also
> > > use "User name".
> > I really dislike these strings. This tooltip is IMHO currently quite
> > confusing and unfriendly. We started discussing replacing them in
> > https://bugzilla.instantbird.org/show_bug.cgi?id=1293
> 
> Sure, but in the meantime, what's your plan here?  I would support removing
> it here, too, if that's your decision.

I pressured the Instantbird developer who wrote JS-IRC (Patrick) to fix https://bugzilla.instantbird.org/show_bug.cgi?id=1293. We fixed it early this morning for Instantbird (although I haven't pushed the patch yet) and the fix is included in my latest attachment here.


> > > >+++ b/mail/locales/en-US/chrome/messenger/imAccountWizard.dtd
> > > >@@ -0,0 +1,28 @@
> > > >+<!ENTITY accountProtocolTitle.label   "Chat service">
> > > >+<!ENTITY accountProtocolInfo.label    "Please choose the network of your chat account.">
> > > >+<!ENTITY accountProtocolField.label   "Protocol:">
> > > If we're saying "choose the network", shouldn't the field also be named
> > > "Network", and the title be "Chat network"?
> > The field should definitely not be named "protocol:" (I changed it to
> > "Network:"). I'm not sure for the title, Chat service seems ok to me (it's
> > what Jb suggested) but if you want to change it, that's ok with me too.
> 
> As long as we use the same words in both places, I'm happy with either "chat
> network" or "chat service".

Ok, I changed "chat service" into "chat network".
(In reply to Florian Quèze from comment #96)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #91)
> > > > We shouldn't have the (Facebook, or any other) password box in the settings
> > > > dialog, I think.
> > > Hmm... I agree this is surprising, especially for Facebook/Gtalk. But for
> > > IRC (where the password is optional) we will never prompt for a password if
> > > there isn't one, so having it in the settings sounds right in that case.
> > There aren't any password boxes in the other account settings.
> > I think people should be going to the Password Manager to set or change
> > their passwords.
> The password manager UI can only show or remove a password, not set it.

Right, but we can pop up a password prompt to set the password…

In the case of IRC, wouldn't we get back an error message from NickServ (or its equivalent), if we don't have a password, but need one?

> > > If you want us to add a check to show the password box only for accounts
> > > that support a password but don't require it (so will never prompt), I think
> > > that's possible.

That might be better, although I still think having a password box in the settings isn't a good idea, for security reasons.  What do you think about a "Set Password" button which popped up the standard password prompt instead?

Thanks,
Blake.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #97)
> (In reply to Florian Quèze from comment #96)
> > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #91)
> > > > > We shouldn't have the (Facebook, or any other) password box in the settings
> > > > > dialog, I think.
> > > > Hmm... I agree this is surprising, especially for Facebook/Gtalk. But for
> > > > IRC (where the password is optional) we will never prompt for a password if
> > > > there isn't one, so having it in the settings sounds right in that case.
> > > There aren't any password boxes in the other account settings.
> > > I think people should be going to the Password Manager to set or change
> > > their passwords.
> > The password manager UI can only show or remove a password, not set it.
> 
> Right, but we can pop up a password prompt to set the password…
> 
> In the case of IRC, wouldn't we get back an error message from NickServ (or
> its equivalent), if we don't have a password, but need one?
There is no standardized way to interact with NickServ across different networks (and there aren't any RFCs for "IRC Services", etc. that I'm aware of). It's not actually part of the protocol, they're just bots running as part of the server. Also many networks have different names for their NickServ (and there are different sets of Services for these NickServs!)

Additionally, if you can parse that information...the NickServ still lets you on the network but will then forcefully change your nick if you wait too long to identify (at least that's the "standard" set up most people use...), the user would then type in their password, we'd try to auth to the NickServ and it would tell us that our nick isn't registered...I think this would be extremely confusing to the user. Note that right now we don't really interact with the NickServ at all, we abuse the PASS command that FreeNode and moznet support (and pass on to the NickServ).

Essentially, it isn't really a viable solution for IRC to not have the password before connection.
Florian mentioned we also use these icons in the Tools menu.
(In reply to Patrick Cloke [:clokep] from comment #98)
> There is no standardized way to interact with NickServ across different
> networks (and there aren't any RFCs for "IRC Services", etc. that I'm aware
> of). It's not actually part of the protocol, they're just bots running as
> part of the server. Also many networks have different names for their
> NickServ (and there are different sets of Services for these NickServs!)

Recently, several networks (primarily Freenode) and IRC daemons have started using SASL authentication as specified in the Charybdis ircd documentation:

<http://git.atheme.org/charybdis/tree/doc/sasl.txt>

This is the preferred method on networks which support it, and returns proper "authentication successful"/"failed" numerics; however, it is only usable during the "registration" stage, not after joining IRC.

Several other "CAP"abilities are documented in:

<http://git.atheme.org/charybdis/tree/doc/>
(In reply to Mantas M. (grawity) from comment #100)
> (In reply to Patrick Cloke [:clokep] from comment #98)
> > There is no standardized way to interact with NickServ across different
> > networks (and there aren't any RFCs for "IRC Services", etc. that I'm aware
> > of). It's not actually part of the protocol, they're just bots running as
> > part of the server. Also many networks have different names for their
> > NickServ (and there are different sets of Services for these NickServs!)
> 
> Recently, several networks (primarily Freenode) and IRC daemons have started
> using SASL authentication as specified in the Charybdis ircd documentation:
> 
> <http://git.atheme.org/charybdis/tree/doc/sasl.txt>
> 
> This is the preferred method on networks which support it, and returns
> proper "authentication successful"/"failed" numerics; however, it is only
> usable during the "registration" stage, not after joining IRC.
> 
> Several other "CAP"abilities are documented in:
> 
> <http://git.atheme.org/charybdis/tree/doc/>

Yes, I'm aware of this (and have pulled a bunch of documentation off there). But this is a different situation and not supported everywhere (also not implemented yet :-D). But I don't want to gum up this bug with off topic information. Email me (or catch me on IRC) if you want to talk about this more.
Attached patch status icons (all platforms) (obsolete) — — Splinter Review 
Also took care of the typo #button-join-chat[disabled] to #button-add-chat[disabled] Richard pointed out on IRC.
Attachment #605072 - Attachment is obsolete: true
Attached patch Patch v11 (obsolete) — — Splinter Review 
Takes into account Mark's review comments about l10n, and includes Andreas' attachment 605082 [details] [diff] [review].
Attachment #604997 - Attachment is obsolete: true
Attachment #605082 - Attachment is obsolete: true
Attachment #605153 - Flags: review?(mbanner)
Makes sure the contacts sidebar get styled correctly again.
>+++ b/chat/components/src/imAccounts.js
>@@ -0,0 +1,982 @@
>+UnknownProtocol.prototype = {
>+  // false seems an acceptable default for all options
>+  // (they should never be called anyway).

If they shouldn't be called, what about making them throw exceptions?

>+  get alias() {
>+    try {
>+      return this.prefBranch.getComplexValue(kPrefAccountAlias,
>+                                             Ci.nsISupportsString).data;
>+    } catch (e) {
>+      return "";

Should we log some sort of message here, too?

>+  _finishedAutoLogin: function() {
>+    if (!this.hasOwnProperty("_autoLoginPending"))
>+      return;
>+    delete this._autoLoginPending;

Why use this instead of setting this._autoLoginPending to false?

>+  connect: function() {
>+    if (this._passwordRequired) {
>+      // If the previous connection attempt failed because we have a wrong password,
>+      // clear the passwor cache so that if there's no password in the password

 Typo: Clear the _password_ cache.

>+  },
>+  createConversation: function(aName)
>+    this._ensurePrplAccount.createConversation(aName),
>+  addBuddy: function(aTag, aName) {

This seems like a good place to mention that we prefer to separate functions
with a blank line.  I'm not going to insist on it for functions that are just
simple expressions, but if there are {}s, we should probably have the newlines
before and after.

>+AccountsService.prototype = {
>+  initAccounts: function() {
[snip…]
>+      } catch (e) {
>+        Cu.reportError(e);
>+        dump(e + " " + e.toSource() + "\n");

We prefer not to use dump statements, since the reportError should be good
enough.

>+      /*dump("autoLoginPending = " + autoLoginPending +
>+             ", lastCrash = " + lastCrashTime +
>+             ", difference = " + lastCrashTime - autoLoginPending + "\n");*/

Please just remove this, or convert it into a log message.

>+  getAccountById: function(aAccountId) {
>+    if (aAccountId.indexOf(kAccountKeyPrefix) != 0)
>+      throw Cr.NS_ERROR_INVALID_ARG;
>+
>+    let id = parseInt(aAccountId.substr(kAccountKeyPrefix.length));
>+    return this.getAccountByNumericId(id);

I thought I saw code similar to this up above.  Any chance of having that code
just call this function instead?

>+      Services.console.logStringMessage("No account " + id + " but there is some data in the buddy list for an account with this number. Your profile may be corrupted.");

This line is way longer than 80 characters.  Let's break it up into a couple
of strings instead.

>+++ b/chat/components/src/imCommands.js
>@@ -0,0 +1,254 @@

Nothing to see here, but that I reviewed this file.

>+++ b/chat/components/src/imContacts.js
>@@ -0,0 +1,1417 @@
>+// TODO move into the tagsService
>+var Tags = [];
>+var TagsById = { };

How hard would it be to do that?

>+var otherContactsTag = {
>+  hiddenTagsPref: "messenger.buddies.hiddenTags",
>+  _hiddenTags: {},

Why is this called "otherContactsTag" when it seems to be all about hidden tags?

>+++ b/chat/components/src/imConversations.js
>@@ -0,0 +1,470 @@
>+  observeConv: function(aTargetId, aSubject, aTopic, aData) {
>+    if (aTargetId != this._currentTargetId &&
>+        (aTopic == "new-text" ||
>+         (aTopic == "update-typing" &&
>+          this._purpleConv[aTargetId].typingState == Ci.prplIConvIM.TYPING)))

I don't think the extra indentation is really necessary here, and makes it
more confusing for me.

>+++ b/chat/components/src/imCore.js
>@@ -0,0 +1,384 @@
>+    } catch (e) {
>+      // This is a real error, the protocol is registered and failed to init.
>+      let error = "failed to create an instance of " + cid + ": " + e;
>+      dump(error + "\n");
>+      Cu.reportError(error);

So, I mentioned removing the dump previously, but I wonder if it would also be
worth passing in an exception to Cu.reportError, to get a better error message?

>+++ b/chat/content/browserRequest.js
>@@ -0,0 +1,175 @@
>+  QueryInterface: function(aIID) {
>+    if (aIID.equals(Components.interfaces.nsIWebProgressListener)   ||
>+        aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
>+        aIID.equals(Components.interfaces.nsISupports))
>+      return this;
>+    throw Components.results.NS_NOINTERFACE;
>+  },

I think we want to use the XPCOMUtils.generateQI function here, if only so that
code that gets copied and pasted from here also gets to use it.

>+++ b/chat/content/browserRequest.xul
>@@ -0,0 +1,75 @@
>+<window id="browserRequest"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        buttons=","
>+        onload="loadRequestedUrl()"
>+        onclose="reportUserClosed()"
>+        title=""
>+        width="800"
>+        height="500"

I think we might want to make these localizable.  Perhaps not, though.

>+++ b/chat/content/convbrowser.xml
>@@ -0,0 +1,1036 @@
>+      <property name="autoscrollEnabled">
>+        <getter>
>+          <![CDATA[
[snip…]
>+            }
>+            catch(ex) {
>+            }

_That_ doesn't seem right…  ;)

>+      <method name="startScroll">
>+        <parameter name="event"/>
>+        <body>
>+          <![CDATA[
[snip…]
>+              // Enable translucency on Windows and Mac
>+              this._autoScrollPopup.setAttribute("translucent", /Win|Mac/.test(navigator.platform));

Not Linux?

>+      <handler event="keypress" modifiers="control" keycode="VK_HOME"
>+               action="this.scrollToPreviousSection(); event.preventDefault();"/>
>+
>+      <handler event="keypress" modifiers="control" keycode="VK_END"
>+               action="this.scrollToNextSection(); event.preventDefault();"/>

I think these should be "accel" instead of "control", so that Mac users can use them too.

>+++ b/chat/modules/imServices.jsm
Looks good to me!
>+++ b/chat/modules/imSmileys.jsm
>@@ -0,0 +1,249 @@
>+function getRegexp()
>+{
[snip…]
>+  let exp = /([\][)(\\|?^$*+])/g;
>+  emoticonList = emoticonList.sort()
>+                             .reverse()
>+                             .map(function(x) x.replace(exp, "\\$1"));

Uh, what does "exp" search for?

>+++ b/chat/modules/imStatusUtils.jsm
Nothing to see here.
>+++ b/chat/modules/imTextboxUtils.jsm
This one seems okay, too.
>+++ b/chat/modules/imThemes.jsm
>@@ -0,0 +1,1037 @@
>+const DEFAULT_THEMES = ["bubbles", "dark", "papersheets", "simple"];

Wait, do we have multiple themes?  If not, can we remove this file, or at least
most of this file?  :)

>+function getChromeBaseURI(aThemeName)
>+{
>+  if (DEFAULT_THEMES.indexOf(aThemeName) != -1)
>+    return "chrome://instantbird-messagestyles/skin/" + aThemeName + "/";

Is this the location of the styles in Thunderbird?

>+++ b/chat/protocols/facebook/facebook.js
Seems okay to me.
>+++ b/chat/protocols/gtalk/gtalk.js
Ditto.
>+++ b/chat/protocols/irc/irc.js
I'm happy with this.
>+++ b/chat/protocols/twitter/twitter.js
>@@ -0,0 +1,1022 @@
>+  consumerKey: "TSuyS1ieRAkB3qWv8yyEw",
>+  consumerSecret: "DKtKaSf5a7pBNhdBsSZHTnI5Y03hRlPFYWmb4xXBlkU",

I'm a little surprised to see these in here…

>+++ b/chat/protocols/xmpp/xmpp.jsm
>@@ -0,0 +1,1045 @@
>+/* Helper class for buddies */
>+const XMPPAccountBuddyPrototype = {
>+  /* Returns a list of TooltipInfo objects to be displayed when the user hovers over the buddy */
>+  getTooltipInfo: function() {

Why would we want to display more than one TooltipInfo?

>+    else {
>+      let s = Stanza.iq("set", null, null,
>+                        Stanza.node("query", Stanza.NS.roster, null,
>+                                    Stanza.node("item", null, {jid: jid},
>+                                                Stanza.node("group", null, null,
>+                                                            aTag.name))));

I think using a two-space indent here might work out better.

>+++ b/chat/themes/browserRequest.css
I like it.
>+++ b/chat/themes/conv.css
Seems okay.
>+++ b/mail/base/content/folderPane.js
Makes sense.
>+++ b/mail/base/content/glodaFacetBindings.xml
This seems okay, but it would be good to check with protz or asuth.
>+++ b/mail/base/content/glodaFacetView.css
>@@ -201,17 +201,17 @@ h1, h2, h3 {
> .facetious[uninitialized] {
>-  display: none;
>+  display: none !important;

Why do you need the "!important"?

>+++ b/mail/base/content/glodaFacetView.js
Seems reasonable.
>+++ b/mail/base/content/mailCore.js
Okay by me.
>+++ b/mail/base/content/mailWindowOverlay.xul
Looks good.
>+++ b/mail/components/im/content/addbuddy.js
Sure, seems reasonable.
>+++ b/mail/components/im/content/addbuddy.xul
Yep.
>+++ b/mail/components/im/content/am-im.js
Okay.
>+++ b/mail/components/im/content/am-im.xul
>@@ -0,0 +1,86 @@
>+  width  = "&accountWindow.width;"
>+  height = "400"

Why is the width localizable, but not the height?

>+++ b/mail/components/im/content/chat-messenger-overlay.xul
This seems alright.
>+++ b/mail/components/im/content/chat.css
>@@ -0,0 +1,108 @@
>+/*
>+.conv-chat {
>+  visibility: collapse; /* FIXME!* /
>+}
>+*/

Please delete this instead of commenting it out.

>+++ b/mail/components/im/content/imAccount.xml
It's big but I didn't see anything wrong with it.
>+++ b/mail/components/im/content/imAccountWizard.js
Nothing here.
>+++ b/mail/components/im/content/imAccountWizard.xul
This is kinda neat.
>+++ b/mail/components/im/content/imAccounts.css
Looks good.
>+++ b/mail/components/im/content/imAccounts.js
Yeah, I'm kinda glossing over these files by now.
>+++ b/mail/components/im/content/imAccounts.xul
Didn't see anything wrong here.
>+++ b/mail/components/im/content/imContextMenu.js
This one is okay, too.
>+++ b/mail/components/im/content/imStatusSelector.js
Nothing here.
>+++ b/mail/components/im/content/imbuddytooltip.xml
Fine.
>+++ b/mail/components/im/content/imcontact.xml
It would be surprising if I found anything in this file.
>+++ b/mail/components/im/content/imconv.xml
Or this one.
>+++ b/mail/components/im/content/imconversation.xml
>@@ -0,0 +1,1489 @@
>+/*
>+        if (aMsg.incoming && !aMsg.system &&
>+            (!aMsg.conversation.isChat || aMsg.containsNick) &&
>+            Services.prefs.getBoolPref("messenger.options.getAttentionOnNewMessages"))
>+          window.getAttention();
>+*/

I think we can just delete this block.

>+++ b/mail/components/im/content/imgroup.xml
Seems good.
>+++ b/mail/components/im/content/imsearch.xml
>@@ -0,0 +1,258 @@
>+  <!--
>+    - The glodaSearch binding implements a gloda-backed search mechanism.  The
>+    -  actual search logic comes from the glodaFacet tab mode in the
>+    -  glodaFacetTabType definition.  This binding serves as a means to display
>+    -  and alter the current search query if a "glodaFacet" tab is displayed,
>+    -  or enter a search query and spawn a new "glodaFacet" tab if one is
>+    -  currently not displayed.
>+    -
>+    - This widget used to have many weird implementation nuances.  Now we are
>+    -  just a little bit of extra stuff on top of the toolkit autocomplete
>+    -  implementation.  Our deviations are:
>+    -  - We collapse ourselves when gloda is disabled; we track the state.
>+    -  -
>+    -->

Something's gone a little weird here, since I don't get the autocomplete dropdown.
And it doesn't use the new "Search…" text…

>+++ b/mail/components/im/content/joinchat.js
I didn't see anything here.
>+++ b/mail/components/im/content/joinchat.xul
Looks fine.
>+++ b/mail/components/im/messages/Footer.html
I like the way you're doing this in code in content.
>+++ b/mail/components/im/messages/Incoming/Content.html
>+++ b/mail/components/im/messages/Incoming/Context.html
>+++ b/mail/components/im/messages/Incoming/NextContent.html
These are all good.
>+++ b/mail/components/im/messages/NextStatus.html
>@@ -0,0 +1,3 @@
>+<p class="%messageClasses%">%time% - %message%</p>

I'm a little worried about this not translating well for rtl languages.
(It appears in other places, too.)

>+++ b/mail/components/im/messages/Status.html
>+++ b/mail/components/im/messages/Variants/Blue_-_Red_Alternating.css
>+++ b/mail/components/im/messages/bubbles.svg
>+++ b/mail/components/im/messages/main.css
>+++ b/mail/components/im/smileys/theme.js
>+++ b/mail/components/im/themes/chat.css
>+++ b/mail/components/im/themes/imAccountWizard.css
These all seem fine.
>+++ b/mail/components/im/themes/imAccounts.css
>@@ -0,0 +1,381 @@
>+#bottombuttons button:-moz-window-inactive {
>+  color: @toolbarbuttonInactiveFontColor@ !important; /* remove this when we support click-through, bug 392188 */

I think we can put the comment on the line above.

>+++ b/mail/components/im/themes/imBuddytooltip.css
>+++ b/mail/components/im/themes/imMenulist.css
>+++ b/mail/components/im/themes/imRichlistbox.css
>+++ b/mail/components/im/themes/imStatus.css
>+++ b/mail/components/preferences/chat.js
>+++ b/mail/components/preferences/chat.xul
>+++ b/mail/components/preferences/preferences.xul
>+++ b/mail/themes/gnomestripe/mail/chat.css
>+++ b/mail/themes/gnomestripe/mail/primaryToolbar.css
>+++ b/mail/themes/pinstripe/mail/chat.css
>+++ b/mail/themes/pinstripe/mail/preferences/preferences.css
>+++ b/mail/themes/qute/mail/chat.css
 These are fine, too.
>+++ b/mail/themes/qute/mail/preferences/preferences.css
>@@ -74,16 +74,19 @@ radio[pane=paneGeneral] {
>+radio[pane=paneChat] {
>+  list-style-image: url("chrome://messenger/skin/preferences/composition.png"); /*FIXME*/
>+}

Uh, fix this?  :)

>+++ b/mail/themes/qute/mail/primaryToolbar-aero.css
>+++ b/mail/themes/qute/mail/primaryToolbar.css
These two are fine.
>+++ b/mailnews/base/prefs/content/AccountManager.js
>@@ -862,23 +870,23 @@ function restorePage(pageId, account)
>+  if ("onPreInit" in top.frames["contentFrame"])
>+    top.frames["contentFrame"].onPreInit(account, accountValues);
>+
>   var pageElements = getPageFormElements();
>   if (!pageElements) 
>     return;
> 
>-  if ("onPreInit" in top.frames["contentFrame"])
>-    top.frames["contentFrame"].onPreInit(account, accountValues);
>-

Why was this change made?

>@@ -1111,42 +1119,45 @@ var gAccountTree = {
>       // Everyone except news and RSS has a junk panel
>       // XXX: unextensible!
>-      if (server.type != "nntp" && server.type != "rss")
>+      if (server.type != "nntp" && server.type != "rss" && server.type != "im")

You should probably update the comment, too.

>+++ b/mailnews/base/prefs/content/AccountManager.xul
No problems here.
>+++ b/mailnews/base/util/errUtils.js
>@@ -115,16 +115,18 @@ Stringifier.prototype = {
>   dumpException: function(exc, message) {
>+    dump(exc + "\n");
>+    Components.utils.reportError(exc);

I'll let this dump slide, cause it's in a function named "dumpException".  :)

>+++ b/mailnews/extensions/mdn/src/mdn-service.js
>+++ b/mailnews/extensions/smime/src/smime-service.js
And finally these two are good.

Okay, so there was a bunch of stuff there, but none of it is string-changing, so
I think I'm good with landing it, and fixing the things in a future patch.

(The biggest thing I'm concerned about at this point is the gloda stuff, but I
 believe you can use the normal gloda strings, and so the fix for that won't
 involve new strings.)

So, r=me for checkin.

Thanks,
Blake.
Attached patch cleanup of unused im theme bits (obsolete) — — Splinter Review 
Removes unused bits of the chat message theme.
Attached patch Patch v12 (obsolete) — — Splinter Review 

        Attachment #587848 -
        Attachment is obsolete: true

        Attachment #587869 -
        Attachment is obsolete: true

        Attachment #605153 -
        Attachment is obsolete: true

        Attachment #605166 -
        Attachment is obsolete: true

        Attachment #605199 -
        Attachment is obsolete: true

        Attachment #605153 -
        Flags: review?(mbanner)

        Attachment #605238 -
        Flags: review?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v12 unbitrotted (obsolete)
        — 
          — Splinter Review
      

    


  
My previous patch has already bitrotted with the landing of BigFiles (bug 698925).

        Attachment #605238 -
        Attachment is obsolete: true

        Attachment #605238 -
        Flags: review?

    
    

    
  


  
(attaching something that isn't completely unrelated this time :))

        Attachment #605250 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 605251 [details] [diff] [review]
Patch v12 unbitrotted

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

r=me for the L10n & L10n-related build config changes. Also r=me for the general build config changes on the assumption we'll look to tidy those up in a follow-up bug.

        Attachment #605251 -
        Flags: review+

    
    

    
  
http://hg.mozilla.org/comm-central/rev/8cdfed92867f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0

    
    

    
  
Follow-up to fix sending the & character on IRC:
http://hg.mozilla.org/comm-central/rev/799cff6215ae

    
    

    
  
(In reply to Mark Banner (:standard8) from comment #110)

> r=me for the L10n & L10n-related build config changes. Also r=me for the
> general build config changes on the assumption we'll look to tidy those up
> in a follow-up bug.

Did you guys create that bug last night ?

We have a shinny Thunderbird/Instant messaging component.

    
  
Component: General → Instant Messaging
QA Contact: general → instant-messaging
Depends on: 735200

    
  
Depends on: 735272
Depends on: 735217
Depends on: 735286

    
  
Depends on: 735292

    
  
Depends on: 735301
Depends on: 735299

    
  
Depends on: 735334

    
  
Depends on: 735366

    
  
Depends on: 735379

    
    

    
  
(In reply to Jonathan Protzenko [:protz] from comment #88)
> There are several occurrences (at least 10 of them) of synchronous SQL being
> performed in your code, including statements that write to the database. The
> Tag service and the Contacts object seem to be designed to work in a
> synchronous fashion. While I understand that it may be painful to rewrite
> your code to work in an asynchronous fashion, I do think that such cleanups
> must be performed before landing. With a lot of database activity occurring
> in Thunderbird already, esp. with Gloda doing its indexing and whatnot, if
> your synchronous statement happens to trigger an fsync() of the underlying
> filesystem, and if I'm not mistaken, the UI is liable to lock up for several
> minutes in the worst case. I don't think we want to have that happening in
> Thunderbird.
> 
> I mentioned this several times already; I understand my comments may not
> have much value, since I do not have the "performance expert" hat :). Taras,
> Yoric, could you please chime in and tell us if it's okay landing code that
> performs synchronous IO? I may be exaggerating the situation, so I'd be
> happy if you could share your thoughts on performing synchronous calls to
> mozStorage on the main thread :).

Bug 699854 comment 13 seems relevant here.  But the (moot?) answer to your question is yes, sync main thread IO should be avoided at all costs.

    
    

    
  
(In reply to Nathan Froyd (:froydnj) from comment #115)
> Bug 699854 comment 13 seems relevant here.  But the (moot?) answer to your
> question is yes, sync main thread IO should be avoided at all costs.

Definitely. It is guaranteed to cause issues, either now or later.

    
  
Depends on: 735718

    
  
Depends on: 735721

    
  
Depends on: 735723

    
    

    
  
I filed bug 735718 to track and fix the synchronous I/O issue, bug 735721 to track Blake's review comments (comment 105) and bug 735723 to handle Mark's build-system review comments (comment 87).

Let's stop discussing things in this closed bug :).
Please file new bugs in the Thunderbird/Instant Messaging component for any issue that we need to take care of. Thanks!

    
  
Depends on: 736035

    
    

    
  
(In reply to Florian Quèze from comment #1)
> Some known issues:

> - Sometimes the "Account Settings..." dialog loses track of *all* IM
> accounts. The accounts are still listed in the "Instant messaging accounts
> status" dialog though. I've seen this only once, but I know Jb has also
> suffered from this problem. We do not know the steps to reproduce so I
> currently can't fix it, if anybody can find steps to reproduce, that would
> be awesome.

Filed bug 736035 on this.

> - this build will identify to the Twitter server as "Instantbird".

This is bug 735642.
Depends on: 735642

    
    

    
  
Why the lastest nightly build of Thunderbird doesn't support other XMPP servers than Google or FaceBook?
I use the Fritalk XMPP server, (fritalk.com & fritalk.org) and want to use it with Thunderbird.

Please, think about that! NOT EVERYONE IS USING GMAIL OR FACEBOOK!

    
  
Depends on: 739258

    
  
Depends on: 740793
Depends on: 739585, 739181

    
    

    
  
The feature page on the wiki indicates that status is "feedback wanted". Should we post them here?

Anyway, I do agree with Vivian, I was glad to be able to use it with my personal XMPP server but presently I'm stuck.

Two other things lack for me : a panel based overview of the chat and notifications when someone talk to you.

The last important point but not really essential, is a more clear integration into account preferences.


Anyway, bravo Florian! Tb avance dans le bon sens :) .
Depends on: 737497
Depends on: 743224
Whiteboard: [secr:ptheriault] → [sec-assigned:ptheriault]

    
    

    
  
While this notifies the user of a new message by adding "(1)" to the chat tab bar, we need to add the options to turn the minimized Window orange on new chat and show alert preview messages like with emails.

    
    

    
  
Really, just the same notification options as instantbird.
Flags: sec-review?(ptheriault)

    
    

    
  
I created a bug for better notifications, but I'm just never sure who sees it: #787455

    
    

    
  
It is critical for this feature to actually be useful / be chosen over dedicated chat clients for it to have better notifications.  Anyone?

    
    

    
  
(In reply to Brad from comment #124)
> It is critical for this feature to actually be useful / be chosen over
> dedicated chat clients for it to have better notifications.  Anyone?

This was the original bug for integration, please file other bugs in the Instant Messaging (I see you filed bug 787455) for other issues.
Flags: sec-review?(ptheriault) → sec-review+
Whiteboard: [sec-assigned:ptheriault]

    
  
Blocks: 1015774

    
    

    
  
Perhaps I am not seeing it, but after connecting to an XMPP chat server and joining a chat room, when first set up you can select to 'Auto-join this Chat Room' however I'm not able to find any way to disable auto-joining for specific chat rooms. Is this a bug, or am I missing something?

    
    

    
  
(In reply to Braden Beer from comment #126)
> Perhaps I am not seeing it, but after connecting to an XMPP chat server and
> joining a chat room, when first set up you can select to 'Auto-join this
> Chat Room' however I'm not able to find any way to disable auto-joining for
> specific chat rooms. Is this a bug, or am I missing something?

Click on "Show accounts" and then "Properties" for that XMPP account. The list of auto-joined rooms can be changed there.

Note for the future: Please don't add comments to old bugs that have been resolved - instead, just file a new bug.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: