Closed Bug 1167006 Opened 5 years ago Closed 5 years ago

Stack Trace in markers should be displayed nicer.

Categories

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

41 Branch
defect
Not set

Tracking

(firefox40 verified, firefox41 fixed)

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

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(2 files, 1 obsolete file)

Vague bug, maybe we should show different data, maybe the sidebar just needs to be wider by default, etc.
Attached patch 1167006-markerdetails.patch (obsolete) — Splinter Review
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.
Attachment #8608846 - Attachment is obsolete: true
Attachment #8609550 - Flags: review+
Whiteboard: [fixed-in-fx-team]
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)
In this particular case, could you move the new rules into widgets.inc.css?
Attached image missing-separator.png
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
Flags: needinfo?(jsantell)
(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)
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.
https://hg.mozilla.org/mozilla-central/rev/ddcb19f7084a
https://hg.mozilla.org/mozilla-central/rev/7ea688b10bcc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
This was reverted; reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/b15bc4bf63db
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Should just verify that we aren't missing the separator.
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.