Closed
Bug 1242601
Opened 8 years ago
Closed 7 years ago
"Show allocation triggers" button doesn't look very button-y or link-y
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: fitzgen, Assigned: keremkat, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [good first bug][mentor=vp][lang=js])
Attachments
(4 files, 2 obsolete files)
And so a lot of people probably don't understand how to use it well. It is in the sidebar in this screenshot: https://bug1233551.bmoattachments.org/attachment.cgi?id=8711077 (If you can't find it, that is more evidence in support of styling this thing better!)
Comment 1•8 years ago
|
||
Triaging. Filter on LULUGUBRIOUSUS.
Priority: -- → P2
Whiteboard: [good first bug][mentor=vp][lang=js]
Comment 2•8 years ago
|
||
Nick, I'd like to work on this bug. Could you please assign it to me? Btw, is there are sequence of actions (e.g., a webpage) that'll help me reproduce a similar performance recording. I tried a couple and wasn't able to get the "Show allocation triggers" button in the sidebar for a non-incremental GC row.
Flags: needinfo?(nfitzgerald)
Reporter | ||
Comment 3•8 years ago
|
||
Octane should trigger some: https://octane-benchmark.googlecode.com/svn/latest/index.html You can also try something like this on a local html page: window.allocations = []; function loop() { for (let i = 0; i < 10000; i++) window.allocations.push({}); setTimeout(loop, 1); } setTimeout(loop, 1);
Assignee: nobody → maurya1985
Flags: needinfo?(nfitzgerald)
Updated•8 years ago
|
Version: unspecified → 46 Branch
Comment 4•8 years ago
|
||
Nick, currently the element is a label. What do you think about replacing the label with a button (rather than a link)?
Comment 5•8 years ago
|
||
something like this ...
Reporter | ||
Comment 6•8 years ago
|
||
Maurya, that looks pretty awesome to me! Jordan knows this code much better than I do, so redirecting mentorship his way.
Mentor: jsantell
Comment 7•8 years ago
|
||
Jordan, I'm not so experienced in JS. Can you help me with this? I replaced a label with a button. But, the flow of execution no longer hits the click event handler. Here's the old code commented out and the new code right below it: https://pastebin.mozilla.org/8861715
Flags: needinfo?(jsantell)
Comment 8•8 years ago
|
||
Jordan, someone on chat suggested styling the label as a button. So, I'm considering trying this out.
Comment 9•8 years ago
|
||
Hm not sure why the click event on the button wouldn't be happening anymore (handled in devtools/client/performance/modules/widgets/marker-details.js _onClick), unless the button element swallows the event.. Trying out your code, it does work for me; clicking on the button selects a new range of the timeline and switches to the allocations view, and looks good! The active state when pressing down on the button, though, looks like it's native OSX button styling (bright blue background, white text), but could probably use some of the styles from other buttons in this tool. Does that help? Are you not seeing the button, and it's not switching views when clicking, in a profile from octane?
Flags: needinfo?(jsantell)
Comment 10•8 years ago
|
||
I tried it out again and without any luck. When I click on the "Show allocation triggers" button, the marker details sidebar becomes blank white entirely. It doesn't switch to the allocation view. I checked the js code using the Browser Toolbox just to be sure that the correct and latest js code is being loaded by the browser. And, there's no problem here. So, I'm puzzled how it's working for you, but not for me!
Flags: needinfo?(jsantell)
Comment 11•8 years ago
|
||
I clobbered and rebuilt just to be sure, but that doesn't change the behavior.
Comment 12•8 years ago
|
||
I'm able to make a label appear like a button using the -moz-appearance CSS property. This might be a better approach since the button is swallowing the event. However, not sure why the results would vary from that of yours.
Flags: needinfo?(jsantell)
Comment 13•8 years ago
|
||
Attachment #8727159 -
Flags: review?(jsantell)
Comment 14•8 years ago
|
||
Sorry it's taken so long to respond, Maurya! I tried out your patch, and gets us 90% there -- attach is a picture of the text color looking a bit too light when in the dark theme, and the picture on the right is when I click the button on the light theme -- so it seems like it just needs a few more tweaks to fix these issues. Thank you! For more CSS changes, Victor Porof (vporof@mozilla.com) is probably a better bet to look over the code.
Updated•8 years ago
|
Attachment #8727159 -
Flags: review?(jsantell)
Updated•8 years ago
|
Flags: needinfo?(vporof)
Comment 15•8 years ago
|
||
Jordan, I'll fix the issue in the dark theme. But, is the display on the light theme also a problem? Or, is that only for comparison that you captured the screenshot?
Flags: needinfo?(jsantell)
Comment 16•8 years ago
|
||
Victor, do you have any suggestions on which existing styles I might be able to use for fixing the display on the dark theme in Mac? devtools-toolbarbutton seems quite similar to devtools-button. I'm a little new to CSS, so your suggestions will help.
Comment 17•8 years ago
|
||
The image of the light theme is when I click the button -- seems to adopt the OS's button select theme which is bright blue on OSX -- it should ideally look more similar to the other buttons in the tool
Flags: needinfo?(jsantell)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 18•8 years ago
|
||
needinfo ping?
Comment 19•8 years ago
|
||
Make sure the element is a <xul:button> with class .devtools-toolbarbutton, and use a "command" event listener instead of a "click" handler.
Flags: needinfo?(vporof)
Comment 20•8 years ago
|
||
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Version: 46 Branch → unspecified
Comment 21•8 years ago
|
||
Victor, I tried your suggestion of using a button with a command event handler. However, the handler is not being called when the event is fired though I have an event listener registered for the command event. Too keep things simple, I tried adding a dummy event listener directly to the button element. It didn't work for me. The surprising thing is that it worked for 2 other people I was working with on the #devtools IRC. Here's the code for that: https://pastebin.mozilla.org/8865079 While trying to debug if my event listeners are in the first place even getting registered, I noticed and filed this bug: Bug 1259070 I tried various approaches and am stuck here. Let me know if you have any suggestions. Also, should we add Bug 1259070 as "Depends on" to this bug?
Comment 23•8 years ago
|
||
needinfo ping
Comment 24•8 years ago
|
||
Jordan, do you have any suggestions for the above problem that I'm running into?
Flags: needinfo?(jsantell)
Comment 26•8 years ago
|
||
Sorry Maurya, its hard for me to get to this right now, but pinging Greg for ya
Flags: needinfo?(jsantell) → needinfo?(gtatum)
Comment 27•8 years ago
|
||
Hello Maurya, are you still interested in working on this? I kind of lost track of this in the shuffle of some of the team over to Project Tofino, and I only joined the team in January so I've still been figuring things out. I was looking through the comment thread again, but unfortunately all of your pastebin examples were deleted as they are probably too old. Could you provide the code samples so I can examine the problem?
Flags: needinfo?(gtatum)
Comment 28•8 years ago
|
||
Hi Greg, thanks for reaching out! I'm currently busy with a few other bugs on the iOS side. So, if anyone's interested meanwhile, I wouldn't mind giving up this bug. The problem I had with the command event handler not being called when an event is fired seems to be isolated to my setup on a Windows 8 machine. Others on the chat channel who were helping me out with this problem were not able to reproduce it on their side. So, I'd say that Victor Porof's suggestions in comment19 should be tried out on the patch that I had previously submitted.
Comment 29•8 years ago
|
||
Ok thanks! My thought on this is that it would probably be a better to go ahead and remove the XUL from this component anyway. I'll take you off of the bug, but leave it open for now.
Updated•8 years ago
|
Assignee: maurya1985 → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Mentor: jsantell
Comment 30•8 years ago
|
||
Is anyone actively working on this ? I could work on this if there is no one else already. Hello nick, a mentor would be great too because it looks like there will be discussion needed on some steps.
Comment 31•8 years ago
|
||
Nope, no one is working on it, feel free to take it. I can mentor on this one. Does the existing discussion give you enough to get started?e
Mentor: gtatum
Comment 32•8 years ago
|
||
Thanks Greg, first I will need a link to see that (link/button) "Show allocation triggers" because Tim's link on comment 3 looks expired. I will inform if I need anything else further.
Comment 33•8 years ago
|
||
Here you go: http://chromium.github.io/octane/ Also CNN.com might generate enough garbage to cause the non-incremental GC to show up, without the crazy overhead of the octane tests. Oh, and if you can make sure to "need info" when you need something from me it'll help make sure that I get back to you in a timely manner. It's that little checkbox and dropdown next to the Save Changes button.
Comment 34•8 years ago
|
||
Hi Greg, I tried your octane link and CNN too, but hardly found a Non-Incremental GC. But whatever I found, there was no button/label like show allocation triggers see this http://prntscr.com/bv3s0e please find a way so that I can see this label, then I will be able to modify it Thanks
Flags: needinfo?(gtatum)
Comment 35•8 years ago
|
||
Unfortunately for the case with this bug, non-Incremental GCs are pretty rare so they are hard to find one. CNN does seem like a good place to find them, I've had trouble with octane freezing the browser because the load is too much. I just did some testing on my end and it looks like the button doesn't show up if there aren't any recorded triggers. So only SOME of the Non-Incremental GCs have the button :-/ I'll double check with someone that that's really the case. It might be worth manually adding that panel for the purpose of working on that button so there is a more consistent way to see what you are working on. I wonder if there is a mochitest that builds up that panel for you. You can run `./mach mochitest path/to/test.js --jsdebugger` and throw in debugger breakpoints to have a place a nice little repeatable workspace. Let me know if you want me to look in the tests and see if I can find a good test that might let you do that.
Flags: needinfo?(gtatum)
Comment 37•7 years ago
|
||
Hi, I'd like to work on this bug,would u assign this to me?
Assignee | ||
Comment 38•7 years ago
|
||
I have added a new CSS style and the button looks like a link with the provided patch. Please review the changes, also, if you have trouble generating the "Non-incremental GC" event with the "Show allocation changes" button, try running half of the the JetStream tests at http://browserbench.org/JetStream/
Attachment #8822915 -
Flags: review?(gtatum)
Assignee | ||
Comment 39•7 years ago
|
||
Added screenshots taken after 0001-devtools-marker-details-link-style.patch is applied.
Comment 40•7 years ago
|
||
Oh thanks! Would you mind using the custom-button class instead of adding a new one, and then placing it in devtools/client/themes/performance.css custom-button doesn't appear to be used anywhere outside of the performance tools, and this way it will only affect the performance tools. https://dxr.mozilla.org/mozilla-central/search?q=custom-button&redirect=false
Comment 41•7 years ago
|
||
As Kerem has submitted a patch and seems to be on the right track, [Once becomes able to set the correct button class name on the target button, the patch will be good to go] Assigning on Kerem for now. [We should not keep Unassigned bugs with patches attached]
Assignee: nobody → keremkat
Status: NEW → ASSIGNED
Assignee | ||
Comment 42•7 years ago
|
||
Thank you for the quick response. As to the unassigned issue with a patch, that is my fault. I haven't asked to be assigned this issue before submitting the first patch.
Attachment #8822915 -
Attachment is obsolete: true
Attachment #8822915 -
Flags: review?(gtatum)
Attachment #8823697 -
Flags: review?(gtatum)
Assignee | ||
Comment 43•7 years ago
|
||
I have made the requested changes, could you take another look? Thanks.
Flags: needinfo?(gtatum)
Updated•7 years ago
|
Attachment #8727159 -
Attachment is obsolete: true
Flags: needinfo?(gtatum)
Comment 44•7 years ago
|
||
Comment on attachment 8823697 [details] [diff] [review] 0002-devtools-marker-details-link-style.patch Review of attachment 8823697 [details] [diff] [review]: ----------------------------------------------------------------- That works for me.
Attachment #8823697 -
Flags: review?(gtatum) → review+
Assignee | ||
Comment 45•7 years ago
|
||
Oh great! Would you mind pushing the patch to the try server for testing?
Flags: needinfo?(gtatum)
Comment 46•7 years ago
|
||
Wow, my tooling is doing weird stuff. Thought I pushed it to try yesterday when I reviewed, now I've pushed it twice. Anyway here it is (twice): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c43fc67df226f4363f3a8020ea882b743764210d https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff32efbd3f079691c5d4dbd8991d3a372cca9ff8
Flags: needinfo?(gtatum)
Keywords: checkin-needed
Comment 47•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/104807b2a841 Change devtools "Show allocation triggers" button style to resemble a link. r=gtatum
Keywords: checkin-needed
Comment 48•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/104807b2a841
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 49•7 years ago
|
||
I have reproduced this bug in nightly 46.0a1 (2016-01-25) on Deepin Linux, 64bit This bug is now verified as fixed in Latest Beta 54.0b12 (64-bit) Build ID 20170529095024 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 [bugday-20170531]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•