884319 - Add http.jsm to toolkit for usage by Thunderbird FileLink, Lightning and Instantbird

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Patrick Cloke [:clokep]]

This is copied and pasted from a conversation I had with Dave Townsend (Mossop), Florian Queze and Mike Conley:

We've had Thunderbird break a couple of times recently because of changes to the chat/ component (see bug 735219 and bug 844175).  The chat/ code is shared between Instantbird and Thunderbird (and manually synced between the two products).  One of the JavaScript modules [1][2] we created for chat/ proved useful for  another Thunderbird feature: FileLink and a copy of it was created in mail/ (since this code cannot depend on a module from chat/ and we were unsure whether the chat feature or FileLink feature would land first).  It really sucks having essentially two copies of the same file in the Thunderbird source tree. Since the FileLink version is built last, it is the one used. If the API is changed and chat/ code expects a newer version, we break chat/ since the "old" version is used.

That was long winded, but it was brought up during our status meeting that moving this code to toolkit might be the right way to share it...and that you are the man to talk to about this!  (Note that putting it in a "common" place in comm-central will not work because Instantbird is not based off comm-central, it's based off mozilla-release.)

Since I've written this, Lightning has started using this code in bug 853236, which is not appropriate since the http.jsm used in Thunderbird is in mail/, not mailnews/, and thus is unavailable to SeaMonkey.

Dave said he'd probably want some API changes, so we'll need to also land patches for c-c after this.

We'd like this to land for the next uplift (June 24) to avoid this problem in the next ESR for Thunderbird.

I'll attach a patch as soon as m-c finishes cloning.

[1] Chat: http://mxr.mozilla.org/comm-central/source/chat/modules/http.jsm
[2] Mail: http://mxr.mozilla.org/comm-central/source/mail/base/modules/http.jsm

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Add http.jsm into toolkit/modules

This is just imported from the newest version in Instantbird/comm-central. It didn't look like I had to change any moz.build files, but I'm unsure how those work. I have a build running now (but I figured I'd get it up since you said you wanted to review the API anyway...and there are no consumers in m-c). Thanks!

Comment 2

[Patrick Cloke [:clokep]]

Fallen let me know on IRC that I need to add it to Makefile.in, I'll put that in the next iteration of the patch.

Also, I've always hated doXHRequest, so now is probably a good time to rename this.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 764184 [details] [diff] [review]
Add http.jsm into toolkit/modules

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

A few comments to be getting on with. This will also need automated tests.

::: toolkit/modules/http.jsm
@@ +6,5 @@
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +// Strictly follow RFC 3986 when encoding URI components.
> +function percentEncode(aString)

Add documentation about what this accepts and returns

@@ +10,5 @@
> +function percentEncode(aString)
> +  encodeURIComponent(aString).replace(/[!'()]/g, escape).replace(/\*/g, "%2A");
> +
> +function doXHRequest(aUrl, aHeaders, aPOSTData, aOnLoad, aOnError, aThis,
> +                     aMethod, aLogger) {

I think to keep this relatively safe to change in the future this should accept just two arguments, the first a url and the second an object containing properties for the other things you want to pass in.

The aThis argument needs to go away, it's quite abnormal to pass such a thing, callers can use aOnLoad.bind(aThis) if that is the behaviour they need.

I think we should rename this to httpRequest, that it uses an xhr underneath is just an implementation detail.

Can we remove the logging?

Comment 4

[Patrick Cloke [:clokep]]

(In reply to Dave Townsend (:Mossop) from comment #3)
> A few comments to be getting on with. This will also need automated tests.
Thanks for the speedy comments. Do you have a suggestion about what type of tests this should have? I haven't written tests that deal with networking before, would this involve using the httpd.js stuff or something else?

> @@ +10,5 @@
> > +function doXHRequest(aUrl, aHeaders, aPOSTData, aOnLoad, aOnError, aThis,
> > +                     aMethod, aLogger) {
> 
> I think to keep this relatively safe to change in the future this should
> accept just two arguments, the first a url and the second an object
> containing properties for the other things you want to pass in.
Wouldn't this just make it more complicated? Passing in some arbitrary object that may or may not have properties defined on it? I don't like this method, personally, but will change it if I must.

> The aThis argument needs to go away, it's quite abnormal to pass such a
> thing, callers can use aOnLoad.bind(aThis) if that is the behaviour they
> need.
Done.

> I think we should rename this to httpRequest, that it uses an xhr underneath
> is just an implementation detail.
Done, should I rename the file to Http.jsm (from http.jsm) to match the rest of the files in this directory?

> Can we remove the logging?
The logging is very useful for figuring out what goes wrong when a request is denied (which happens frequently when creating new oauth type mechanisms as they're all implemented slightly differently). Why do you dislike logging?

I'm taking the other comments into account now. Thanks!

Comment 5

[Patrick Cloke [:clokep]]

Attachment: m-c patch v2

I'm trying to get c-c to build so I can write / test my tests, but this is how I've changed the API so far.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: c-c patch v1

This is the comm-central changes to match. I haven't tested this as I'm still trying to get c-c to build properly (the tree was burning last time I checked), but this will need review from a variety of people.

We also need to update the following (I'll provide patches in the respective places):
https://github.com/gierschv/thunderbird-filelink-hubiC

I'm not aware of any third-party chat code that uses this. I used http://mxr.mozilla.org/comm-central/search?string=http.jsm&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central to generate this patch.

I've also ensure the following don't use this:
https://github.com/mikeconley/thunderbird-filelink-dropbox (uses oauth.jsm)
https://addons.mozilla.org/en-US/thunderbird/addon/fileswapcom-filelink/ (use XHR)
https://addons.mozilla.org/en-US/thunderbird/addon/ajaxplorer-for-filelink/ (uses XHR)
https://addons.mozilla.org/en-US/thunderbird/addon/~okeanos-for-filelink/ (uses XHR)
https://addons.mozilla.org/en-US/thunderbird/addon/webdav-for-filelink/ (uses sockets)

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 765564 [details] [diff] [review]
c-c patch v1

r+ for calendar.

I've done a quick search at https://mxr.mozilla.org/addons and found there are no AMO addons that use this http.jsm except for the FileLink one you mentioned.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Patrick Cloke [:clokep] from comment #4)
> (In reply to Dave Townsend (:Mossop) from comment #3)
> > A few comments to be getting on with. This will also need automated tests.
> Thanks for the speedy comments. Do you have a suggestion about what type of
> tests this should have? I haven't written tests that deal with networking
> before, would this involve using the httpd.js stuff or something else?
> 
> > @@ +10,5 @@
> > > +function doXHRequest(aUrl, aHeaders, aPOSTData, aOnLoad, aOnError, aThis,
> > > +                     aMethod, aLogger) {
> > 
> > I think to keep this relatively safe to change in the future this should
> > accept just two arguments, the first a url and the second an object
> > containing properties for the other things you want to pass in.
> Wouldn't this just make it more complicated? Passing in some arbitrary
> object that may or may not have properties defined on it? I don't like this
> method, personally, but will change it if I must.

My concern here is one of maintaining compatibility in the future if we add more functionality. tacking on parameters ad infinitum gets ugly fast and old callers break badly if we suddenly lose a parameter for some reason. Using an object means that old callers continue to work in most cases.

> > Can we remove the logging?
> The logging is very useful for figuring out what goes wrong when a request
> is denied (which happens frequently when creating new oauth type mechanisms
> as they're all implemented slightly differently). Why do you dislike logging?

I don't dislike logging, we just don't have a standard way to do it and this module uses something different to anything I've seen before and I'd rather it didn't. Assuming the logger was either a web console object or a log4moz object (both have .debug and .log functions) would be ok. Alternatively you could just log the failure to connect to the error console and not log the connecting message.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: m-c patch v3

This patch uses an object to configure the HTTP request, it still includes the logger, but uses the console/log4moz style of .log/.debug, I hope that seems reasonable.

I've, unfortunately, been unable to get tests to work using httpd.js. The file, while attached here, isn't very useful. It seems that the server keeps closing before the tests actually occur. I'm hoping someone might have an idea of what's wrong.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: c-c patch v3

The matching comm-central patch to attachment 766739 [details] [diff] [review], sorry to have to re-request review like this.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 766739 [details] [diff] [review]
m-c patch v3

There's a bunch of stuff in here that shouldn't be making me wonder if this is the right patch? Attach a version without the .gdbinit changes etc. and I'll do the final review.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 766741 [details] [diff] [review]
c-c patch v3

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

r=philipp for calendar with this fixed:

::: calendar/base/modules/OAuth2.jsm
@@ +174,5 @@
> +        let t = this;
> +        let options = {
> +          postData: params,
> +          onLoad: t.onAccessTokenReceived.bind(t),
> +          onError: t.onAccessTokenFailed.bind(t)

You can use |this| directly here, no need to use a shortcut. Just t is very uncommon in calendar code.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 766741 [details] [diff] [review]
c-c patch v3

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

r=me for the chat/ part if you apply these 2 changes, or explain why bind isn't needed there.

::: chat/protocols/twitter/twitter.js
@@ +462,2 @@
>  
> +    let t = this;

Similar to what Philipp said, you can use 'this' directly, no need for the variable.

@@ +465,5 @@
> +      headers: (aHeaders || []).concat([["Authorization", authorization]]),
> +      postData: aPOSTData,
> +      onLoad: aOnLoad.bind(t),
> +      onError: aOnError.bind(t),
> +      logger: {log: t.LOG, debug: t.DEBUG}

I suspect these 2 functions need .bind(this) to work correctly.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 766739 [details] [diff] [review]
m-c patch v3

Drive-by: it looks like all the |aOptions.hasOwnProperty(foo)| calls in the new Http.jsm file want to be just |foo in aOption|. (I see no reason to exclude methods that would be inherited from a prototype of the option object.)

Comment 15

[Patrick Cloke [:clokep]]

Attachment: m-c patch v4

I have no idea how those other files ended up in that patch, I've never touched those files. Sorry about that. Here's the patch for review, with tests! :) Thanks for the quick reviews.

Comment 16

[Patrick Cloke [:clokep]]

Attachment: c-c patch v4

Should be the last version. Thanks for the review comments! This drops the "let t = this" throughout the entire patch (chat/ and calendar/ parts). I have no idea what I was thinking in there...

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 767016 [details] [diff] [review]
m-c patch v4

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

::: toolkit/modules/tests/xpcshell/test_Http.js
@@ +21,5 @@
> +  aResponse.setHeader("Content-Type", "application/json");
> +  aResponse.write("Success!");
> +}
> +
> +function checkData(aRequest, aResponse) {

Test the HTTP method in here

@@ +74,5 @@
> +
> +add_test(function test_PostData() {
> +  do_test_pending();
> +  let options = {
> +    onLoad: function(aResponse) {

Test the response is correct

Comment 18

[Patrick Cloke [:clokep]]

Attachment: m-c patch v5

Changes:
- Meets review comments.
- Updated for changes to m-c (i.e. modules moved to moz.build from Makefile.in).
- Removed dump call in test.

Comment 19

[Patrick Cloke [:clokep]]

Once the m-c part is checked in and I get real reviews for the c-c part (and the tree stops burning) we can check that part in.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4842fd9edc

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 767017 [details] [diff] [review]
c-c patch v4

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

I'm fine with this, on the condition that you confirm that the cloudfile Mozmill tests all still pass. Thanks Patrick!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e4842fd9edc

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 767017 [details] [diff] [review]
c-c patch v4

Looks good (I haven't tested locally but I'm sure you have).

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 767017 [details] [diff] [review]
c-c patch v4

r=philipp for calendar

Comment 25

[Mike Conley (:mconley) (:⚙️)]

This change appears to have broken the cloudfile Mozmill tests:

https://tbpl.mozilla.org/php/getParsedLog.php?id=25216223&tree=Thunderbird-Trunk