454064 - Eliminate legacy code for param-passing to the compose window

Status: NEW

(Thunderbird :: Message Compose Window, defect)

Description

[Joey Minta]

Attachment: patch v1

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1184
The answer appears to be "yes", once the commandline-handler does its own conversion from string->params. This patch moves the string conversion to the commandline handler (and cleans it up a bit), and then removes the code from the compose window.

Comment 1

[Magnus Melin [:mkmelin]]

Comment on attachment 337288 [details] [diff] [review]
patch v1

This breaks at least the case of doing a
 $ thunderbird -compose to='[e-mail]',cc='[e-mail]',subject="something",body="Something else",attachment="file://home/magnus/tmp/mm24.eml"

=> EX: = TypeError: params is null


>+  // Replace all commas (that aren't in quotes) with our actual separator

Here and elsewhere, please put a dot on the end when you're using a full sentence anyway.

>+    // Now assign all this data to a nsIMsgComposeParam
>+    var params = Cc["@mozilla.org/messengercompose/composeparams;1"].
>+                 createInstance(Ci.nsIMsgComposeParams);
>+    params.composeFields = Cc["@mozilla.org/messengercompose/composefields;1"].
>+                           createInstance(Ci.nsIMsgCompFields);

Always put the dots on the next line. Here and in a few other places in the patch.

>+   var argname = pair.substring(0, pos);

One space off

Comment 2

[:aceman]

This could take even more removal now that recycling is gone. The aParams to ComposeStartup() is unused in TB code (was used by recycling). We could check addons and then remove it.

Jorg, would you like to finish this?

Comment 3

[Jorg K (CEST = GMT+2)]

Not necessarily ;-) You can do it and I review.

Comment 4

[:aceman]

OK, I'll try.

Comment 5

[:aceman]

(In reply to Magnus Melin from comment #1)
> This breaks at least the case of doing a
>  $ thunderbird -compose
> to='[e-mail]',cc='[e-mail]',subject="something",
> body="Something else",attachment="file://home/magnus/tmp/mm24.eml"
> 
> => EX: = TypeError: params is null

Yes, still happens. I'll look at that. But even without the patch, this command args just open an empty compose window, nothing prefilled.

And it produces:
ReferenceError: reference to undefined property args.bodyislink 1 MsgComposeCommands.js:2297

> >+  // Replace all commas (that aren't in quotes) with our actual separator
> 
> Here and elsewhere, please put a dot on the end when you're using a full
> sentence anyway.

Ok.

> >+    // Now assign all this data to a nsIMsgComposeParam
> >+    var params = Cc["@mozilla.org/messengercompose/composeparams;1"].
> >+                 createInstance(Ci.nsIMsgComposeParams);
> >+    params.composeFields = Cc["@mozilla.org/messengercompose/composefields;1"].
> >+                           createInstance(Ci.nsIMsgCompFields);
> 
> Always put the dots on the next line. Here and in a few other places in the
> patch.

Ok.

> >+   var argname = pair.substring(0, pos);
> 
> One space off

Ok

Comment 6

[:aceman]

I think I see what the problem is. The place Joey is modifying in nsMailDefaultHandler.js is for the command line argument in the format:
-remote xfedocommand(composemessage, to='[e-mail]',cc='[e-mail]',subject="something",
> body="Something else",attachment="file://home/magnus/tmp/mm24.eml).

But -compose is handled at https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1371 .

Note Firefox has removed support for "-remote <anything>" (https://developer.mozilla.org/en-US/docs/Mozilla/Command_Line_Options#Remote_Control).

Now open-coding parsing the arguments at both places does not look nice. Maybe we could make the parsing a method of nsMsgComposeService and have it called from both places. Then even Seamonkey could use that code.

Comment 7

[:aceman]

(In reply to :aceman from comment #2)
> This could take even more removal now that recycling is gone. The aParams to
> ComposeStartup() is unused in TB code (was used by recycling). We could
> check addons and then remove it.

A few addons override ComposeStartup and/or later call the internal thing. Those still have it with 2 arguments (recycled, aParams) so they are already broken anyway.

Comment 8

[Jorg K (CEST = GMT+2)]

This has stalled somewhat.

Note that in bug 882104 we're busily adding more options to
thunderbird -compose ...

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #6)
> Maybe we could make the parsing a method of nsMsgComposeService and have it
> called from both places. Then even Seamonkey could use that code.

Sounds good to me.