647685 - mailNavigatorOverlay is over 60% script

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[[e-mail]]

It would seem to make sense to move the script to its own file.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Moved JS code to mailNavigatorOverlay.js (v1)

Comment 2

[[e-mail]]

Comment on attachment 524058 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v1)

Nit: while we're here, a few tweaks wouldn't go amiss.

>+var gUseExternalMailto =
>+    Components.classes["@mozilla.org/network/io-service;1"]
>+              .getService(Components.interfaces.nsIIOService)
>+              .getProtocolHandler("mailto")
>+               instanceof Components.interfaces.nsIExternalProtocolHandler;
We can now use Services.io i.e.
>var gUseExternalMailto = Services.io.getProtocolHandler("mailto") instanceof Components.interfaces.nsIExternalProtocolHandler;
(not sure how to wrap this version though)

>+    var params = Components.classes["@mozilla.org/messengercompose/composeparams;1"]
>+                           .createInstance(Components.interfaces.nsIMsgComposeParams);
>+    if (params)
>+    {
>+      params.composeFields = Components.classes["@mozilla.org/messengercompose/composefields;1"]
>+                                       .createInstance(Components.interfaces.nsIMsgCompFields);
>+      if (params.composeFields)
We don't need these if statements, createInstance will throw if there's a problem. So these can be written
>var params = Components.classes["@mozilla.org/messengercompose/composeparams;1"]
>                       .createInstance(Components.interfaces.nsIMsgComposeParams);
>params.composeFields = Components.classes["@mozilla.org/messengercompose/composefields;1"]
>                                 .createInstance(Components.interfaces.nsIMsgCompFields);
>if (attachment == 0 || attachment == 1)
etc.

>+          var attachmentData = Components.classes["@mozilla.org/messengercompose/attachment;1"]
>+                                         .createInstance(Components.interfaces.nsIMsgAttachment);
>+          if (attachmentData)
This if can go too.

>+        var composeService = Components.classes["@mozilla.org/messengercompose;1"]
>+                                       .getService(Components.interfaces.nsIMsgComposeService);
>+        if (composeService) {
And this one too.

>+  var mailto = url ? "mailto:?body="+encodeURIComponent(url)+"&subject="+encodeURIComponent(title) : "mailto:";
Nit: put spaces around the plus signs and try to wrap it nicely.

>+  var uri = ioService.newURI(mailto, null, null);
Another Services.io thus eliminating the ioService variable.

>+  if (gUseExternalMailto) {
>+    openExternalMailer();
>+  }
>+  else {
>+    if ("MsgNewMessage" in window)
>+    {
>+      MsgNewMessage(null);
>+      return;
>+    }
This would be neater as an if... else if... else i.e.
>if (gUseExternalMailto) {
>  openExternalMailer();
>}
>else if ("MsgNewMessage" in window) {
>  MsgNewMessage(null);
>}
>else {
etc.

>+    var msgComposeService = Components.classes["@mozilla.org/messengercompose;1"].getService();
>+    msgComposeService = msgComposeService.QueryInterface(Components.interfaces.nsIMsgComposeService);
Nit: combine these into one statement using
.getService(Components.interfaces.nsIMsgComposeService);

>+    var charset =  getCharsetforSave(null);
Nit: two spaces after =, only need one

Comment 3

[Edmund Wong (:ewong)]

Attachment: Moved JS code to mailNavigatorOverlay.js (v2)

Comment 4

[[e-mail]]

Comment on attachment 524174 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v2)

>+  var mailto = url ? "mailto:?body=" + encodeURIComponent(url) 
>+                                     + "&subject="+encodeURIComponent(title) : "mailto:";
Nit: missed the + after "&subject=" also I think it would be slightly neater to line that + up with the + on the line above.

>+  if (gUseExternalMailto) {
>+    openExternalMailer();
>+  }
>+  else if ("MsgNewMessage" in window)
>+  {
>+    MsgNewMessage(null);
>+  }
>+  else
>+  {
Bah, I just noticed that there's a mixture of { styles. I think more of them have the { on the same line, so it would be great if you could fix the others.

Comment 5

[Edmund Wong (:ewong)]

Attachment: Moved JS code to mailNavigatorOverlay.js (v3)

Comment 6

[[e-mail]]

Comment on attachment 524608 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v3)

>+  if (gUseExternalMailto) {
Missed one ;-) No new review needed.

>+  var mailto = url ? "mailto:?body=" + encodeURIComponent(url)
>+                                     + "&subject="
>+                                     + encodeURIComponent(title) : "mailto:";
(Not what I was thinking of by this works too.)

Comment 7

[Edmund Wong (:ewong)]

Attachment: Moved JS code to mailNavigatorOverlay.js (v3) [Checked in: Comment 8 & 9]

Comment 8

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/8dad96e3d0da

Comment 9

[Philip Chee]

Pushed to comm-2.0
http://hg.mozilla.org/releases/comm-2.0/rev/d49e268febbd

Comment 10

[[e-mail]]

(In reply to comment #2)
> We can now use Services
Well, that turns out to be not quite true, because Services is loaded by utilityOverlay which is loaded by navigatorOverlay but that means it is queued after mailNavigatorOverlay. Oops.

Comment 11

[[e-mail]]

Comment on attachment 524614 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v3)
[Checked in: Comment 8 & 9]

Can someone please fix the DOS line endings in mailNavigatorOverlay.js?

Comment 12

[[e-mail]]

Attachment: Fix startup error [Checked in: Comment 14]

Note: this patch is against unix line endings. The line endings need to be fixed first in a separate commit.

Comment 13

[Philip Chee]

Comment on attachment 527218 [details] [diff] [review]
Fix startup error [Checked in: Comment 14]

> Created attachment 527218 [details] [diff] [review]
> Fix startup error
r=me

> Note: this patch is against unix line endings. The line endings need to be
> fixed first in a separate commit.
rs=me on fixing line endings.

Comment 14

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/381a61d3643e (Fix DOS newlines)
http://hg.mozilla.org/comm-central/rev/0801160b7f27 (Fix startup error)
Pushed to comm-20:
http://hg.mozilla.org/releases/comm-2.0/rev/bc168fbd6232 (Fix DOS newlines)
http://hg.mozilla.org/releases/comm-2.0/rev/8508ced092c1 (Fix startup error)