859980 - JSON request not parsed

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: json tree is empty

If I request a JSON resource, I expect the results to be parsed into a tree, but this isn't currently happening.

Sample urls:

* http://live-menu.staugustinesvancouver.com/taps.json
* https://api.twitter.com/1/related_results/show/308926577822007296.json?include_entities=1

Comment 1

[Victor Porof [:vporof][:vp]]

LongStrings are special.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1

This fixes the problem. I'm too sleepy to write tests.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v2

With tests.

Comment 4

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 735661 [details] [diff] [review]
v2

And this.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 735661 [details] [diff] [review]
v2

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

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +478,5 @@
> +   * @return object Promise
> +   *         A promise that is resolved when the full string contents
> +   *         are available, or rejected if something goes wrong.
> +   */
> +  getString: function NEH_getString(aStringGrip) {

I didn't think we needed these function signatures anymore? Surprised to see them in a new file, but old habits and their dying, etc.

@@ +481,5 @@
> +   */
> +  getString: function NEH_getString(aStringGrip) {
> +    // Make sure this is a long string.
> +    if (typeof aStringGrip != "object" || aStringGrip.type != "longString") {
> +      return Promise.resolve(aStringGrip); // Go home string, you're drunk.

yes!

@@ +486,5 @@
> +    }
> +    // Fetch the long string only once.
> +    if (aStringGrip._fullText) {
> +      return aStringGrip._fullText.promise;
> +    }

without delving too deeply, when/why do aStringGrip's have ._fullText.promise already populated?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +884,5 @@
>      headersScope.expanded = true;
>  
>      for (let header of aResponse.headers) {
>        let headerVar = headersScope.addVar(header.name, { null: true }, true);
> +      gNetwork.getString(header.value).then((aString) => headerVar.setGrip(aString));

lovin' the new phat arros.

@@ +1016,5 @@
> +    let paramsArray = aParams.replace(/^[?&]/, "").split("&").map((e) =>
> +      let (param = e.split("=")) {
> +        name: NetworkHelper.convertToUnicode(unescape(param[0])),
> +        value: NetworkHelper.convertToUnicode(unescape(param[1]))
> +      });

this is nicer here.

::: browser/devtools/netmonitor/test/sjs_content-type-test-server.sjs
@@ +60,5 @@
>          response.setHeader("Content-Type", "text/html; charset=utf-8", false);
>          response.write("<blink>Not Found</blink>");
>          response.finish();
>          break;
> +      }

what's with the extra { } ?

Comment 7

[popescu.andreea.z]

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)> 
> I didn't think we needed these function signatures anymore? Surprised to see
> them in a new file, but old habits and their dying, etc.
> 

Yeah, hard to lose old habits.

> @@ +486,5 @@
> > +    }
> > +    // Fetch the long string only once.
> > +    if (aStringGrip._fullText) {
> > +      return aStringGrip._fullText.promise;
> > +    }
> 
> without delving too deeply, when/why do aStringGrip's have
> ._fullText.promise already populated?
> 

On a second call to getString with the same long actor grip. This can happen, for example, when examining a network request's response a second time. I would even go as far as to say that this pattern it's one of the many sugary things you can easily do with promises.
> 
> ::: browser/devtools/netmonitor/test/sjs_content-type-test-server.sjs
> @@ +60,5 @@
> >          response.setHeader("Content-Type", "text/html; charset=utf-8", false);
> >          response.write("<blink>Not Found</blink>");
> >          response.finish();
> >          break;
> > +      }
> 
> what's with the extra { } ?

Variable declarations (let and var) in "case:"s are not block scoped, believe it or not (wat!). The extra { } specifically delimit a block and avoid declarations interfering with each other (you actually get a type error if you do:

switch (foo) {
  case 1:
    let a = 42;
    break;
  case 2:
    let a = 43;
    break;
}

)

Comment 8

[popescu.andreea.z]

(In reply to popescu.andreea.z from comment #7)
> 
> Variable declarations (let and var) in "case:"s are not block scoped.

Or, to phrase it more correctly, "case:"s in switch statements don't delimit blocks, and let declarations can't be duplicated in the same block (except in the global scope, again, wat).

To make this more fun, execute this in a Scratchpad:
let a = 1;
let a = 2;

then this:
{
let a = 1;
let a = 2;
}

and then turn off your computer.

Comment 9

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/2e8e4155ec4c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e8e4155ec4c