Closed Bug 1049865 Opened 10 years ago Closed 9 years ago

Remove url query from items in the call tree

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

defect

Tracking

(firefox40 verified, firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- fixed

People

(Reporter: vporof, Assigned: waffles, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(3 files, 1 obsolete file)

Attached image ugly
      No description provided.
Priority: -- → P3
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee: jsantell → nobody
Mentor: jsantell
Whiteboard: [good first bug][lang=js]
Hey Jordan, I'd like to work on this bug. What's the actual issue here? Can you get me started?
Flags: needinfo?(jsantell)
Hey Ravi! Thanks for your interest. The call tree logic that this bug touches is being reworked to handle a faster format in bug 1154115, and a follow up in bug 1160691 to change the location parsing logic -- these /should/ be completed sometime early this week, I hope; at that point I'll be able to give you better help on where to look for that :) I'll update in the bug when those are sorted out. Thanks!
Ah, okay. I'll definitely look out for your comment in my mail. Thanks! :)
Ok looks like the other patches just landed yesterday, so this is ready to go!

I gave this a once over to make sure I understand it all myself :)

Ok so in attachment 8468738 [details] we display the filename with all of the extra query params there #moatClientLevel=1&moatClientClient2= etc.. This doesn't look very good! While we have the URL for each frame, we also have the filename. The full URL is displayed when hovering over the link there, which is accurate and links to the debugger, but the file name displayed (in blue, the moatad.js#moatClientLevel....) should not contain query params.

In browser/devtools/shared/profiler/frame-utils.js, there's a utility function called parseLocation -- in line 132, we take an nsIURI object and create the `fileName` here -- and attach the `uri.ref` if it exists, but I'm not sure if that's correct because it looks like it's query params in the image -- but I think that's a good place to start. Let me know if you have any other questions or a step in the right direction!
Assignee: nobody → wafflespeanut
Flags: needinfo?(jsantell)
Attached patch urlquery (obsolete) — Splinter Review
I guess appending that `uri.ref` to the filename is arousing the problem here. So, I've removed it. I think this might get better if there's a testcase against which this patch can be tested. What do you think?
Attachment #8605222 - Flags: review?(jsantell)
There are a few tests in browser/devtools/performance/test/browser_profiler_tree_[model|view]-*.js -- can run everything via `./mach mochitest-devtools browser/devtools/performance` -- and I'm not sure the original reasoning here. Do we want to display the `ref` field, and not query params? I think `fileName` should just be the last part of the path, and not have the ref or query fields, like you have here (since we can get the full URL elsewhere).

Victor I think wrote this initial patch, so pinging him on the reasoning here, but I think it should be good -- if we can get a test in one of those files ( probably browser_profiler-frame-utils-01.js in that test directory), and update any necessary tests, I think that sounds good!
Flags: needinfo?(vporof)
Comment on attachment 8605222 [details] [diff] [review]
urlquery

Looks good, just need a test case for this, and possibly update other test cases that test # in file name
Attachment #8605222 - Flags: review?(jsantell) → feedback+
Hey Jordan, let me clarify a few things. Please tell me if I'm wrong with my reasoning. We're supposed to hide the query params (the stuff following the `#`), right? It appeared as if `uri.ref` represents those things (because it's the one attached with `#` in the fileName). So, I replaced that line to just carry the file name. And, I tested the latest build with my line change, which returned a lot of errors because almost all of them were looking for the `#<something>` in the tests.
Flags: needinfo?(jsantell)
I think it's fine to remove it, but Victor had those assertions in place in the first place, so if he says that's good, then we can probably just change the tests to not hold on to any query params
Flags: needinfo?(jsantell)
Okay then, I'll wait for Victor's reply :)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)

Hiding both the query and the id ref is fine. My initial implementation was based on 20 seconds of experimentation. It now seems that it doesn't give us the pretties results.

In any case, it doesn't matter because the user can still jump to the correct file in the debugger, since we store the entire url.
Flags: needinfo?(vporof)
Thanks Victor :)

Hey Jordan, regarding modifying those tests in `browser_profiler-frame-utils-01.js`, I guess I gotta remove those test cases which have `#` in them. So, I have to delete those element from the respective arrays, right?
Flags: needinfo?(jsantell)
Well, we shouldn't remove them -- we should ensure that a url's with # and query params still have them in the url, but stripped from the filename, so to rewrite the 4th element in that test:

"hello/<.world (http://foo/bar.js#baz:123:987)" -> 

Previously:
["hello/<.world", "bar.js#baz", "foo", "http://foo/bar.js#baz", 123, 987, "foo", null]

Now should be:
["hello/<.world", "bar.js", "foo", "http://foo/bar.js#baz", 123, 987, "foo", null]

So it looks like just the 4th and 5th assertions should change (since they have #'s), and we should add a new one with a query string (bonus points for ending in a number to try and confuse the line parser), like:
"hello/<.world (http://foo/bar.js?myquery=params&search=1:123:987)"
and ensure we just get the file name "bar.js", and the line/columns are still correct. Does that help?
Flags: needinfo?(jsantell)
Attached patch urlquerySplinter Review
Yeah, that helps! I've modified those tests. Please try & review it :)
Attachment #8605222 - Attachment is obsolete: true
Attachment #8609288 - Flags: review?(jsantell)
Comment on attachment 8609288 [details] [diff] [review]
urlquery

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

Looks great, Ravi! Will run some tests with this and get it landed hopefully soon if everything looks good. Thanks!
Attachment #8609288 - Flags: review?(jsantell) → review+
Great! And, thanks for mentoring me Jordan :)
Flags: needinfo?(jsantell)
No problem at all :) looks like another test failed so just made a small fix to that, and we're good to land!
Flags: needinfo?(jsantell)
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/11775de89565
https://hg.mozilla.org/mozilla-central/rev/e9900b5c4da8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 41
Flags: qe-verify+
Comment on attachment 8609288 [details] [diff] [review]
urlquery


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8609288 - Flags: approval-mozilla-aurora?
Comment on attachment 8609565 [details] [diff] [review]
1049865-test.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8609565 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8609565 [details] [diff] [review]
1049865-test.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8609565 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8609288 [details] [diff] [review]
urlquery

Change approved to skip one train as part of the spring campaign.
Attachment #8609288 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 40.0a2 (2015-06-08), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Query string parameters are no longer displayed for any URL available in the "Call Tree", with the exception of a few URLs referring to google ads - see http://i.imgur.com/S4P03e3.png. I'm not sure if this is expected or not, as I haven't noticed any other ones showing parameters. Ravi, Jordan - any thoughts on this?
Flags: needinfo?(wafflespeanut)
You're right that the goal of this bug is to get rid of the ugly query params while displaying the call tree. And, I checked with Aurora 40.0a2 for a sample webpage (containing google-ads). I don't seem to get the query params. It might be helpful if you can throw me a link.

(Pinging Jordan on this)
Flags: needinfo?(wafflespeanut)
Flags: needinfo?(jsantell)
Flags: needinfo?(andrei.vaida)
Looks like this is different because those frames do not have a function name, so it's a different code path and kind of a different problem -- opened up bug 1172974 for this. Thanks all!
Flags: needinfo?(jsantell)
(In reply to Ravi Shankar [:waffles] from comment #27)
> You're right that the goal of this bug is to get rid of the ugly query
> params while displaying the call tree. And, I checked with Aurora 40.0a2 for
> a sample webpage (containing google-ads). I don't seem to get the query
> params. It might be helpful if you can throw me a link.
> 
> (Pinging Jordan on this)

It seems to be happening with specific Wikipedia.org links as well, here's a screenshot: http://i.imgur.com/HnEPPpP.png.

Since this issue will be treated in Bug 1172974, I'm gonna go ahead and close this one.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Good find, Andrei, and thanks!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.