Closed
Bug 1167006
Opened 9 years ago
Closed 9 years ago
Stack Trace in markers should be displayed nicer.
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(2 files, 1 obsolete file)
16.79 KB,
patch
|
jsantell
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
134.20 KB,
image/png
|
Details |
Vague bug, maybe we should show different data, maybe the sidebar just needs to be wider by default, etc.
Assignee | ||
Updated•9 years ago
|
Blocks: perf-polish
Assignee | ||
Comment 1•9 years ago
|
||
Turned into more of a refactoring of actionable things in the marker details. Pulled out the stack trace logic into utils. Marker Details catches bubbled up click events so we can handle arbitrary actions without the element builders knowing about marker details. Pulled out logic for rendering source links, @see bug 1167075 for the follow ups Bigger default marker details view (300px, saving that size in bug 1167093) I would love to have the ellipses rendering that web console has for file names ("jquery.1.5.mi...:15") so we can take advantage of a resizable view, but I've ran out of animals to sacrifice to XUL. Should follow up in another bug for someone more CSS savvy
Comment 2•9 years ago
|
||
Comment on attachment 8608846 [details] [diff] [review] 1167006-markerdetails.patch Review of attachment 8608846 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/common.css @@ +252,5 @@ > +/* Links to source code, like displaying `myfile.js:45` */ > + > +/* > + * Unsure if :visited is necessary still > + * @see http://bugzil.la/575675 Don't think so ::: browser/themes/shared/devtools/dark-theme.css @@ +411,5 @@ > +} > + > +/* > + * FIXME: http://bugzil.la/575675 CSS links without :visited set cause assertion > + * failures in debug builds. wat
Attachment #8608846 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8608846 [details] [diff] [review] 1167006-markerdetails.patch Review of attachment 8608846 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/dark-theme.css @@ +403,5 @@ > background-color: #0f171f; > color: var(--theme-body-color); > } > > +.theme-link, Oh this all needs to go i'm not sure what this is. dupes.
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8608846 -
Attachment is obsolete: true
Attachment #8609550 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 6•9 years ago
|
||
This is causing the splitter to not show up properly for the toolbox because common.css is loaded in browser.xul where var(--theme-splitter-color) isn't defined. And now this rule for devtools splitters is being loaded in the browser window: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#1030. Any changes that are toolbox specific shouldn't go into common.css, and we should *not* be adding `%include toolbars.inc.css` into common.css since it means devtools theme changes could unintentionally interact with the browser window. I know the naming is confusing - as I mention in https://bugzilla.mozilla.org/show_bug.cgi?id=957663#c1 this file should be called something like "devtools-browser.css" instead. But I think this should be backed out while we figure out the best way to handle this case.
Flags: needinfo?(jsantell)
Comment 7•9 years ago
|
||
In this particular case, could you move the new rules into widgets.inc.css?
Comment 8•9 years ago
|
||
Screenshot of the missing separator
Assignee | ||
Comment 9•9 years ago
|
||
Toolbox should not be included here. That's a mistake. Would removing that fix this? This is for source link styling, that's it
Flags: needinfo?(jsantell)
Comment 10•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9) > Toolbox should not be included here. That's a mistake. Would removing that > fix this? This is for source link styling, that's it Yes, removing that would fix the problem. In general the source link stuff shouldn't be in common.css but it'd be OK to tackle that separately (in Bug 957663)
Assignee | ||
Comment 11•9 years ago
|
||
Reverting https://hg.mozilla.org/integration/fx-team/rev/780e1f999f54
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 12•9 years ago
|
||
Revert part 2 from bad merge. https://hg.mozilla.org/integration/fx-team/rev/ddcb19f7084a
Assignee | ||
Comment 13•9 years ago
|
||
I don't want to talk about how many times I've messed up this revert. https://hg.mozilla.org/integration/fx-team/rev/7ea688b10bcc
Comment 14•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13) > I don't want to talk about how many times I've messed up this revert. > https://hg.mozilla.org/integration/fx-team/rev/7ea688b10bcc Sorry - I guess it would have been easier after all to just push a change to remove the import.
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ddcb19f7084a https://hg.mozilla.org/mozilla-central/rev/7ea688b10bcc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 16•9 years ago
|
||
This was reverted; reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b15bc4bf63db
Blocks: perf-40-uplifts
Whiteboard: [fixed-in-fx-team]
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b15bc4bf63db
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment 20•9 years ago
|
||
Comment on attachment 8609550 [details] [diff] [review] 1167006-markerdetails.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 #8609550 -
Flags: approval-mozilla-aurora?
Comment 21•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9febab782065 https://hg.mozilla.org/releases/mozilla-aurora/rev/d6d39fe9ec03 https://hg.mozilla.org/releases/mozilla-aurora/rev/0cda21618fea
status-firefox40:
--- → fixed
Comment 22•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 23•9 years ago
|
||
Comment on attachment 8609550 [details] [diff] [review] 1167006-markerdetails.patch Change approved to skip one train as part of the spring campaign.
Attachment #8609550 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•9 years ago
|
||
Verified fixed on Aurora 40.0a2 (2015-06-08), using Windows 7 (x64), Ubuntu 14.04 (x46) and Mac OS X 10.9.5. The separator is now visible and displayed properly.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•