Closed
Bug 1162662
Opened 10 years ago
Closed 9 years ago
Timeline Markers reason/cause fields should be symbols, not sentences
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox40 verified, firefox41 fixed)
VERIFIED
FIXED
Firefox 41
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jsantell
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
"javascript: URI" should be content as well.
Assignee | ||
Comment 3•10 years ago
|
||
So this bug will map some keys "EventListener.handleEvent" to a more human-readable tag.
Blocks: operation-instrument
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
I'm digging this, check it out, see what you think.
Attachment #8607947 -
Flags: review?(vporof)
Assignee | ||
Updated•9 years ago
|
Blocks: perf-40-uplifts
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8607947 -
Attachment is obsolete: true
Attachment #8609543 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 10•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Flags: qe-verify+
Comment 11•9 years ago
|
||
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?
Comment 12•9 years ago
|
||
status-firefox40:
--- → fixed
Comment 13•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 14•9 years ago
|
||
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+
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
(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).
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•