Improving test coverage to ensure httpRequest API allows setting custom MIME type for POST and overriding response MIME type.

RESOLVED FIXED in mozilla38

Status

()

defect
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: yarik.sheptykin, Assigned: yarik.sheptykin, Mentored)

Tracking

unspecified
mozilla38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

User Story

Tests for Http.jsm API must ensure that httpRequest API allows manipulating the MIME-types freely. In particular these two features are used by the Translation module:
 a) Choosing Content-Type different than "application/x-www-form-urlencoded" of POST requests when sending string data.
 b) Setting overrideMimeType on xhr object to get enable parsing response as responseJson and responseXML.

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Http.jsm API needs to be augmented to allow manipulating the MIME-types more freely. In particular this has to be added to make Http.jsm useful for Translation module:
 a) Choose Content-Type of POST requests, which it currently forces to application/x-www-form-urlencoded
 b) Set overrideMimeType to get responseJson and responseXML
(Assignee)

Updated

4 years ago
Assignee: nobody → yarik.sheptykin
(Assignee)

Updated

4 years ago
Blocks: 1012532
(Assignee)

Comment 1

4 years ago
Hey, Felipe!

My apologies, for taking this long for the fix for bug 1012532!
I have been working on the first part of that problem: augmenting the httpRequest API. I created this bug to commit the changes to httpRequest separately, as you suggested. I, however, felt like features a) and b) are somewhat related as they both unlock more freedom in working with MIME-types. So I put them both in one bug report.
Attachment #8530851 - Flags: review?(qamail.felipe)
(Assignee)

Updated

4 years ago
Mentor: qamail.felipe → felipc
(Assignee)

Updated

4 years ago
Attachment #8530851 - Flags: review?(qamail.felipe) → review?(felipc)
Comment on attachment 8530851 [details] [diff] [review]
Extending API to allow setting custom MIME type for POST and overriding response MIME type.

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

Hey there, this looks great! And yeah, it makes total sense to put these two things in the same patch.

r+ from me, no changes to be made. But I'll also ask Florian to review it since he wrote Http.jsm
Attachment #8530851 - Flags: review?(florian)
Attachment #8530851 - Flags: review?(felipc)
Attachment #8530851 - Flags: review+
Comment on attachment 8530851 [details] [diff] [review]
Extending API to allow setting custom MIME type for POST and overriding response MIME type.

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

Overall these changes look good (and thanks for expanding this code to handle more cases!) There's a few nits that should be fixed before this is committed, however. I also have a couple of questions.

::: toolkit/modules/Http.jsm
@@ +26,3 @@
>   *          parameters: the error, the responseText and the XHR object.
>   *  logger, an object that implements the debug and log methods (e.g. log.jsm).
> + *  overrideMimeType, overrides the MIME type returned by the server with this 

Nit: Trailing white space.

@@ +82,4 @@
>    if (aOptions.headers) {
>      aOptions.headers.forEach(function(header) {
>        xhr.setRequestHeader(header[0], header[1]);
> +      if(header[0] == "Content-Type") {

Nit: Space between if and (

@@ +96,4 @@
>    // Handle adding postData as defined above.
>    let POSTData = aOptions.postData || "";
>    if (Array.isArray(POSTData)) {
> +    if(!isContentTypeSet) {

Nit: Space between if and (.

@@ +97,5 @@
>    let POSTData = aOptions.postData || "";
>    if (Array.isArray(POSTData)) {
> +    if(!isContentTypeSet) {
> +      xhr.setRequestHeader("Content-Type",
> +                           "application/x-www-form-urlencoded; charset=utf-8");

Is this really not done automatically? That surprises me that we have to add this manually. Are we sure this won't break any current callers of this code?
Attachment #8530851 - Flags: review-
(In reply to Iaroslav Sheptykin from comment #0)
> Http.jsm API needs to be augmented to allow manipulating the MIME-types more
> freely. In particular this has to be added to make Http.jsm useful for
> Translation module:
>  a) Choose Content-Type of POST requests, which it currently forces to
> application/x-www-form-urlencoded

Http.jsm only forces the content type if the post data is an Array, and it serializes it automatically. If the provided postdata is a string, the content type isn't touched. Does the translation code really need provides an array of data to post?

>  b) Set overrideMimeType to get responseJson and responseXML

Can you remind me what the use case is for this?
(In reply to Patrick Cloke [:clokep] from comment #3)

> > +      xhr.setRequestHeader("Content-Type",
> > +                           "application/x-www-form-urlencoded; charset=utf-8");
> 
> Is this really not done automatically? That surprises me that we have to add
> this manually.

Definitely not when we are POSTing a string. If we were using the new https://developer.mozilla.org/en-US/docs/Web/API/FormData API that didn't exist at the time we wrote this code, the Content-Type header may be set automatically.
(Assignee)

Comment 6

4 years ago
Hi, Florian!

Thanks for taking a closer look on the requirements.

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> Http.jsm only forces the content type if the post data is an Array, and it
> serializes it automatically. If the provided postdata is a string, the
> content type isn't touched. 

True! Now, after taking a closer look at the code and re-reading the comments it is clear. Maybe, I could add a couple of comments to the code to make this behavior more obvious?

> content type isn't touched. Does the translation code really need provides
> an array of data to post?

To my understanding the translation component wants to send a UTF8-encoded XML string (http://mxr.mozilla.org/mozilla-central/source/browser/components/translation/BingTranslator.jsm#325). So the MIME-type should not be changed in this case. 

> >  b) Set overrideMimeType to get responseJson and responseXML
> 
> Can you remind me what the use case is for this?

As far as I understand, the use case for b) is in BingTranslator's response handling chain. (http://mxr.mozilla.org/mozilla-central/source/browser/components/translation/BingTranslator.jsm#180). The module parses the response body itself whereas this could be done by xhr. If XHR is capable of parsing XML and is going to do it in future than it might be reasonable to let it do that. However, xhr parses the content only for certain MIME-types set by the server. I don't know what MIME-type Bing server uses for sending XML documents but I trust :felipe (see bug 1012532) that translation has to override it with text/xml.
(Assignee)

Comment 7

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #3)

Thanks for detailed review, Patrick! I will wait for the discussion to finish before updating the patch.
(In reply to Iaroslav Sheptykin from comment #6)

> > >  b) Set overrideMimeType to get responseJson and responseXML
> > 
> > Can you remind me what the use case is for this?
> 
> As far as I understand, the use case for b) is in BingTranslator's response
> handling chain.
> (http://mxr.mozilla.org/mozilla-central/source/browser/components/
> translation/BingTranslator.jsm#180). The module parses the response body
> itself whereas this could be done by xhr. If XHR is capable of parsing XML
> and is going to do it in future than it might be reasonable to let it do
> that. However, xhr parses the content only for certain MIME-types set by the
> server. I don't know what MIME-type Bing server uses for sending XML
> documents but I trust :felipe (see bug 1012532) that translation has to
> override it with text/xml.

Ok. The reason I was asking is I was wondering if we could make the API nicer. Maybe with an expected response type that would have the value "plain" (by default), "json" or "xml". Just a thought at this point, not sure if it would be a real improvement.
(Assignee)

Comment 9

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #8)

> Ok. The reason I was asking is I was wondering if we could make the API
> nicer. Maybe with an expected response type that would have the value
> "plain" (by default), "json" or "xml". Just a thought at this point, not
> sure if it would be a real improvement.

Interesting! Do you suggest to add option expectedResponse of "plain" (default), "json", or "xml" instead of overrideMimeType? Then, when delegating the response to the handler (http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#71) pass an object according to the expectedResponse instead of always passing the response body?

A completely different idea could be to add a callback option "beforeSend" of function(XHRObject). And let the API user do what they need to do (maybe even cancel) with the XHRObject before the request is sent. This could be a general solution for several edge cases.
(In reply to Iaroslav Sheptykin from comment #9)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)
> 
> > Ok. The reason I was asking is I was wondering if we could make the API
> > nicer. Maybe with an expected response type that would have the value
> > "plain" (by default), "json" or "xml". Just a thought at this point, not
> > sure if it would be a real improvement.
> 
> Interesting! Do you suggest to add option expectedResponse of "plain"
> (default), "json", or "xml" instead of overrideMimeType? Then, when
> delegating the response to the handler
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#71)
> pass an object according to the expectedResponse instead of always passing
> the response body?

Yes. Like I said, it's just a thought at this point, I haven't thought about it enough to decide if it would be a real improvement or not.
So it looks like the first part might not be necessary, as Http.jsm only forces content-type if Array.isArray is true, and it looks like we're passing a string in the request. So we can still set the content-type as wanted. But I need to double check.. Iaroslav, could you try that?

For the second part, having this would be a great improvement, as the translation code right now has to call the xml parsing manually after all of the content is received, but if this is tied to the XHR object than it can do a streaming parsing of xml which is a lot more efficient.

I don't have any particular opinion on which API would look nicer. Maybe we can try both and compare? Patrick and Florian are the owners of this file so I'll let the final decision up to them. But in reality I think either choice is good enough so perhaps we shouldn't add too much complexity right now for this single use case.
Felipe, don't forget that you can manually modify things on the XHR object after you call httpRequest, a la [1]. I think the two things in comment 0 can already be done though this way. If we can easily add this by adding to the options, we should make the API clearer. (In fact I think there are lots of things we can do to improve httpRequest, e.g. return a promise...).

[1] http://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#690 (signAndSend calls httpRequest and then returns the result).
(Assignee)

Comment 13

4 years ago
(In reply to :Felipe Gomes (behind on reviews until the end of the week) from comment #11)

> So it looks like the first part might not be necessary, as Http.jsm only
> forces content-type if Array.isArray is true, and it looks like we're
> passing a string in the request. So we can still set the content-type as
> wanted. But I need to double check.. Iaroslav, could you try that?

Sure! I will modify the test I have already created to cover this.
(Assignee)

Comment 14

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #12)

> Felipe, don't forget that you can manually modify things on the XHR object
> after you call httpRequest, a la [1]. I think the two things in comment 0
> can already be done though this way. If we can easily add this by adding to
> the options, we should make the API clearer. (In fact I think there are lots
> of things we can do to improve httpRequest, e.g. return a promise...).
> 
> [1]
> http://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.
> js#690 (signAndSend calls httpRequest and then returns the result).

Thanks for the example, Patrick! I first naively assumed that after sending request there is nothing you can do to affect its processing. I will write a tests for both cases from comment 0 and if they pass, I will remove blocking of bug 1012532.
(Assignee)

Comment 15

4 years ago
Hi, Patrick!

I modified the tests to check if the current httpRequest covers the cases from comment 0. All tests in test_Http.js pass on my Ubuntu machine with the firefox build from Monday. Could you please see if I tested correctly? I would push this patch to the try server then.
Attachment #8530851 - Attachment is obsolete: true
Attachment #8530851 - Flags: review?(florian)
Attachment #8532016 - Flags: review?(clokep)
(Assignee)

Comment 16

4 years ago
Hi All!

Short update. I pushed the changes to the test server. Here is the link:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6913205a5918

No idea why tryserver found so many (35) chang-sets in my repo but the patch is on top.
(In reply to Iaroslav Sheptykin from comment #16)

> Short update. I pushed the changes to the test server. Here is the link:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6913205a5918

Looks green! :)
(Assignee)

Comment 18

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #17)

> Looks green! :)

Yes! All X tests passed on Linux, Win, and Mac. I guess it is safe to decouple this bug from bug 1012532. It appears httpRequest never blocked the development of the translation API. Thanks you guys, for pointing to the proper use of the API!

Regarding the ideas for making the API nicer, I would be ready to implement them as soon as they get more mature. We could keep this bug open for that sake or mark as "works for me". Maybe :felipe could decide?

Is there a documentation page on MDN about httpRequest? I could not find one. I could update it with the examples of API use, which we collected here.
(In reply to Iaroslav Sheptykin from comment #18)
> We could keep this bug open for that
> sake or mark as "works for me".

Or rephrase the summary of the bug to cover what the patch here actually does (ie. improve test coverage). Would be strange to land a patch and mark the bug works for me at the same time :-).

> Is there a documentation page on MDN about httpRequest? I could not find
> one. I could update it with the examples of API use, which we collected here.

I'm afraid there isn't one. Creating one would probably be appreciated though.
(Assignee)

Updated

4 years ago
No longer blocks: 1012532
(Assignee)

Updated

4 years ago
Summary: Extend httpRequest API to allow setting custom MIME type for POST and overriding response MIME type. → Improving test coverage to ensure httpRequest API allows setting custom MIME type for POST and overriding response MIME type.
(Assignee)

Updated

4 years ago
User Story: (updated)
(Assignee)

Updated

4 years ago
User Story: (updated)
Comment on attachment 8532016 [details] [diff] [review]
Ensuring that API allows setting custom MIME type for POST and overriding response MIME type.

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

Sorry for taking so long to get back to this review.

Overall, I think this looks really good! Most of the changes I've requested are nits or reorganization changes. I did have one question below though.

Thanks for adding these tests! Should be much clearer for consumers to see how to use this code. :) My next review cycle should be much faster now that I've reacquainted myself with this code.

::: toolkit/modules/tests/xpcshell/test_Http.js
@@ +154,5 @@
>    run_next_test();
>  });
>  
> +/**
> + * Makes sure that httpRequest Api allows setting a custom Content-Type header

Nit: API, all caps.

@@ +161,5 @@
> +add_test(function test_CustomContentTypeOnPost() {
> +  do_test_pending();
> +
> +  // Preparing the request parameters.
> +  const kData = JSON.stringify(kPostDataSent);

Good idea to reuse this! :) Don't make this a separate variable though, just inline it below in the options object.

@@ +179,5 @@
> +    headers: [['Content-Type', "application/json"]]
> +  }
> +
> +  // Registering new request handler.
> +  const kJsonPostPath = "/json_post";

Please put this at the top of the file with the other constants.

@@ +180,5 @@
> +  }
> +
> +  // Registering new request handler.
> +  const kJsonPostPath = "/json_post";
> +  server.registerPathHandler(kJsonPostPath, function(aRequest, aResponse) {

Please do this in the same way the other path handlers are registered (inside of run_test).

@@ +181,5 @@
> +
> +  // Registering new request handler.
> +  const kJsonPostPath = "/json_post";
> +  server.registerPathHandler(kJsonPostPath, function(aRequest, aResponse) {
> +    // Checking if the Content-Type is still application/json

Nit: End comments with a period (.).

@@ +182,5 @@
> +  // Registering new request handler.
> +  const kJsonPostPath = "/json_post";
> +  server.registerPathHandler(kJsonPostPath, function(aRequest, aResponse) {
> +    // Checking if the Content-Type is still application/json
> +    let contentType = aRequest.getHeader("Content-Type");

Let's just add another parameter to getDataChecker which checks the content type. We should be doing this anyway to test the code around http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#86

@@ +222,5 @@
> +      do_check_true(false);
> +      do_test_finished();
> +      run_next_test();
> +    },
> +    postData: null

Does this explicitly have to be null?

@@ +228,5 @@
> +
> +  // Firing the request.
> +  let xhr = httpRequest(kGetUrl, options);
> +
> +  // Telling xhr to use this MIME type for the responses.

Can you reword this to "Override the response MIME type."
Attachment #8532016 - Flags: review?(clokep) → review-
(Assignee)

Comment 21

4 years ago
Hi, Patrick! Thanks for the detailed review! Sorry, for taking this long to update the patch. I had to shift partitions on my hard drive to get more space for firefox.
Attachment #8532016 - Attachment is obsolete: true
Attachment #8537269 - Flags: review?(clokep)
(Assignee)

Comment 22

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #19)

Hi, Florian!

> I'm afraid there isn't one. Creating one would probably be appreciated
> though.

Before creating the MDN page I created a little document on GoogleDocs [1] with a little text on how to use the httpRequest API. As soon as Patrick approves the patch I will extend it with some examples. 
If you find the document reasonable I will publish it on MDN and hope it will be corrected/extended. How do you find it?

1. https://docs.google.com/document/d/1LK-SHUBuHANNXPfGtzFfSOeCfUjQNAco-tBDPxh_AnM/edit?usp=sharing
Flags: needinfo?(florian)
Comment on attachment 8537269 [details] [diff] [review]
Ensuring that API allows setting custom MIME type for POST and overriding response MIME type.

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

A few really minor nits and this will be good to go! I'd just like a couple of variables renamed and comments reworded. Looks like the next version will be all set!

::: toolkit/modules/tests/xpcshell/test_Http.js
@@ +21,5 @@
> +const kPostMimeTypeReceived = "application/x-www-form-urlencoded; charset=utf-8";
> +
> +const kJsonPostPath = "/json_post";
> +const kJsonPostUrl = kBaseUrl + kJsonPostPath;
> +const kJsonPostDataReceived = JSON.stringify(kPostDataSent);

Let's just call this kJsonPostData since it's the data that's both sent AND received. (Same thing with the kJsonPostMimeTypeReceived, leave off "Received".)

@@ +184,5 @@
> +      do_check_true(false);
> +      do_test_finished();
> +      run_next_test();
> +    },
> +    postData: JSON.stringify(kPostDataSent),

Use the constant from above here.

@@ +206,5 @@
> +  let options = {
> +    onLoad: function(aResponse, xhr) {
> +      do_check_eq(aResponse, "Success!");
> +
> +      // Make sure that the server set a different MIME-type.

Nit: "set the expected MIME-type"

@@ +207,5 @@
> +    onLoad: function(aResponse, xhr) {
> +      do_check_eq(aResponse, "Success!");
> +
> +      // Make sure that the server set a different MIME-type.
> +      let reportedMimeType = xhr.getResponseHeader('Content-Type');

Nit: Please use double quotes throughout this code, this matches the older code that was in this file.

@@ +211,5 @@
> +      let reportedMimeType = xhr.getResponseHeader('Content-Type');
> +      do_check_neq(reportedMimeType, kMimeType);
> +
> +      // If overriding mime type succeeded, xhr should had tried to parse the
> +      // response as XML and the responseXML is not null.

Nit: Please  reword this slight: "If overriding the MIME type succeeded, an attempt is made to parse the response as XML. Ensure that responseXML is not null."
Attachment #8537269 - Flags: review?(clokep) → review-
(Assignee)

Comment 24

4 years ago
Hi, Patrick!

Sorry for a long time out. I was busy packing and unpacking Christmas presents. Thanks for taking a close look on my last patch. I updated the test code according to your comments. The tests still pass locally. I will push the patch to the tryserver as soon as I can.
Attachment #8537269 - Attachment is obsolete: true
Attachment #8547203 - Flags: review?(clokep)
(Assignee)

Comment 25

4 years ago
Update:
Here is a link to the try-server patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af2b3d88eb3
Comment on attachment 8547203 [details] [diff] [review]
Ensuring that API allows setting custom MIME type for POST and overriding response MIME type.

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

This looks good to me! Thanks! Looks like there's a couple of failing xpcshell tests in the try run, but they don't seem related. This needs a review (or rubberstamp) from a toolkit reviewer, as I'm not one.
Attachment #8547203 - Flags: review?(clokep) → review+
(Assignee)

Comment 27

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #26)

Thanks for the fast review, Patrick!

> This looks good to me! Thanks! Looks like there's a couple of failing
> xpcshell tests in the try run, but they don't seem related. This needs a
> review (or rubberstamp) from a toolkit reviewer, as I'm not one.

How do I find the toolkit reviewer? Or how should I proceed with closing this bug?
Flags: needinfo?(clokep)
Comment on attachment 8547203 [details] [diff] [review]
Ensuring that API allows setting custom MIME type for POST and overriding response MIME type.

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

Generally reviewers can be found from https://wiki.mozilla.org/Modules/All#Toolkit or by looking at a file and seeing who had previously reviewed changes in it. You can also click on the "suggested reviewers" link in Bugzilla, although that's not always accurate. I've asked Dave Townsend/Mossop to review or rubberstamp this.
Attachment #8547203 - Flags: review?(dtownsend)
Flags: needinfo?(clokep)
 Iaroslav, does this still needinfo from Florian?
(Assignee)

Comment 30

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #29)
>  Iaroslav, does this still needinfo from Florian?

No, I guess we can remove this pending need-info request. I was related to the idea of documenting the use-cases of httpRequest API. I created a short text document for it. I will extend it with the examples of the test-cases from this bug and submit to MDN. We can let them decide if there is a place for it on MDN.
Flags: needinfo?(florian)
(In reply to Iaroslav Sheptykin from comment #22)

> If you find the document reasonable I will publish it on MDN and hope it
> will be corrected/extended. How do you find it?

I just read it and made a few edits. That's a great starting point, and in my opinion totally worth adding to MDN. Then indeed you or others can continue to extend it :-).
(Assignee)

Comment 32

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> (In reply to Iaroslav Sheptykin from comment #22)
> 
> > If you find the document reasonable I will publish it on MDN and hope it
> > will be corrected/extended. How do you find it?
> 
> I just read it and made a few edits. That's a great starting point, and in
> my opinion totally worth adding to MDN. Then indeed you or others can
> continue to extend it :-).

Thanks, Florian! I will try to get it published!
Attachment #8547203 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 33

4 years ago
Looks like we can check in this patch. Should I mark it check-in needed? If I remember correctly, I have to add [check-in] to the whiteboard, right?
(In reply to Iaroslav Sheptykin from comment #33)
> Looks like we can check in this patch. Should I mark it check-in needed? If
> I remember correctly, I have to add [check-in] to the whiteboard, right?

You put the checkin-needed value in the "Keywords" field.

Sheriffs will likely ask you for a link to try server results with the patch; do you have try server access?
(Assignee)

Comment 35

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> You put the checkin-needed value in the "Keywords" field.

Thanks, Florian!

> Sheriffs will likely ask you for a link to try server results with the
> patch; do you have try server access?

I pushed the patch to the try-server a couple of days ago. The patch hasn't been modified since.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af2b3d88eb3
Although some tests failed, Dave Townsend confirmed that it was not related to this patch. At least this is how I interpret his review+ .
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(In reply to Iaroslav Sheptykin from comment #35)
> I pushed the patch to the try-server a couple of days ago. The patch hasn't
> been modified since.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af2b3d88eb3
> Although some tests failed, Dave Townsend confirmed that it was not related
> to this patch. At least this is how I interpret his review+ .

Sorry, I don't share your optimism. Please post a link to a green Try run.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #36)
> (In reply to Iaroslav Sheptykin from comment #35)
> > I pushed the patch to the try-server a couple of days ago. The patch hasn't
> > been modified since.
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af2b3d88eb3
> > Although some tests failed, Dave Townsend confirmed that it was not related
> > to this patch. At least this is how I interpret his review+ .
> 
> Sorry, I don't share your optimism. Please post a link to a green Try run.

Yeah sorry but my r+ was for the patch I had not looked at the try results. Something is definitely broken there, possible it is unrelated to your patch so I'd try applying it on a newer known green m-c revision and pushing again.
(Assignee)

Comment 38

4 years ago
Hi there!

(In reply to Dave Townsend [:mossop] from comment #37)
> so I'd try applying it on a newer known green m-c revision and pushing again.

Thanks for the hint, Dave! I did just that! I have re-pushed the same patch to the try server and the tests look alright to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=171762de8b5b

One test failed. But this seems to be a known issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1102923

Do you think I can add another checkin-needed?
(In reply to Iaroslav Sheptykin from comment #38)
> Hi there!
> 
> (In reply to Dave Townsend [:mossop] from comment #37)
> > so I'd try applying it on a newer known green m-c revision and pushing again.
> 
> Thanks for the hint, Dave! I did just that! I have re-pushed the same patch
> to the try server and the tests look alright to me:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=171762de8b5b
> 
> One test failed. But this seems to be a known issue:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1102923
> 
> Do you think I can add another checkin-needed?

Yes. Thanks for pushing it to try again :-).
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
You ran the tests on debug and they failed, and you ran them on opt and they're passing. I haven't seen evidence yet that the original failures are actually gone.
Keywords: checkin-needed
(Assignee)

Comment 41

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #40)
> You ran the tests on debug and they failed, and you ran them on opt and
> they're passing. I haven't seen evidence yet that the original failures are
> actually gone.

Oh! Sorry, my fault!
Here are the results of testing with the debug build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=716477c325ef

Some tests failed again. This time there are tests of social sharing. It seems like this problem is unrelated to our patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=1115131
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a510d4f839bf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 44

4 years ago
Hey everyone! I created a new article at MDN for Http.jsm. 
https://developer.mozilla.org/en-US/docs/Http.jsm
You need to log in before you can comment on or make changes to this bug.