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)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: vporof, Assigned: waffles, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(3 files, 1 obsolete file)
440.13 KB,
image/png
|
Details | |
3.69 KB,
patch
|
jsantell
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•10 years ago
|
Blocks: profiler-followups
Reporter | ||
Updated•10 years ago
|
Priority: -- → P3
Updated•9 years ago
|
Blocks: perf-polish
Updated•9 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Updated•9 years ago
|
Assignee: jsantell → nobody
Mentor: jsantell
Whiteboard: [good first bug][lang=js]
Assignee | ||
Comment 1•9 years ago
|
||
Hey Jordan, I'd like to work on this bug. What's the actual issue here? Can you get me started?
Flags: needinfo?(jsantell)
Comment 2•9 years ago
|
||
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!
Assignee | ||
Comment 3•9 years ago
|
||
Ah, okay. I'll definitely look out for your comment in my mail. Thanks! :)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Okay then, I'll wait for Victor's reply :)
Reporter | ||
Comment 11•9 years ago
|
||
(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)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
Yeah, that helps! I've modified those tests. Please try & review it :)
Attachment #8605222 -
Attachment is obsolete: true
Attachment #8609288 -
Flags: review?(jsantell)
Comment 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
Great! And, thanks for mentoring me Jordan :)
Flags: needinfo?(jsantell)
Comment 17•9 years ago
|
||
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)
Updated•9 years ago
|
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Updated•9 years ago
|
Blocks: perf-40-uplifts
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/11775de89565 https://hg.mozilla.org/integration/fx-team/rev/e9900b5c4da8
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/11775de89565 https://hg.mozilla.org/mozilla-central/rev/e9900b5c4da8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Flags: qe-verify+
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
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?
Comment 22•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/955dc6f63530 https://hg.mozilla.org/releases/mozilla-aurora/rev/c47ce06ace20
status-firefox40:
--- → fixed
Comment 23•9 years ago
|
||
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 24•9 years ago
|
||
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 25•9 years ago
|
||
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+
Comment 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
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)
Comment 28•9 years ago
|
||
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)
Comment 29•9 years ago
|
||
(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)
Comment 30•9 years ago
|
||
Good find, Andrei, and thanks!
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•