Closed Bug 647685 Opened 9 years ago Closed 9 years ago

mailNavigatorOverlay is over 60% script

Categories

(SeaMonkey :: General, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1final

People

(Reporter: neil, Assigned: ewong)

References

()

Details

Attachments

(2 files, 3 obsolete files)

It would seem to make sense to move the script to its own file.
Status: NEW → ASSIGNED
Summary: mailTasksOverlay is over 60% script → mailNavigatorOverlay is over 60% script
Attachment #524058 - Flags: review?(neil)
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
Attachment #524058 - Flags: review?(neil) → review+
Attachment #524058 - Attachment is obsolete: true
Attachment #524174 - Flags: review?(neil)
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.
Attachment #524174 - Flags: review?(neil) → review+
Attachment #524174 - Attachment is obsolete: true
Attachment #524608 - Flags: review?(neil)
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.)
Attachment #524608 - Flags: review?(neil) → review+
Keywords: checkin-needed
Pushed to comm-2.0
http://hg.mozilla.org/releases/comm-2.0/rev/d49e268febbd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
(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 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?
Note: this patch is against unix line endings. The line endings need to be fixed first in a separate commit.
Attachment #527218 - Flags: review?(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.
Attachment #527218 - Flags: review?(philip.chee) → review+
Keywords: checkin-needed
Whiteboard: [2 commits on both comm-central and comm-2.0 as per comments 11-13]
Depends on: 651458
Attachment #524614 - Attachment description: Moved JS code to mailNavigatorOverlay.js (v3) → Moved JS code to mailNavigatorOverlay.js (v3) [Checked in: Comment 8 & 9]
Version: unspecified → Trunk
Status: RESOLVED → REOPENED
blocking-seamonkey2.1: --- → ?
Resolution: FIXED → ---
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)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #527218 - Attachment description: Fix startup error → Fix startup error [Checked in: Comment 14]
blocking-seamonkey2.1: ? → ---
Whiteboard: [2 commits on both comm-central and comm-2.0 as per comments 11-13]
You need to log in before you can comment on or make changes to this bug.