1662665 - move nsMessengerContentHandler.cpp functionality into MessengerContentHandler.jsm

Status: RESOLVED FIXED

(Thunderbird :: General, task, P3)

Description

[Magnus Melin [:mkmelin]]

We have two messengercontenthandlers now. The remaining stuff in the .cpp one should be moved to MessengerContentHandler.jsm

https://searchfox.org/comm-central/source/mailnews/base/src/nsMessengerContentHandler.cpp

https://searchfox.org/comm-central/source/mail/components/MessengerContentHandler.jsm

This will make it easier to do ports of BrowserContentHandler.jsm functionality: https://searchfox.org/comm-central/source/mozilla/browser/components/BrowserContentHandler.jsm
E.g. showWhatsNewPage() should be handled there, like in it's browser equivalence.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-0.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9186636 [details] [diff] [review]
Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-0.patch

Review of attachment 9186636 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/MessengerContentHandler.jsm
@@ +111,2 @@
>  
>    var listener = {

please add @implements {nsIURIContentListener}

@@ +111,5 @@
>  
>    var listener = {
>      doContent(ctype, preferred, request, handler) {
> +      let uri = request.URI;
> +      if (uri) {

There's always going to be a uri

@@ +145,5 @@
> +            "chrome://messenger/content/messageWindow.xhtml",
> +            "_blank",
> +            "all,chrome,dialog=no,status,toolbar",
> +            request.URI
> +          );

this is the same for if-else so move it there. 
But what about not mailnewsurl, not file? I think nothing should happen then?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6931,5 @@
> +        let queryPart = mailnewsurl.query.replace(
> +          "type=message/rfc822",
> +          "type=application/x-message-display"
> +        );
> +        request.URI = mailnewsurl

would use the local uri variable you have there for these, and pass that in

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-1.patch

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ac56b0f15aaa70217f8b069ee517ef0e214793a2&selectedTaskRun=W8UnoneJRpa59h2OZMxpow.0

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9187640 [details] [diff] [review]
Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-1.patch

Review of attachment 9187640 [details] [diff] [review]:
-----------------------------------------------------------------

Why does it now have to be in two places?

::: mail/components/MessengerContentHandler.jsm
@@ +109,4 @@
>  
>    loadgroup.groupObserver = loadlistener;
>  
> +  // @implements {nsIURIContentListener}

please add to attachmentopener as well

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6921,5 @@
>          .setQuery(newQuery)
>          .finalize();
>      }
> +    let uri = request.URI;
> +    if (uri) {

uri will always be set, I'd imagine?

@@ +6943,5 @@
> +          "all,chrome,dialog=no,status,toolbar",
> +          request.URI
> +        );
> +      } else {
> +        if (uri.scheme == "file") {

like for the other cased, else if, and then use a common open uri

Comment 5

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #4)

Why does it now have to be in two places?

I have inlined it at two places where we are using the MessengerContentHandler from C++. Now, it's more of a javascript function.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-2.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9187646 [details] [diff] [review]
Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-2.patch

Review of attachment 9187646 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't MessengerContentHandler implement nsIContentHandler, and then you don't need the duplication in js?

Comment 8

[Khushil Mistry [:khushil324]]

Previously, we are using it as a util function only by calling nsMessengerContentHandler.handleContent. And we are using it in the openURI function which is used by the MessengerContentHandler only. So it will be a kind of recursion: https://searchfox.org/comm-central/source/mail/components/MessengerContentHandler.jsm#73,114-117,146,148,176,181,577,581,588
We can do one thing, create a util function and use at both the places.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9187646 [details] [diff] [review]
Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-2.patch

Review of attachment 9187646 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/MessengerContentHandler.jsm
@@ +109,4 @@
>  
>    loadgroup.groupObserver = loadlistener;
>  
> +  // @implements {nsIURIContentListener}

jsdoc style please
/** @implements {nsIURIContentListener} */

Otherwise it wouldn't get into automatically generated documentation

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6903,4 @@
>    } // if one attachment selected
>  }
>  
> +// @implements {nsIURIContentListener}

same here

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-3.patch

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9188070 [details] [diff] [review]
Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-3.patch

Review of attachment 9188070 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6903,5 @@
>    } // if one attachment selected
>  }
>  
> +/**
> + * @implements {nsIURIContentListener}

would add also @see {MessengerContentHandler}

@@ +6931,5 @@
> +      let queryPart = mailnewsurl.query.replace(
> +        "type=message/rfc822",
> +        "type=application/x-message-display"
> +      );
> +      request.URI = mailnewsurl

for consistency with the other impl, use uri, here and the two places below

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1662665_move-nsMessengerContentHandler-into-MessengerContentHandler-4.patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8587f3e2dbbf
move nsMessengerContentHandler.cpp functionality into MessengerContentHandler.jsm. r=mkmelin

Comment 14

[Geoff Lankow (:darktrojan)]

This broke these tests that try to open an email attachment, so I've backed it out:

mail/test/browser/attachment/browser_attachment.js
mail/test/browser/composition/browser_forwardedEmlActions.js
mail/test/browser/message-header/browser_phishingBar.js

We've lost registration of the nsIURIContentListener, so opening an attachment goes straight to the external helper app service.

Backout:
https://hg.mozilla.org/comm-central/rev/8444d32fb2784a3b015029090fa7c4b74396d661

Comment 15

[Khushil Mistry [:khushil324]]

Geoff, any thoughts on how to approach this?

Comment 16

[Geoff Lankow (:darktrojan)]

Looks like you should be able to do it with a components.conf. Some examples.

Comment 17

[Magnus Melin [:mkmelin]]

Ping, maybe you can finish this off at some point

Comment 18

[Ping Chen [:rnons] (away)]

Attachment: Bug 1662665 - Merge nsMessengerContentHandler.cpp into MessengerContentHandler.jsm. r=mkmelin

Based on Khushil's patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f033e1b873da
Merge nsMessengerContentHandler.cpp into MessengerContentHandler.jsm. r=mkmelin