Closed Bug 1167006 Opened 5 years ago Closed 5 years ago
Stack Trace in markers should be displayed nicer
Vague bug, maybe we should show different data, maybe the sidebar just needs to be wider by default, etc.
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
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8608846 - Flags: review?(vporof)
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+
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.
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.
In this particular case, could you move the new rules into widgets.inc.css?
Screenshot of the missing separator
Toolbox should not be included here. That's a mistake. Would removing that fix this? This is for source link styling, that's it
(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)
Revert part 2 from bad merge. https://hg.mozilla.org/integration/fx-team/rev/ddcb19f7084a
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
(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.
This was reverted; reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Should just verify that we aren't missing the separator.
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?
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 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+
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.
You need to log in before you can comment on or make changes to this bug.