Timeline Markers reason/cause fields should be symbols, not sentences

VERIFIED FIXED in Firefox 40

Status

defect
VERIFIED FIXED
4 years ago
Last year

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

(Blocks 1 bug)

37 Branch
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox40 verified, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

For example, Function Calls that have "setTimeout handle" for a reason -- this should just be "setTimeout", and the front end can display just that, or add a proper localization for "handle" in this case if necessary, or map the cause to something else that's localized.

Many of our values should not be localized (setTimeout, all the JIT opt info, etc.) but we should ensure that these stay symbols, rather than sentences.
We should translate these on the client (may or may not need l10n, depending on if its just "setTimeout", or otherwise)

And there are also some markers that aren't related to content. These should be simply labeled as (Gecko) when not showing platform data.

Content Markers:

* "<script> element"
* “EventListener.handleEvent"
* “setInterval handler"
* “setTimeout handler"
* “FrameRequestCallback"
* “EventHandlerNonNull"
* "promise callback"
* “promise initializer"
* "Worker runnable”

Platform Markers: (some of these probably do not get ever emitted, as they're intermediate steps, but I can't tell from looking at the commit -- these should probably be printed untranslated)

* “promise thenable"
* “worker runnable"
* "nsHTTPIndex set HTTPIndex property"
* "XPCWrappedJS method call"
* "nsHTTPIndex OnFTPControlLog"
* "XPCWrappedJS QueryInterface"
* "xpcshell argument processing”
* "XPConnect sandbox evaluation "
* "component loader report global"
* "component loader load module"
* "Cross-Process Object Wrapper call/construct"
* "Cross-Process Object Wrapper ’set'"
* "Cross-Process Object Wrapper 'get'"
* "nsXULTemplateBuilder creation"
* “TestShellCommand"
* “precompiled XUL <script> element"
* "XBL <field> initialization "
* "NPAPI NPN_evaluate"
* "NPAPI get"
* "NPAPI set"
* "NPAPI doInvoke"
* "javascript: URI"
* “geolocation.always_precise indexing"
* "geolocation.app_settings enumeration"
* "WebIDL dictionary creation"
* “XBL <constructor>/<destructor> invocation"
* “message manager script load"
* “message handler script load"
* "nsGlobalWindow report new global"
  it looks like when a new window is created, there's a call into the debugger API to tell the debugger about it; and this call requires settng up for a JS API call
"javascript: URI" should be content as well.
Depends on: 1165504
No longer depends on: 1165504
So this bug will map some keys "EventListener.handleEvent" to a more human-readable tag.
Depends on: 1165504
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Posted patch 1162662-js-markers.patch (obsolete) — Splinter Review
I'm digging this, check it out, see what you think.
Attachment #8607947 - Flags: review?(vporof)
Duplicate of this bug: 1166005
Comment on attachment 8607947 [details] [diff] [review]
1162662-js-markers.patch

Review of attachment 8607947 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/timeline/global.js
@@ +159,5 @@
> +  "promise initializer":       "Promise Init",
> +  "Worker runnable":           "Worker",
> +  "javascript: URI":           "JavaScript URI",
> +  // As far as I know, the difference between these two
> +  // event handler markers are differences in their webidl implementation.

Remove this comment lol

::: browser/devtools/shared/timeline/marker-utils.js
@@ +70,5 @@
> +  // If blueprint.fields is a function, use that
> +  if (typeof blueprint.fields === "function") {
> +    let fields = blueprint.fields(marker);
> +    // Add a ":" to the label since the localization files contain the ":"
> +    // if not present. This should be changed, ugh.

Is there a bug for our l10n story? Add the bug no here or something.

I'm starting to dislike this file.
Attachment #8607947 - Flags: review?(vporof) → review+
Comment on attachment 8607947 [details] [diff] [review]
1162662-js-markers.patch

Review of attachment 8607947 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/timeline/global.js
@@ +159,5 @@
> +  "promise initializer":       "Promise Init",
> +  "Worker runnable":           "Worker",
> +  "javascript: URI":           "JavaScript URI",
> +  // As far as I know, the difference between these two
> +  // event handler markers are differences in their webidl implementation.

This is useful information, as they are displayed the same (and may be proven differently in the future)

::: browser/devtools/shared/timeline/marker-utils.js
@@ +70,5 @@
> +  // If blueprint.fields is a function, use that
> +  if (typeof blueprint.fields === "function") {
> +    let fields = blueprint.fields(marker);
> +    // Add a ":" to the label since the localization files contain the ":"
> +    // if not present. This should be changed, ugh.

Bug number added -- I mostly hate constructing elements in general
Attachment #8607947 - Attachment is obsolete: true
Attachment #8609543 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2c1741464d75
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Flags: qe-verify+
Comment on attachment 8609543 [details] [diff] [review]
1162662-js-markers.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 #8609543 - 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 8609543 [details] [diff] [review]
1162662-js-markers.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8609543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm not quite sure what needs to be verified in this patch. Jordan, could you please elaborate a bit on what Manual QA can do here to help?
Flags: needinfo?(jsantell)
Mostly that the mappings for JS markers are mapped from the left hand side to the right hand side[0], changing the name of the "reason" to something more consistent and understandable (like mapping EventHandlerNonNull to "Event Handler")

[0] https://hg.mozilla.org/releases/mozilla-aurora/rev/efff9db98e86#l2.136
Flags: needinfo?(jsantell)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)
> Mostly that the mappings for JS markers are mapped from the left hand side
> to the right hand side[0], changing the name of the "reason" to something
> more consistent and understandable (like mapping EventHandlerNonNull to
> "Event Handler")
> 
> [0] https://hg.mozilla.org/releases/mozilla-aurora/rev/efff9db98e86#l2.136

Thanks!

This is verified fixed on Aurora 40.0a2 (2015-06-15), using Mac OS X 10.9.5, Ubuntu 14.04 (x64) and Windows 8.1 (x64).
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.