I'm using this bug to track/implement the core mailnews changes required for moving from wallet to login manager.

This bug doesn't need to block TB 3.0, bug 239131 already covers blocking TB 3.0 and this bug blocks that, so there is no need to have two bugs blocking for the same thing.

Retriaging according to new policy for flags. https://wiki.mozilla.org/Thunderbird:Release_Driving (bugs marked wanted- don't indicate we wouldn't accept patches, but that they're not going to be the focus for release drivers)

Moving to blocking, since this has to be in sync with bug 239131. It's possible that both this and that bug could be demoted to wanted in the future; need input from Standard8 on that.

This work may make beta 1 but the strings aren't (see bug 239131) therefore moving out the milestone to beta 2.

I'm starting to look into this in more detail now. First some unit tests based on the existing password manager system. These just do basic auth most of the time but that's enough to verify that we're loading signons.txt (if that file is missing, we fail the test because we try and create a prompt window). I had to extend the news & pop3 fake servers for these tests. A lot of it is copy and paste, and I think we need to look at making the structure for doing this sort of test simpler and make better use of re-using code, however that's for a different bug.

Comment on attachment 344067 [details] [diff] [review] [checked in] Implement unit tests for existing password manager (SMTP/POP3/NNTP) very nice

Comment on attachment 344067 [details] [diff] [review] [checked in] Implement unit tests for existing password manager (SMTP/POP3/NNTP) Checked in: http://hg.mozilla.org/comm-central/rev/84d8ea6a0561

With this patch (and the appropriate build changes) I think Pop3 & Imap will work for at least non-ssl connections (maybe ssl, you never know!). It won't work if before you migrate you passwords file you have changed your username or hostname on an account. In our current wallet days, we store the original hostname & username in an account. I want to be able to store our current hostname & username, as this is more logical and will mean we don't get bugs if/when people change things. To get around this, I think we're going to need some sort of migration/upgrade routine. There's various options: 1) Have a hook from toolkit's upgrade routine that mailnews can listen to and do the appropriate upgrades (note, switching to password manager also triggers a password storage upgrade, which will give us this notification). 2) Have a preference in mailnews that we examine to fire the upgrades. 3) When we go to find a password, look for the ones with the current real username/hostname and if not found, check the original username/hostname combination, if not, prompt. -- We could improve this slightly by if we find the original combination, we upgrade it to the current combination. I don't think any of these options are ideal, but I really think we should be using the real hostname/username in password manager and whilst we are doing the conversion, it would be a good time to fix this. Thoughts?

I believe the reason we use the original hostname and username is the following scenario: I have a work account - the IT department decides to move my account to a different mail server. This means my host name changes, but my old password and username still work. As a user, I just have to change the host name in my account settings. (With MCD, this can all be transparent to me, but since not many folks use it, it's not so relevant). Maybe server infrastructure has gotten more sophisticated so that accounts are usually moved w/o changing server names, or maybe the advantages of storing the real name are more important. I think I prefer option 1. Option 3 would be a last resort.

(In reply to comment #8) > In our current wallet days, we store the original hostname & username in an > account. I want to be able to store our current hostname & username What's stored in wallet? Just the original hostname? (And the mailnews code knows to look for oldhost.com logins when it's talking to newhost.com?) > 1) Have a hook from toolkit's upgrade routine that mailnews can listen to and > do the appropriate upgrades (note, switching to password manager also triggers > a password storage upgrade, which will give us this notification). This might be a bit tricky, because the (1) mozStorage based backed defers importing logins until someone asks it for password data and (2) the notification would while while the mozStorage backend is still initializing, so an observer couldn't immediately do anything. Not saying it's impossible, but might require a bit more work than it would first sound like. > 2) Have a preference in mailnews that we examine to fire the upgrades. I'd tend to think that this would be the simplest approach and a bit cleaner (in that password manager shouldn't have to be involved in the caller's semantics). This could be a per-account flag, specifying if the login has been migrated to the new hostname (new accounts would set this by default). Then you'd have some code that looks like: if (no_pref_set) { oldLogin = getMatchingLogin(oldhost, user, pass); newLogin = clone(oldLogin); newLogin.hostname = newhost; pwmgr.modifyLogin(oldLogin, newLogin); setPref() } (In reply to comment #9) > I believe the reason we use the original hostname and username [...] Presumably mailnews would now also use pwmgr.modifyLogin() to change the hostname when the user edits the hostname in this scenario.

>What's stored in wallet? Just the original hostname? (And the mailnews code >knows to look for oldhost.com logins when it's talking to newhost.com?) Right. Each account knows the original host name and the current actual host name. >I'd tend to think that this would be the simplest approach and a bit cleaner >(in that password manager shouldn't have to be involved in the caller's >semantics). I'm probably confused, but it sounds more like the problem is that the mailnews caller needs to be aware of the password manager internals :-) I had hoped to avoid #2 because of the pref pollution it causes. If possible, it would be better to have just a single pref, and migrate all our account passwords to the new mechanism at the same time...

(In reply to comment #11) > I'm probably confused, but it sounds more like the problem is that the mailnews > caller needs to be aware of the password manager internals :-) Heh. :) The point is just that the semantic mapping between oldhost.com and newhost.com is entirely within mailnews. Password manager exposes a modifyLogin() API for making a change like this, but otherwise doesn't have any reason to care that the two are somehow related. IIRC, the need to change this is a side effect of cleaning up the prompting code, so that the old hacks for who fills in data and for what host are done more sensibly. Bug 382437 has the details.

(In reply to comment #9) > I believe the reason we use the original hostname and username is the following > scenario: > > I have a work account - the IT department decides to move my account to a > different mail server. This means my host name changes, but my old password and > username still work. As a user, I just have to change the host name in my > account settings. (With MCD, this can all be transparent to me, but since not > many folks use it, it's not so relevant). Originally I hadn't considered this scenario. I think it is a much reduced case now, especially as most IT department will probably rarely change mail server names these days (more likely the ip addresses behind them). I also think that to the user who has changed their account details, and looks in password manager, then it is confusing as it would display the old details - and this could be the more likely scenario. (In reply to comment #12) > (In reply to comment #11) > > > I'm probably confused, but it sounds more like the problem is that the mailnews > > caller needs to be aware of the password manager internals :-) > > Heh. :) The point is just that the semantic mapping between oldhost.com and > newhost.com is entirely within mailnews. Password manager exposes a > modifyLogin() API for making a change like this, but otherwise doesn't have any > reason to care that the two are somehow related. We just need mailnews to be aware of which version of password manager it is using and if it just upgraded ;-) > IIRC, the need to change this is a side effect of cleaning up the prompting > code, so that the old hacks for who fills in data and for what host are done > more sensibly. Bug 382437 has the details. Actually, we could still use the oldhost.com / oldusername data for password maanger even with those changes. I'm just saying that I think now would be an ideal time to switch to newhost/newusername considering the switch to the new password manager anyway, and hence trying to find a reasonable solution.

Justin, I'm requesting review on this patch more to get you to look at it as soon as possible than for a patch that's ready for review. This patch will send round a observer notification (passwords-upgraded) at the appropriate time. It will include the version of the file we upgraded from (just in case anyone needs to track that). It works by recording the version on legacy import, then if it has been imported and a version is set, the mozStorage version will send out the observer. I've also only added an attribute to ensure add-on compatibility so we can hopefully get this into the 1.9.1 branch. The more I think about it the more I'm coming to the conclusion that the notification has to come from within the password manager, or some intermediate proxy. We could, for example, migrate passwords on startup (pref controlled), but we would need to be hooking onto final-ui-startup, and if the user didn't let us know the master password, we couldn't migrate them. So, the next choice would be whenever we request a password from the password manager - I have noted at least 6 locations in mailnews where we are calling PromptPassword, and expect a few more by the time I look into PromptUsernamePassword. This will give us lots of places to have to apply hooks, and you can bet that at some stage one will be forgotten and mess everything up. We'd also need individual upgrade prefs for each account on this. This leaves me to think that the best time is still when we upgrade the file in the password manager. I've only done limited testing so far and I'm just going to look at actually doing some of the upgrade, but I think this would be a much better way to go.

(In reply to comment #14) > This patch will send round a observer notification (passwords-upgraded) If there's no other way to handle this, I can probably convince myself to live with this. With name more obvious to its purpose ("passwords-imported-omg-mailnews-hack" or something :-). This avoids confusion with internal schema version updates. [Eg, bug 467463] What happens if you crash after receiving the notification, but before you've completed fixing up the user's accounts? > It will include the version of the file we upgraded from > (just in case anyone needs to track that). Do you really need this? I'd rather not add just-in-case interface changes, and I really don't want to expose back-end version details. This gets to be a huge pain if consumers have to be aware of all the possible combinations (add in Gnome/OS X backends, etc). > We could, for example, migrate passwords on startup (pref controlled), but we > would need to be hooking onto final-ui-startup, and if the user didn't let us > know the master password, we couldn't migrate them. I hope you're not relying on the master password having been entered when importing, because that's going to be undone in bug 451267. You'll still have to deal with the user not entering the MP.

(In reply to comment #15) > > We could, for example, migrate passwords on startup (pref controlled), but we > > would need to be hooking onto final-ui-startup, and if the user didn't let us > > know the master password, we couldn't migrate them. > > I hope you're not relying on the master password having been entered when > importing, because that's going to be undone in bug 451267. You'll still have > to deal with the user not entering the MP. I'm hoping this won't break the existing import code we've got in password manager for mailnews style logins (I'm not sure we've got tests for those where they are encrypted. At the moment I'm now thinking of dropping the idea of migrating these passwords to their 'current' values, and just using what the account was created with. There is just nowhere that we can channel all requests through easily and have our own code handle things appropriately. The only other option I can see would be to go to per-account prefs, or have a fallback routine i.e. if the 'current' values weren't found, try the old ones. Both of which I'd have preferred to avoid. Still, as these appear to be the only real options, I can ship that work out to bug 302388 and just migrate password manager using the original account values.

This is the mozilla-* patch that will enable toolkit password manager for us.

This is a substantially updated and extended patch: - To apply you need the patches from the bugs this depends on (LDAP, SMTP, password manager news migration fix). - Also the "Toolkit build config change" patch. - See the bugs that this one blocks for UI patches (these are just the preferences display). Notes: - It should leave signons.txt alone, but back up anyway. Once migration has occurred, it won't migrate again (although you can delete signons.sqlite). - I believe that POP3/IMAP/SMTP should all be working fine. There is an issue with POP3 and the tests in bug 470973, but I think that is a test issue, not a mailnews backend issue. - News will be flakey if it wants to forget logins - that probably won't work correctly. I still need to fix the final bits on that. Any testing would be useful, especially with SSL or non-standard ports - I think I've covered those options in the unit tests and my own tests, but still would be useful.

Comment on attachment 354346 [details] [diff] [review] WIP v2 >diff --git a/mailnews/base/util/Makefile.in b/mailnews/base/util/Makefile.in >--- a/mailnews/base/util/Makefile.in >+++ b/mailnews/base/util/Makefile.in >@@ -84,6 +84,7 @@ REQUIRES = xpcom \ > htmlparser \ > content \ > layout \ >+ loginMgr \ > $(NULL) This needs to be a lowercase m to make it actually compile.

(In reply to comment #19) > This needs to be a lowercase m to make it actually compile. ...on certain platforms ;-) Mac didn't seem to complain at that. I also forgot to mention, this patch doesn't fix the password protect cache hidden option code - for that I need to add some more protocol handlers (for the 'xmailbox', 'ximap' etc. protocols), and possibly adjust the password migration code in the password manager again.

See comment 18 for main notes, improvements in this patch: - fixed compilation issue - newsgroup usernames and passwords should now be forgotten correctly when instructed at the code level. Other notes: - expect newsgroup ssl logins not to work - the previously noted failures about the tests in bug 470973 failing have been resolved as a test case problem rather than a problem with this patch. - Bug 470439 has landed on mozilla-central, currently waiting approval to land it on mozilla-1.9.1

I wasn't expecting to do this today, however this patch fixes: - newsgroup SSL logins - newsgroup username prompt (string change). I believe all that is left to do now is to sort out the password protect cache code that I think we still have to support.

re password protect cache code, I think associating it with the master password is an idea worth exploring - perhaps not showing any folder contents until the master password is entered, if the master password is set, would be sufficient. Then we could get rid of the hidden pref and make the master password be the password protection of the local cache.

What's the "password protect cache"?

I tested WIP v2 in SeaMonkey and it works fine for my uses (mostly IMAPs with auto-login using saved passwords). As I saw you included the packaging changes here (thanks!), do you want to include the client-py changes to not pull wallet here or should I do a followup for that?

(In reply to comment #25) > As I saw you included the packaging changes here (thanks!), do you want to > include the client-py changes to not pull wallet here or should I do a followup > for that? I think we'll push that to a different patch. This patch is already getting quite large, and dropping pulling wallet can be done separately (and we can consider there if we want to remove the old directory from builds or not).

(In reply to comment #24) > What's the "password protect cache"? mail.password_protect_local_cache - a hidden feature that makes you enter the server password to show locally cached IMAP messages and downloaded POP messages, not just to fetch new ones, to "secure" against people with physical access who don't know how to find your profile or open plaintext files in it.

Patch that fixes bitrot caused by me when I pushed a patch earlier this evening.

Slightly updated patch: quietens a couple of js exceptions due to our channel implementations of nsIRequest::name returning not implement (i.e. implements them), and removes some printfs. With respect to the password protect cache option, this is what I'm currently planning on doing: - If master password is not set, ignore password protect cache preference. - Always store appropriate passwords if user requests us to in the standard format (i.e. no x<protocol>:// urls). - If password protect cache is set, then respect the pref in hopefully the same manner as we do now. This allows us to drop some bad code. I'm not going to enable it or drop the pref (as has been suggested) as I think it would lead to a false sense of security and this bug isn't about that anyway. I have a work in progress patch to do this, hopefully will have that and the rest ready for review by end of day tomorrow.

Now I'm ready for reviews. Due to the complexity of switching to password manager, I'm going to get reviews for all patches, then do one landing rather than try to ifdef things. There are two parts to the mailnews backed changes: Part 1 - non-password protect local cache changes Part 2 - password protect local cache specific changes. To apply and test these patches you need: - Attachment 354341 [details] [diff] Toolkit build config change applied to the mozilla/ part. - The patch on bug 419595. - Parts 1 and 2 (although part 1 should mostly work without part 2). - Thunderbird UI patch from bug 239131 or SeaMonkey UI patches from bug 390025. At the moment, we will suffer from bug 316084, I'm hoping that will be fixed in time for us to put this well before beta 2 - I'd like to get the reviews sorted anyway. Notes about Part 1: - Part 1 has a couple of XXX/commented out sections in nsMsgIncomingServer which are removed in Part 2. - Includes most build type changes - Adds extra tests for the reworked forgetPassword functions (I couldn't add these earlier due to the different interfaces required in the tests). - Newsgroup username - we have to find/prompt/save manually the username. It is done separately to the password as that is how the RFC specifies it can be done (in theory you can login with just a username). - I've tested as many of the interfaces (imap/news/pop3/smtp and a couple of ssl variants) as I can, though as we're not re-working how we're storing the urls we shouldn't run into too many problems.

Part 2: This contains the changes for mail.password_protect_local_cache: - The basic assumption here is that you need a master password to protect your local cache, or to enter (and not save) a server password. I'm tempted to rename the pref, but I'm not sure its really necessary. - Due to the assumption above, this means we don't need to store separate passwords under different urls (i.e. ximap://...), this lets us drop some functions - SMTP servers: I thought a bit about these when I saw the code for saving the password only for the session - I guess what this is on about is that if you don't save the password, and you're protecting the cache, then someone can't send an email for you. As login manager doesn't support session passwords, I've just changed this to never, as we'll still save the password as a member variable, so we'll effectively have a session password anyway.

Toolkit build config patch with slightly more context (so the ifndef MOZ_THUNDERBIRD can be seen). When ready, I'll be checking this into 1.9.1 as NPOTFFB as it doesn't affect the actual Firefox build (unless there are any objections) - it just moves toolkit password manager in the order of building and enables it for Thunderbird and SeaMonkey.

(In reply to comment #30) > At the moment, we will suffer from bug 316084, I'm hoping that will be fixed in > time for us to put this well before beta 2 - I'd like to get the reviews sorted > anyway. I wouldn't worry too much about that bug. Users who update will be no worse off than they are today, login manager won't create new logins with base64 encoding, and the fix for 316084 can land after this patch without any problems. [Not to say it isn't am important issue, but I don't think it's on the critical path to this landing.]

(In reply to comment #33) > I wouldn't worry too much about that bug. Users who update will be no worse off > than they are today, login manager won't create new logins with base64 > encoding, and the fix for 316084 can land after this patch without any > problems. [Not to say it isn't am important issue, but I don't think it's on > the critical path to this landing.] Thanks for the explanation Justin.

Comment on attachment 355453 [details] [diff] [review] Part 2 v1 >+#include "nsILoginManager.h" >+#include "nsILoginInfo.h" I guess this hunk is shared with another patch ;-) >+// Returns true if the user is already logged in or logged in successfully. Both the callers call GetUserNeedsToAuthenticate first. Might be worth moving that check here instead (rename to PromptForMasterPasswordIfNecessary?) >+ // Do we have a master password? Interestingly passwordManager.js uses somewhat different code to do much the same thing (require you to enter your master password [to show passwords]). >+ nsCOMPtr<nsIMsgAccountManager> accountManager = >+ do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); I'm agonising over whether false or true is correct in this case ;-) >+ PRBool PromptForMasterPassword(); static? (class static, that is) >+ rv = dialog->PromptPassword(nsString(aPromptTitle).get(), >+ nsString(aPromptMessage).get(), My preference is to use PromiseFlatString instead, although it comes to the same thing when you use external linkage. >+ passwordProtectLocalCache ? nsIAuthPrompt::SAVE_PASSWORD_NEVER : >+ nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY, Confusing indentation here; perhaps: passwordProtectLocalCache ? nsIAuthPrompt::SAVE_PASSWORD_NEVER : nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY,

Comment on attachment 355457 [details] [diff] [review] [checked in] Toolkit build config change This is inside an ifndef MOZ_THUNDERBIRD, this isn't going to break thunderbird, is it?

(In reply to comment #36) > (From update of attachment 355457 [details] [diff] [review]) > This is inside an ifndef MOZ_THUNDERBIRD, this isn't going to break > thunderbird, is it? Nope, because the rest of the patches on this and related bugs are about switching Thunderbird + SeaMonkey to toolkit password manager ;-)

Comment on attachment 355453 [details] [diff] [review] Part 2 v1 This patch doesn't seem to apply...

Comment on attachment 355452 [details] [diff] [review] Part 1 v1 >-#ifdef USE_TK_LOGIN_MANAGER Are you going to remove the logic that sets this define? >-MOZ_EXTENSIONS_DEFAULT=" wallet" >+MOZ_EXTENSIONS_DEFAULT=" " Just omit the line completely. > components/txmgr.dll > components/uconv.dll > components/wallet.dll >+components/@DLL_PREFIX@walletviewers@DLL_SUFFIX@ Why does only this one deserve the @DLL_???FIX@? > enterUsername=Please enter a username for news server access: >+saveUsername=Use Password Manager to remember this value. > enterPassword=Please enter a password for news server access: > enterPasswordTitle=News Server Password Required I noticed while testing this patch that there's no enterUsernameTitle :-( > nsMsgIncomingServer::ForgetPassword() Apparent code duplication with other Forget methods. >+ // Login Manager >+ var loginMgr = Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager); >+ >+ // Passwords File (generated from Mozilla 1.8 branch). >+ var signons = do_get_file("../mailnews/test/data/signons-mailnews1.8-multiple.txt"); >+ >+ // Copy the file to the profile directory for a PAB >+ signons.copyTo(gProfileDir, "signons.txt"); Don't you have to do this before getting the login manager, so it will migrate, or does it migrate on first query? Can you rely on that? >+ loginMgr->SetLoginSavingEnabled(NS_LITERAL_STRING("testhost"), PR_FALSE); ??? >+ if (count > 0) >+ { >+ nsString username; >+ rv = logins[0]->GetPassword(username); >+ >+ NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY(count, logins); Not sure you're allowed to condition this on count > 0

(In reply to comment #39) > (From update of attachment 355452 [details] [diff] [review]) > >-#ifdef USE_TK_LOGIN_MANAGER > Are you going to remove the logic that sets this define? There's no logic that sets it at the moment. > > enterUsername=Please enter a username for news server access: > >+saveUsername=Use Password Manager to remember this value. > > enterPassword=Please enter a password for news server access: > > enterPasswordTitle=News Server Password Required > I noticed while testing this patch that there's no enterUsernameTitle :-( Although not strictly part of this patch, I've added it anyway. > > nsMsgIncomingServer::ForgetPassword() > Apparent code duplication with other Forget methods. I'd prefer not to spend time moving these around now (except for fixes), so I filed bug 472503. > >+ // Login Manager > >+ var loginMgr = Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager); > >+ > >+ // Passwords File (generated from Mozilla 1.8 branch). > >+ var signons = do_get_file("../mailnews/test/data/signons-mailnews1.8-multiple.txt"); > >+ > >+ // Copy the file to the profile directory for a PAB > >+ signons.copyTo(gProfileDir, "signons.txt"); > Don't you have to do this before getting the login manager, so it will migrate, > or does it migrate on first query? Can you rely on that? Login manager doesn't migrate until first query - I think that's for perf reasons. > > >+ loginMgr->SetLoginSavingEnabled(NS_LITERAL_STRING("testhost"), PR_FALSE); > ??? Opps I have no idea why that is there, removed. > >+ nsCOMPtr<nsIMsgAccountManager> accountManager = > >+ do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); > >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); > I'm agonising over whether false or true is correct in this case ;-) I choose false - if we get this far and fail, then there's something wrong anyway, do there's not much point in letting them see the files.

Comment on attachment 355452 [details] [diff] [review] Part 1 v1 r=me given that you've updated the patch as per your comment.

Comment on attachment 355453 [details] [diff] [review] Part 2 v1 r=me with my comments addressed as discussed on IRC.

Revised part 1 addressing Neil's comments.

Revised part 2 with Neil's comments.

This is an additional fix we're going to need. I didn't pick up on it before because I wasn't looking at complex usernames (e.g. user.name@host.com). I was planning on unescaping everything in password manager, but that could be risky at this stage (see discussion on bug 472824), so I think the best low-risk option is to keep the usernames as escaped (as per the urls), backing out bug 461341. This patch adjusts our forgetPassword routines so that they will unescape the stored usernames before comparison with the stored username. This will all work because: - The migration routines get the old signons.txt url from the file and shove it into a nsStandardURL derived URL. This will force the username to be encoded completely. - Although mailnews urls only ESCAPE_XALPHAS, we call promptPassword with a string URI, which again is put into an nsStandardURL and forces the username to be encoded completely. - We have unit tests to cover ourselves against bustages. I'm just requesting a quick-sr comment to check this is acceptable, because if so, I need to sort out bug 472824. I still want to fix up some additional unit tests to get this fully covered.

Comment on attachment 356524 [details] [diff] [review] Additional fix: unescaping usernames. >+ if (NS_FAILED(logins[i]->GetUsername(username))) >+ break; Previous behaviour would be to continue and try the next login. As if GetUsername would fail ;-) >+ // Ensure username is unescaped, we have to do it this way round because >+ // mailnews encodes passwords to ESCAPE_XALPHAS level, however on migration >+ // password manager encodes to URI level which encodes more. Hence we need >+ // to decode each login to get the match to work. Comment is incorrect. We do it this way around because password manager uses the encoded password provided by nsStandardURL. (Migration actually tries to decode the passwords, which might actually stop them working.) Of course, you might be able to persuade login manager to decode the standard username ;-) >+ print(kUser1); ;-)

(In reply to comment #32) > Created an attachment (id=355457) [details] > Toolkit build config change > > When ready, I'll be checking this into 1.9.1 What about the mozilla-central trunk?

Comment on attachment 355457 [details] [diff] [review] [checked in] Toolkit build config change Checked into trunk: http://hg.mozilla.org/mozilla-central/rev/5011a32be5d3 and 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cefb32039398

Comment on attachment 355774 [details] [diff] [review] [checked in] Part 1 v2 Checked in as part of http://hg.mozilla.org/comm-central/rev/30c7a484a3ae

Comment on attachment 355775 [details] [diff] [review] [checked in] Part 2 v2 Checked in as part of http://hg.mozilla.org/comm-central/rev/30c7a484a3ae