Switch to Services.jsm: compose code

RESOLVED FIXED in Thunderbird 20.0

Status

Thunderbird
Message Compose Window
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aryx, Assigned: aryx)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 20.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
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

5 years ago
@@ -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?
Because of my lack of concentration
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.
Created attachment 665483 [details] [diff] [review]
Fix for test-save-changes-on-quit.js tests
(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 ?
(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.
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
Attachment #665429 - Attachment is obsolete: true
Attachment #665483 - Attachment is obsolete: true
Attachment #667508 - Flags: review?(mconley)
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
Attachment #667508 - Flags: review?(mconley) → review-

Comment 10

5 years ago
May have been bitrotted in MsgComposeCommands.js by bug 271730.

Comment 11

4 years ago
:Aryx, can you finish this?
Assignee: nobody → archaeopteryx
Status: NEW → ASSIGNED
Flags: needinfo?(archaeopteryx)

Updated

4 years ago
Blocks: 819770
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
Attachment #667508 - Attachment is obsolete: true
Attachment #690325 - Flags: review?(mconley)
Flags: needinfo?(archaeopteryx)
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
Attachment #690325 - Flags: review?(mconley) → review+
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.
Attachment #690325 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/99481e63615e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.