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)
Tracking
(firefox40 fixed, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: peterbe, Assigned: jsnajdr, Mentored)
References
Details
(Whiteboard: [polish-backlog][good first bug][lang=javascript])
Attachments
(2 files, 3 obsolete files)
1.67 MB,
image/png
|
Details | |
11.08 KB,
patch
|
janx
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Note, in Chrome it shows as "video/" for the above mentioned URLs. Also, Chrome displays the whole full URL in the tooltip.
Comment 2•9 years ago
|
||
(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
Comment 3•9 years ago
|
||
(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]
Comment 4•9 years ago
|
||
(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)
Comment 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
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]
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
(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)
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8604649 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jsnajdr)
Comment 14•9 years ago
|
||
Thanks Jarda! I'll have a look now, and give you some pointers for the test.
Comment 15•9 years ago
|
||
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+
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8605252 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8606258 -
Attachment is obsolete: true
Comment 21•9 years ago
|
||
Thanks Jarda! I'll have a look at your patch tomorrow.
Comment 22•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
Adding 'checkin-needed'. Linux debug tests (dt1,dt2) failed due to some timeout, not related to the patch.
Keywords: checkin-needed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9c0ebc0bdf00
Keywords: checkin-needed
Comment 25•9 years ago
|
||
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.
https://hg.mozilla.org/mozilla-central/rev/9c0ebc0bdf00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 28•9 years ago
|
||
(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 29•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 31•9 years ago
|
||
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+
Comment 32•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a06bc930e34c
Flags: in-testsuite+
Updated•9 years ago
|
Whiteboard: [devedition-40][good first bug][lang=javascript] → [polish-backlog][good first bug][lang=javascript]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•