mailNavigatorOverlay is over 60% script

RESOLVED FIXED in seamonkey2.1final

Status

--
trivial
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: neil, Assigned: ewong)

Tracking

Trunk
seamonkey2.1final

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
It would seem to make sense to move the script to its own file.
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Summary: mailTasksOverlay is over 60% script → mailNavigatorOverlay is over 60% script
(Assignee)

Comment 1

8 years ago
Created attachment 524058 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v1)
Attachment #524058 - Flags: review?(neil)
(Reporter)

Comment 2

8 years ago
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+
(Assignee)

Comment 3

8 years ago
Created attachment 524174 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v2)
Attachment #524058 - Attachment is obsolete: true
Attachment #524174 - Flags: review?(neil)
(Reporter)

Comment 4

8 years ago
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+
(Assignee)

Comment 5

8 years ago
Created attachment 524608 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v3)
Attachment #524174 - Attachment is obsolete: true
Attachment #524608 - Flags: review?(neil)
(Reporter)

Comment 6

8 years ago
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+
(Assignee)

Comment 7

8 years ago
Created attachment 524614 [details] [diff] [review]
Moved JS code to mailNavigatorOverlay.js (v3)
[Checked in: Comment 8 & 9]
Attachment #524608 - Attachment is obsolete: true
Attachment #524614 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 8

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

Comment 9

7 years ago
Pushed to comm-2.0
http://hg.mozilla.org/releases/comm-2.0/rev/d49e268febbd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
(Reporter)

Comment 10

7 years ago
(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.
(Reporter)

Comment 11

7 years ago
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?
(Reporter)

Comment 12

7 years ago
Created attachment 527218 [details] [diff] [review]
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.
Attachment #527218 - Flags: review?(philip.chee)

Comment 13

7 years ago
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+
(Reporter)

Updated

7 years ago
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 → ---

Comment 14

7 years ago
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
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

7 years ago
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.