Last Comment Bug 794909 - Switch to Services.jsm: compose code
: Switch to Services.jsm: compose code
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 20.0
Assigned To: Sebastian Hengst [:aryx][:archaeopteryx]
:
Mentors:
Depends on:
Blocks: 720356 819770
  Show dependency treegraph
 
Reported: 2012-09-27 06:47 PDT by Sebastian Hengst [:aryx][:archaeopteryx]
Modified: 2012-12-16 16:05 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
compose with services.jsm, work in progress patch, v1 (47.86 KB, patch)
2012-09-27 06:47 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Fix for test-save-changes-on-quit.js tests (2.21 KB, patch)
2012-09-27 08:42 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Siwth to Services.jsm: Compose code, patch, v3 (50.07 KB, patch)
2012-10-03 09:04 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
mconley: review-
Details | Diff | Splinter Review
Switch to Services.jsm: Compose code, patch, v4 (55.12 KB, patch)
2012-12-10 01:57 PST, Sebastian Hengst [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Compose code, patch, v5 (55.53 KB, patch)
2012-12-13 16:13 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review

Description Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 06:47:39 PDT
Created attachment 665429 [details] [diff] [review]
compose with services.jsm, work in progress patch, v1

Using Services.jsm will clean up the readability of our code a bit, and should avoid the costs the getService call each time we want a service.

https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm
Comment 1 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 06:53:54 PDT
This patch currently causes Mozmill errors, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c04f17949001

I didn't modify anything in mail/test/mozmill/

If I download the build and run mail/test/mozmill/composition/test-save-changes-on-quit.js locally, it shows me:

fail :: Component returned failure code: 0x80550013 [nsIMsgFolder.createSubfolder]
name: null
message: Component returned failure code: 0x80550013 [nsIMsgFolder.createSubfolder]
lineNumber: 87
fileName: undefined
stack: undefined
test: function (module) { let fdh = collector.getModule("folder-display-helpers"); fdh.installInto(module); let composeHelper = collector.getModule("compose-helpers"); composeHelper.installInto(module); let ph = collector.getModule("prompt-helpers"); ph.installInto(module); }

if the line is correct, there are two files calling createSubfolder in line 87: 
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-tree-modes/test-smart-folders.js#87 (another mozmill test) and http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/messageInjection.js#87 (mailnews)
Comment 2 :aceman 2012-09-27 07:30:37 PDT
@@ -2825,67 +2804,56 @@ function CheckValidEmailAddress(to, cc, 
   if (invalidStr)
   {
-    if (gPromptService)
-      gPromptService.alert(window, getComposeBundle().getString("addressInvalidTitle"),
-                           getComposeBundle().getFormattedString("addressInvalid",
-                                                     [invalidStr], 1));
-    return false;
+    Services.prompt.alert(window, getComposeBundle().getString("addressInvalidTitle"),
+                          getComposeBundle().getFormattedString("addressInvalid",
+                          [invalidStr], 1));

Why is the return removed?
Comment 3 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 07:37:53 PDT
Because of my lack of concentration
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-09-27 08:41:38 PDT
Ok, I figured out what was going on.

Services.prompt, by design, caches a copy of the Prompt Service. test-save-changes-on-quit.js is a test that mocks the prompt service so that it doesn't block us, and allows us to read data that would be passed to the prompt.

However, switching to using Services.prompt bypasses the mock, because when the test executes, Services.prompt already has the real service cached, and this is what is used.

The solution is to overwrite Services.prompt every time the mock prompt service is registered and unregistered, effectively flushing the cache.

I've got a patch coming up that will fix the issue.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-09-27 08:42:43 PDT
Created attachment 665483 [details] [diff] [review]
Fix for test-save-changes-on-quit.js tests
Comment 6 Ludovic Hirlimann [:Usul] 2012-09-28 02:37:41 PDT
(In reply to Mike Conley (:mconley) from comment #5)
> Created attachment 665483 [details] [diff] [review]
> Fix for test-save-changes-on-quit.js tests

no review flags ?
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-10-01 07:11:21 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #6)
> (In reply to Mike Conley (:mconley) from comment #5)
> > Created attachment 665483 [details] [diff] [review]
> > Fix for test-save-changes-on-quit.js tests
> 
> no review flags ?

I figured Archaeopteryx was still interested in driving this, and might fold my patch into his, and r? when we was ready.
Comment 8 Sebastian Hengst [:aryx][:archaeopteryx] 2012-10-03 09:04:40 PDT
Created attachment 667508 [details] [diff] [review]
Siwth to Services.jsm: Compose code, patch, v3

Thank you, I folded the patches and the new patch passed on Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=88d6f34dd1cc
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-10-10 10:24:27 PDT
Comment on attachment 667508 [details] [diff] [review]
Siwth to Services.jsm: Compose code, patch, v3

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

Just a few issues here, but nothing major.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +120,5 @@
>    defaultSaveOperation = "draft";
>    gSendOrSaveOperationInProgress = false;
>    gAutoSaving = false;
>    gCloseWindowAfterSave = false;
> +  gIsOffline = Services.io.offline;

Do we even need this global anymore? If not, just replace it entirely.

@@ +1558,5 @@
>          if (LDAPSession) {
>              let url = getPref(autocompleteDirectory + ".uri", true);
>  
> +            LDAPSession.serverURL = Services.io.newURI(url, null, null)
> +                                               .QueryInterface(Components.interfaces.nsILDAPURL);

Please line up the . in .QueryInterface vertically with the . in .io

@@ +1881,5 @@
>    // see if the string is a mailto url....do this by checking the first 7 characters of the string
>    if (/^mailto:/i.test(mailtoUrl))
>    {
>      // if it is a mailto url, turn the mailto url into a MsgComposeParams object....
> +    var uri = Services.io.newURI(mailtoUrl, null, null);

While you're here, replace the var with let

@@ +2651,5 @@
>      // and the message (still) contains attachment keywords.
>      if ((gRemindLater || (getPref("mail.compose.attachment_reminder_aggressive") &&
>            document.getElementById("attachmentNotificationBox").currentNotification)) &&
>          ShouldShowAttachmentNotification(false)) {
> +      var flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING +

While you're here, please replace the vars with lets

@@ +3335,5 @@
>    if (gSendOrSaveOperationInProgress)
>    {
>      var result;
>  
> +    var brandBundle = document.getElementById("brandBundle");

While you're here, please replace these vars with lets.

@@ +3362,5 @@
>    {
>      // call window.focus, since we need to pop up a dialog
>      // and therefore need to be visible (to prevent user confusion)
>      window.focus();
> +    result = Services.prompt.confirmEx(window,

Line up .confirmEx on the next line like this:

result = Services.prompt
                 .confirmEx(window,

@@ +3472,5 @@
>  {
>    var lastDirectory;
>  
>    try {
> +    lastDirectory = Services.prefs.getComplexValue(kComposeAttachDirPrefName, Components.interfaces.nsILocalFile);

> 80 chars - let's break this up over two lines.

@@ +3490,5 @@
>    try {
>      var file = attachedLocalFile.QueryInterface(Components.interfaces.nsIFile);
>      var parent = file.parent.QueryInterface(Components.interfaces.nsILocalFile);
>  
> +    Services.prefs.setComplexValue(kComposeAttachDirPrefName, Components.interfaces.nsILocalFile, parent);

This is probably > 80 chars too.

@@ +3665,5 @@
>  }
>  
>  function AttachPage()
>  {
> +  var result = {value:"http://"};

Replace the vars with lets

@@ +3666,5 @@
>  
>  function AttachPage()
>  {
> +  var result = {value:"http://"};
> +  if (Services.prompt.prompt(window,

Please align like:

Services.prompt
        .prompt(window

@@ +3800,5 @@
>      return; // not one attachment selected
>  
>    var item = bucket.getSelectedItem(0);
>    var attachmentName = {value: item.attachment.name};
> +  if (Services.prompt.prompt(

Services.prompt
        .prompt

@@ +3849,5 @@
>      else
>      {
>        // turn the url into a nsIURL object then open it
>  
> +      var url = Services.io.newURI(attachmentUrl, null, null);

let not var

@@ +3854,5 @@
>        url = url.QueryInterface( Components.interfaces.nsIURL );
>  
>        if (url)
>        {
> +        var channel = Services.io.newChannelFromURI(url);

let not var

@@ +4247,5 @@
>              //
>              // also skip mailto:, since it doesn't make sense
>              // to attach and send mailto urls
>              try {
> +              var scheme = Services.io.extractScheme(rawData);

let not var

@@ +4342,5 @@
>                                             [msgfolder.name,
>                                              msgfolder.server.prettyName]);
>  
>      var CheckMsg = bundle.getString("CheckMsg");
> +    Services.prompt.alertCheck(window, SaveDlgTitle, dlgMsg,

Services.prompt
        .alertCheck(window

@@ +4695,2 @@
>    if (aIsComplex) {
>        return prefB.getComplexValue(aPrefName, Ci.nsISupportsString).data;

Need to take care of this prefB too.

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +87,5 @@
>              //is it a Windows remote file?
>              if (/^\s*file:\/\/\/\/\//i.test(gMsgCompInputElement.value))
>              {
>                try {
> +                if (Services.prefs.getBoolPref("mail.compose.dont_attach_source_of_local_network_links"))

Let's break this up like:

Services.prefs
        .getBoolPref("mail.compose.dont_attach_source_of_local_network_links")

Even better - assign the pref key to a const, and use that instead.

::: mailnews/compose/test/unit/test_smtpPassword2.js
@@ +33,5 @@
>    var i;
>    var count = {};
>  
>    // Test - Check there are two logins to begin with.
> +  var logins = Services.logins.findLogins(count, kServerUrl, null, kServerUrl);

let not var

::: mailnews/compose/test/unit/test_smtpPasswordFailure1.js
@@ +113,2 @@
>      let count = {};
> +    let logins = Services.logins.findLogins(count, "smtp://localhost", null,

Services.logins
        .findLogins(count

::: mailnews/compose/test/unit/test_smtpPasswordFailure2.js
@@ +138,2 @@
>        let count = {};
> +      let logins = Services.logins.findLogins(count, "smtp://localhost", null, "smtp://localhost");

Services.logins
        .findLogins

::: mailnews/compose/test/unit/test_smtpPasswordFailure3.js
@@ +122,2 @@
>      let count = {};
> +    let logins = Services.logins.findLogins(count, "smtp://localhost", null,

Services.logins
        .findLogins
Comment 10 :aceman 2012-10-31 04:39:47 PDT
May have been bitrotted in MsgComposeCommands.js by bug 271730.
Comment 11 :aceman 2012-12-08 08:59:22 PST
:Aryx, can you finish this?
Comment 12 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-10 01:57:04 PST
Created attachment 690325 [details] [diff] [review]
Switch to Services.jsm: Compose code, patch, v4

Comments have been addressed. Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c05548089b51
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-12-13 09:57:57 PST
Comment on attachment 690325 [details] [diff] [review]
Switch to Services.jsm: Compose code, patch, v4

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

Just some style nits, but this looks good. Thanks!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1610,5 @@
>          //
>          if (LDAPSession) {
>              let url = getPref(autocompleteDirectory + ".uri", true);
>  
> +            LDAPSession.serverURL = Services.io.newURI(url, null, null)

Nit - I'd prefer you break it up like this:

LDAPSession.serverURL = Services.io
                                .newURI(url, null, null)
                                .QueryInterface(Components.interfaces.nsILDAPURL);

@@ +2744,5 @@
>        var dontAskAgain = getPref(kDontAskAgainPref);
>        if (!dontAskAgain)
>        {
>          var checkbox = {value:false};
> +        var okToProceed = Services.prompt.confirmCheck(

Switch var to let

@@ +2938,5 @@
>      var warn = getPref("mail.warn_on_send_accel_key");
>  
>      if (warn) {
>          var checkValue = {value:false};
> +        var buttonPressed = Services.prompt.confirmEx(window,

Switch var to let

@@ +3382,5 @@
> +    let brandBundle = document.getElementById("brandBundle");
> +    let brandShortName = brandBundle.getString("brandShortName");
> +    let promptTitle = getComposeBundle().getString("quitComposeWindowTitle");
> +    let promptMsg = getComposeBundle().getFormattedString("quitComposeWindowMessage2",
> +                                              [brandShortName], 1);

Alignment on lines 3385-3386 looks wrong. I think it should look like:

let promptMsg = getComposeBundle().getFormattedString("quitComposeWindowMessage2",
    [brandShortName], 1);

@@ +3534,5 @@
>  function SetLastAttachDirectory(attachedLocalFile)
>  {
>    try {
>      var file = attachedLocalFile.QueryInterface(Components.interfaces.nsIFile);
> +    var parent = file.parent.QueryInterface(Components.interfaces.nsIFile);

Switch var to let

@@ +3655,5 @@
>        // them. This fixes issues where an icon wasn't showing up if you dragged a
>        // web url that had a query or reference string after the file name and for
>        // mailnews urls where the filename is hidden in the url as a &filename=
>        // part.
> +      var url = Services.io.newURI(attachment.url, null, null);

Switch var to let
Comment 14 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-13 16:13:17 PST
Created attachment 692071 [details] [diff] [review]
Switch to Services.jsm: Compose code, patch, v5

Successful run on try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=24ccf058a3d1 - the oranges are the same like for other try runs.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-12-16 16:05:34 PST
https://hg.mozilla.org/comm-central/rev/99481e63615e

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