Closed
Bug 1143004
Opened 9 years ago
Closed 9 years ago
Performance timeline should label events from console.timeStamp
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
Tracking
(firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
5.92 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
4.86 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
https://developer.chrome.com/devtools/docs/console#marking-the-timeline If we are recording a profile, the perf tool should add console events (console.timeStamp("foo")) to the overview markers and waterfall view. Perf tool should not do anything the tools are not open and not recording a profile.
Assignee | ||
Updated•9 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jsantell
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•9 years ago
|
Priority: -- → P2
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Not a huge fan of how these are rendered, and this, and console.time/timeEnd should be rendered differently, but can handle that later. Contains an l10n string. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9d44550d0fb
Attachment #8603872 -
Flags: review?(vporof)
Comment 2•9 years ago
|
||
Comment on attachment 8603872 [details] [diff] [review] 1143004-timestamp-ui.patch Review of attachment 8603872 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/timeline/global.js @@ +75,5 @@ > label: L10N.getStr("timeline.label.consoleTime") > }, > + "TimeStamp": { > + group: 2, > + colorName: "content-color3", content-color3? UUUGH T_T Let's use a more nicely named color, even though it matches the ones in our palette, it seems very out of place here.
Attachment #8603872 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Yeah, color names are bad :/ we'll need to update them all anyway I think because the light theme ones just don't look good (using the same highlight colors as text used elsewhere) -- OK to leave as now until we do new colors for 40.1? Ideally something that is similar, but still visible when inside of, as the console.time/time end markers?
Assignee | ||
Comment 4•9 years ago
|
||
updated colors
Attachment #8603872 -
Attachment is obsolete: true
Attachment #8604998 -
Flags: review+
Assignee | ||
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1981440629bc
Whiteboard: [devedition-40][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1981440629bc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Blocks: perf-40-uplifts
Comment 7•9 years ago
|
||
Comment on attachment 8604998 [details] [diff] [review] 1143004-timestamp-ui.patch Approval Request Comment [Feature/regressing bug #]: Performance Tool for Developer Edition 40.1 (1075567) [User impact if declined]: We won't ship the new performance tool [Describe test coverage new/current, TreeHerder]: There's a test for this new feature [Risks and why]: Risk is contained to the frontend of DevTools. Pretty minimal here, just marking when console.timeStamp() was called in the timeline frontend. [String/UUID change made/needed]: timeline.label.timestamp was added to browser/devtools/timeline.properties. We'd like to uplift even with the string change if possible. The alternative would be to ship a version of the patch with hardcoded English strings
Attachment #8604998 -
Flags: approval-mozilla-aurora?
Comment 8•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #7) > [String/UUID change made/needed]: timeline.label.timestamp was added to > browser/devtools/timeline.properties. We'd like to uplift even with the > string change if possible. The alternative would be to ship a version of > the patch with hardcoded English strings Hi Francesco, we have a number of uplifts for Aurora upcoming for the new performance tool in DevTools. We did our best to preland strings in 40, but a number of the patches still include string changes (either new strings or renaming parts of the tool to better match up with what people expect). Can you help us figure out the best course to take for these uplifts? Would we be able to uplift even with string changes, or would it be best to hardcode the strings in the code just for the 40 release?
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 9•9 years ago
|
||
Why is this being uplifted separately? Also, bgrins, wrt l10n check out [Bug 1163763] Fx41 localization revisit
Comment 10•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9) > Why is this being uplifted separately? Also, bgrins, wrt l10n check out [Bug > 1163763] Fx41 localization revisit We are going to have to do uplifts for everything blocking Bug 1163763 - this is just the first one. Just commented in Bug 1163763, but also going to leave the needinfo for :flod for this particular bug. It seems that there aren't many other string changes in the current queue of bug 1163763 so I'd like advice on how we should proceed in this particular case.
Comment 11•9 years ago
|
||
As Jordan pointed out, there was a similar question in bug 1163763, I think it makes more sense to approach this issue globally for devtools+fx40, not one string at a time (will add a comment there).
Flags: needinfo?(francesco.lodolo)
Comment 12•9 years ago
|
||
Comment on attachment 8604998 [details] [diff] [review] 1143004-timestamp-ui.patch Clearing uplift request - as discussed in Bug 1163763 we will need to uplift a version of this patch with a hardcoded string.
Attachment #8604998 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 13•9 years ago
|
||
If its easier, let's land a patch on top that hardcodes and revert later rather than making a special case for this
Comment 14•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13) > If its easier, let's land a patch on top that hardcodes and revert later > rather than making a special case for this Seems like it'd be easiest to uplift a version of the patch directly that has the hardcoded string instead of the localized one. Otherwise we are going to have to uplift two patches and have the extra push + backout on m-c.
Updated•9 years ago
|
Flags: qe-verify+
Comment 15•9 years ago
|
||
Patch for aurora uplift - uses a hardcoded string for "Timestamp" instead of the new string
Comment 16•9 years ago
|
||
Comment on attachment 8611414 [details] [diff] [review] 1143004-timestamp-ui-aurora.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 #8611414 -
Flags: approval-mozilla-aurora?
Comment 17•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/02659d1bb649
status-firefox40:
--- → fixed
Comment 18•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 19•9 years ago
|
||
Comment on attachment 8611414 [details] [diff] [review] 1143004-timestamp-ui-aurora.patch Change approved to skip one train as part of the spring campaign.
Attachment #8611414 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•9 years ago
|
||
Verified fixed on Aurora 40.0a2 (2015-06-04), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.8.5. Events from console.timeStamp() are showing up in the waterfall with a blue marker, e.g. http://i.imgur.com/BNcrkbO.png.
Comment 21•9 years ago
|
||
I think the section in the Waterfall docs: https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall#Timestamp_markers ought to cover this.
Flags: needinfo?(jsantell)
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Updated•9 years ago
|
Whiteboard: [polish-backlog]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•