1143004 - Performance timeline should label events from console.timeStamp

Status: VERIFIED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1143004-timestamp-ui.patch

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

Comment 2

[Victor Porof [:vporof][:vp]]

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.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

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?

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1143004-timestamp-ui.patch

updated colors

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

https://hg.mozilla.org/integration/fx-team/rev/1981440629bc

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1981440629bc

Comment 7

[Brian Grinstead [:bgrins]]

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

Comment 8

[Brian Grinstead [:bgrins]]

(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?

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Why is this being uplifted separately? Also, bgrins, wrt l10n check out [Bug 1163763] Fx41 localization revisit

Comment 10

[Brian Grinstead [:bgrins]]

(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

[Francesco Lodolo [:flod]]

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).

Comment 12

[Brian Grinstead [:bgrins]]

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.

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

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

[Brian Grinstead [:bgrins]]

(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.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: 1143004-timestamp-ui-aurora.patch

Patch for aurora uplift - uses a hardcoded string for "Timestamp" instead of the new string

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/02659d1bb649

Comment 18

[Brian Grinstead [:bgrins]]

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

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8611414 [details] [diff] [review]
1143004-timestamp-ui-aurora.patch

Change approved to skip one train as part of the spring campaign.

Comment 20

[Andrei Vaida [:avaida]]

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

[Will Bamberg [:wbamberg]]

I think the section in the Waterfall docs: https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall#Timestamp_markers ought to cover this.

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

LGTM!