Closed Bug 1162677 Opened 9 years ago Closed 9 years ago

Showing JSON URLs without a basename becomes just '/' in File column

Categories

(DevTools :: Netmonitor, defect, P2)

36 Branch
defect

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: peterbe, Assigned: jsnajdr, Mentored)

References

Details

(Whiteboard: [polish-backlog][good first bug][lang=javascript])

Attachments

(2 files, 3 obsolete files)

See attached screenshot. 

The AJAX calls to /new/api/XXX/video/ shows in the Network Monitor as just `/`.
This isn't very helpful. Especially as in my case it loads /new/api/0001/video/ and /new/api/0002/video/ and /new/api/0003/video/ etc. 

Hovering over the little / just says "/" in the tooltip.
Note, in Chrome it shows as "video/" for the above mentioned URLs. 

Also, Chrome displays the whole full URL in the tooltip.
(In reply to Peter Bengtsson [:peterbe] from comment #1)
> Note, in Chrome it shows as "video/" for the above mentioned URLs. 
> 
> Also, Chrome displays the whole full URL in the tooltip.

Jeff, what do you think?

I also think it should show more context if the only thing that's going to show up in the File column is '/' (unless of course the request is made to the root of the domain).  Maybe go back to the previous '/' in the URL and show as chrome is doing.

I think that the tooltip when hovering the name should also include the entire URL.
Flags: needinfo?(jgriffiths)
Summary: Showing JSON URLs without a basename becomes just / → Showing JSON URLs without a basename becomes just '/' in File column
(In reply to Brian Grinstead [:bgrins] from comment #2)
...
> Jeff, what do you think?
> 
> I also think it should show more context if the only thing that's going to
> show up in the File column is '/' (unless of course the request is made to
> the root of the domain).  Maybe go back to the previous '/' in the URL and
> show as chrome is doing.

I think we should show the entire path, eg 

document.location.pathname + document.location.search

> I think that the tooltip when hovering the name should also include the
> entire URL.

Yup, agreed.

Adding devedition-40 flag and p2. Is this a good first bug?
Flags: needinfo?(jgriffiths) → needinfo?(bgrinstead)
Priority: -- → P2
Whiteboard: [devedition-40]
(In reply to Jeff Griffiths (:canuckistani) from comment #3)
> Adding devedition-40 flag and p2. Is this a good first bug?

Forwarding question to :janx - who would also be a good mentor if it is :)
Flags: needinfo?(bgrinstead) → needinfo?(janx)
I also think this qualifies as a good first bug, and I'm willing to mentor it. Leaving needinfo; I'll decide whether to fix or mentor it shortly.
To fix this Network Monitor bug, I think it would be great to match Chrome's behavior, described in comment 1.

The code that needs to be changed lives in netmonitor-view.js: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js (likely inside the "updateMenuView" and "_getUriNameWithQuery" functions).

I'll mentor this bug. For any questions, please flag me as "needinfo".
Mentor: janx
Flags: needinfo?(janx)
Whiteboard: [devedition-40] → [devedition-40][good first bug][lang=javascript]
I can fix this. Firebug has a very nice behavior - show the whole path if there is no filename, show the full URL (including protocol and host) on mouseover.
Implemented Firebug behavior for the File column:
- if the URL has a fileName, show just the fileName, without the path
- if there is no fileName (happens for Ajax API calls), show the whole path (e.g., /new/api/003/video/)
- the tooltip shows the whole URL

Any tests need to be written for this?
Attachment #8604649 - Flags: review?(janx)
Thanks for taking the time to fix this, Jarda! I'll have a look at your patch now.
Assignee: nobody → jsnajdr
Status: NEW → ASSIGNED
Comment on attachment 8604649 [details] [diff] [review]
show nicer JSON URLs in netmonitor

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

Your patch looks and works great! Just a few tweaks, and you're good to go. Again, thanks a lot for fixing this bug so quickly :)

I think it's a good idea to show the full path (e.g. /new/api/0003/video/) like in Firebug, instead of just the folder name (e.g. video/) like in Chrome. And both tools show the full URL in the tooltip, so we should do the same. Jeff, do you agree with the suggested behavior?

Jarda, since your code changes the expected tooltip, you will also need to update test/head.js: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/head.js (the expected item name is at line 278, and the expected tooltip is at both lines 298 and 303). To see if your patch clears the tests, you can try running:

./mach mochitest-devtools browser/devtools/netmonitor/test/browser_net_status-codes.js

Also, this fix doesn't actually have a lot to do with JSON: Please update the patch name and description, and add "r=janx" after the description (our usual format is "Bug xxxxxx - Description of the change. r=reviewer").

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1423,5 @@
>          } catch(e) {
>            break; // User input may not make a well-formed url yet.
>          }
>          let nameWithQuery = this._getUriNameWithQuery(uri);
> +        let pathWithQuery = this._getUriPathWithQuery(uri);

I don't think it's useful to create an extra "_getUriPathWithQuery" function. Instead, here you can simply use "let pathWithQuery = NetworkHelper.convertToUnicode(unescape(uri.spec))". And maybe it would make sense to rename "pathWithQuery" to "unicodeSpec"? Not sure, you decide.

@@ +1911,4 @@
>      let query = NetworkHelper.convertToUnicode(unescape(aUrl.query));
>      return name + (query ? "?" + query : "");
>    },
> +  _getUriPathWithQuery: function(aUrl) {

As previously mentioned, I don't think it's useful to create a "_getUriPathWithQuery" function here.
Attachment #8604649 - Flags: review?(janx) → feedback+
Jarda, I've reviewed your patch with a few minor remarks. Please address them, and upload a new version of your patch whenever you want, so I can review it again.

Jeff, needinfo for my question in comment 10.
Flags: needinfo?(jsnajdr)
Flags: needinfo?(jgriffiths)
(In reply to Jan Keromnes [:janx] from comment #10)
...
> I think it's a good idea to show the full path (e.g. /new/api/0003/video/)
> like in Firebug, instead of just the folder name (e.g. video/) like in
> Chrome. And both tools show the full URL in the tooltip, so we should do the
> same. Jeff, do you agree with the suggested behavior?

I agree - let's show the full path.
Flags: needinfo?(jgriffiths)
Attached patch show nicer URLs in netmonitor (obsolete) — Splinter Review
Fixed issues pointed out by review:
- changed the name of the commit (removed JSON, added r= flag)
- removed method _getUriPathWithQuery, assigning directly to variable unicodeUrl (used also by the _setStatus method)
- fixed the status-codes test, everything passes now

What is missing:
I'd like to add a new test that tests the new URL formatting behavior. But I'm a bit overwhelmed here: should
I add a new case to existing test file? Create a completely new test? I need some advice.
Attachment #8605252 - Flags: review?(janx)
Attachment #8604649 - Attachment is obsolete: true
Flags: needinfo?(jsnajdr)
Thanks Jarda! I'll have a look now, and give you some pointers for the test.
Comment on attachment 8605252 [details] [diff] [review]
show nicer URLs in netmonitor

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

Looks great!

> - fixed the status-codes test, everything passes now

It does, thanks!

> What is missing:
> I'd like to add a new test that tests the new URL formatting behavior. But
> I'm a bit overwhelmed here: should
> I add a new case to existing test file? Create a completely new test? I need
> some advice.

Well, the changes you made to head.js already verify the format of many item names and tooltips, because it's called in many netmonitor tests.

However, I'm not sure that a request is being made to a URL ending with a "/" anywhere in these tests, and I have a suspicion that your change in head.js won't work with unicode URLs (I believe the expected `aURL` won't be unicode). To ensure everything works right, please add a new test, composed of:

1. A new HTML page (called something like "test/html_api-calls.html") that does XHR requests to an imaginary API (e.g. to "/api/new/" and something with unicode like "/api/☃/", they'll fail with 404 but that's not a problem). You can get inspiration from test/html_params-test-page.html, and add your new file to both head.js and browser.ini.

2. A new JS file that loads the HTML page and verifies that the items' name and tooltip look OK, with inspiration from test/browser_net_copy_params.js. Also add this to browser.ini.

::: browser/devtools/netmonitor/test/head.js
@@ +292,1 @@
>    }

You'll probably also want to update "let name = ..." above.
Attachment #8605252 - Flags: review?(janx) → feedback+
Jarda, I've reviewed your patch. Let me know when you get a chance to have a look, or if you have any questions.
Flags: needinfo?(jsnajdr)
I've been working on the test today. When I'm finished with the Unicode stuff, I'll submit a new version of the patch.

I'm not sure which "let name = ..." you mean - the one in verifyRequestItemTarget?
Flags: needinfo?(jsnajdr)
Attached patch show nicer URLs in netmonitor (obsolete) — Splinter Review
Here is a new version of the patch including a new test: browser_net_api-calls.js

- it tests if the file and its tooltip are displayed correctly for the API-like URLs
- it also contains several cases that test if Unicode characters in URL behave nicely

The rules for Unicode are:
- in the data (for example, the aUrl variable), the URL is always UTF-8 encoded and %-escaped, e.g., "file%E2%98%A2.xml" contains the U+2622 character (radioactive sign)
- in the view, we always unescape the URL and convert it to Unicode string, so you see "file☢.xml" everywhere and we expect this value in the test

The test fails at this moment, because I can't use the NetworkHelper.convertToUnicode function in the test - it's available in the browser code (netmonitor-view.js), but not in the test code (head.js). The verifyRequestItemTarget function in head.js now only calls unescape() on all the strings that need to be converted, but the next step that needs to be added is conversion from UTF-8 to UTF-16. This is not happening now and the string comparison fails. Any advice?
Attachment #8606258 - Flags: review?(janx)
Attachment #8605252 - Attachment is obsolete: true
Comment on attachment 8606258 [details] [diff] [review]
show nicer URLs in netmonitor

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

Almost there! I suggested a few changes below:

::: browser/devtools/netmonitor/test/browser_net_api-calls.js
@@ +13,5 @@
> +    let { RequestsMenu, NetworkDetails } = NetMonitorView;
> +
> +    RequestsMenu.lazyUpdate = false;
> +
> +    Task.spawn(function () {

`Task.spawn` is generally called with a generator (`function*() {}`) in order for `yield` to work. Your code seems to work without the star, but please add it back, just for consistency (in the netmonitor tests, occurrences with a star are twice as common as occurrences without).

@@ +20,5 @@
> +      verifyRequestItemTarget(RequestsMenu.getItemAtIndex(0), "GET", "http://example.com/api/fileName.xml");
> +      verifyRequestItemTarget(RequestsMenu.getItemAtIndex(1), "GET", "http://example.com/api/file%E2%98%A2.xml");
> +      verifyRequestItemTarget(RequestsMenu.getItemAtIndex(2), "GET", "http://example.com/api/ascii/get/");
> +      verifyRequestItemTarget(RequestsMenu.getItemAtIndex(3), "GET", "http://example.com/api/unicode/%E2%98%A2/");
> +      verifyRequestItemTarget(RequestsMenu.getItemAtIndex(4), "GET", "http://example.com/api/search/?q=search%E2%98%A2");

Nit: Not very important, but there is a lot of repeated code here. Please create a helper function outside the `Task.spawn`, and call it with every URL here.

::: browser/devtools/netmonitor/test/head.js
@@ +10,3 @@
>  let { gDevTools } = Cu.import("resource:///modules/devtools/gDevTools.jsm", {});
>  let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>  let { CurlUtils } = Cu.import("resource:///modules/devtools/Curl.jsm", {});

Here you can import NetworkHelper by using:

    let NetworkHelper = devtools.require("devtools/toolkit/webconsole/network-helper");

This will allow you to use `NetworkHelper.convertToUnicode(unescape(url))` in test code.

@@ +278,5 @@
>          transferred, size, time, fromCache } = aData;
>    let { attachment, target } = aRequestItem
>  
>    let uri = Services.io.newURI(aUrl, null, null).QueryInterface(Ci.nsIURL);
> +  let unicodeUrl = unescape(aUrl);

With NetworkHelper imported above, you'll be able to use `NetworkHelper.convertToUnicode` here.

@@ +281,5 @@
>    let uri = Services.io.newURI(aUrl, null, null).QueryInterface(Ci.nsIURL);
> +  let unicodeUrl = unescape(aUrl);
> +  let name = unescape(uri.fileName || uri.filePath || "/");
> +  let query = unescape(uri.query);
> +  let hostPort = unescape(uri.hostPort);

I'm not sure you need to unescape `hostPort`.
Attachment #8606258 - Flags: review?(janx) → feedback+
Fixed the following:
- import and use NetworkHelper.convertToUnicode in verifyRequestItemTarget
- rewrite the api-calls test into a forEach loop
- fixed the function* syntax in browser_net_api-calls.js and also in browser_net_copy_params.js - I originally copied the test skeleton from here.
Attachment #8606677 - Flags: review?(janx)
Attachment #8606258 - Attachment is obsolete: true
Thanks Jarda! I'll have a look at your patch tomorrow.
Comment on attachment 8606677 [details] [diff] [review]
show nicer URLs in netmonitor

Looks good to me!

I took the liberty to submit your patch to try, out test infrastructure, to see how your patch behaves in all devtools tests on most platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5373c61d961f

The command I used for this is `git push-to-try` from the moz-git-tools, but there are similar tools for mercurial.

Once the results are in, and there are no failures (red/orange) related to your patch, feel free to add the `checkin-needed` keyword to this bug. This will ask sheriffs to land your patch in nightly.
Attachment #8606677 - Flags: review?(janx) → review+
Adding 'checkin-needed'. Linux debug tests (dt1,dt2) failed due to some timeout, not related to the patch.
Keywords: checkin-needed
Congratulations on your first patch in Firefox!

Thanks a lot Jarda for taking the time to figure out how we do things, and for fixing an important Developer Tools bug! (Arguably two bugs even).

Your patch was integrated in the fx-team development branch, which is regularly merged into mozilla-central, the branch from which our Nightly versions are taken, and every 6 weeks a snapshot of it is taken to Aurora ("Developer Edition"), then Beta, and finally the stable version that most people use. If all goes well, your patch should be in Developer Edition in about 5 weeks (we just started a new cycle), and in stable in about 4 months.
Jeff, do we want to uplift this patch to 40?
Flags: needinfo?(jgriffiths)
https://hg.mozilla.org/mozilla-central/rev/9c0ebc0bdf00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(In reply to Jan Keromnes [:janx] from comment #26)
> Jeff, do we want to uplift this patch to 40?

Yes please!
Flags: needinfo?(jgriffiths)
Comment on attachment 8606677 [details] [diff] [review]
show nicer URLs in netmonitor

Please uplift this small, high-value polish improvement.

[Feature/regressing bug #]: None.

[User impact if declined]: Users will continue to be confused by Ajax-calls named "/", and they won't be able to know the more precise URL when hovering (this will also just show "/").

[Describe test coverage new/current, TreeHerder]: This change comes with an additional devtools mochitest, which also improves netmonitor coverage (adds requests to URLs ending with "/" and containing unicode).

[Risks and why]: Small, it's a stand-alone code change.

[String/UUID change made/needed]: None.
Attachment #8606677 - Flags: approval-mozilla-aurora?
Comment on attachment 8606677 [details] [diff] [review]
show nicer URLs in netmonitor

Let's do it!
Attachment #8606677 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [devedition-40][good first bug][lang=javascript] → [polish-backlog][good first bug][lang=javascript]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.