GCLI commands to start and stop our JavaScript Profiler

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anton, Assigned: anton)

Tracking

Trunk
Firefox 22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
profiler.start
profiler.stop

Anything else? I think profiler.start should always create a new profile and then start it.
"profiler load myprofile" would be useful too, after bug 828046.
Same goes for "profiler save" of course.
GCLI has its own component now.
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
OS: Mac OS X → All
Hardware: x86 → All
Can I reply to IRC here?

I think this belongs in the profiler directory. That's where we put the other commands (I mean along with the parent tool :), and it's because you're only likely to use a public API on the command line, but could well use a private API on the profiler (although perhaps not in this specific case)

Can you imagine what it would be like if new *nix installs all wrote their commands into the same directory?
Oh wait, you can.
That's what I'd like to avoid.
(Assignee)

Updated

5 years ago
Component: Developer Tools: Graphic Commandline and Toolbar → Developer Tools: Profiler
(Assignee)

Updated

5 years ago
Depends on: 853650
(Assignee)

Updated

5 years ago
Assignee: nobody → anton
(Assignee)

Comment 5

5 years ago
Created attachment 730356 [details] [diff] [review]
Add GCLI commands for the JavaScript Profiler

Refactored ProfilerPanel code to correctly display named profiles and added these GCLI commands:

 * profiler open
 * profiler close
 * profiler start [name]
 * profiler stop <name>
 * profiler list
 * profiler show <name>

I uncommented disabled profiler tests while working on this patch to make sure they still pass. Can't ask Joe to review since he's on PTO.

@dcamp, could you also check my English in the .properties file?
Attachment #730356 - Flags: review?(past)
Attachment #730356 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 730356 [details] [diff] [review]
Add GCLI commands for the JavaScript Profiler

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

Quick drive-by.

::: browser/devtools/profiler/cmd-profiler.jsm
@@ +30,5 @@
> + * 'profiler open' command
> + */
> +gcli.addCommand({
> +  name: "profiler open",
> +  description: gcli.lookup("profilerOpen"),

Please could you s/profilerXXX/profilerXXXDesc/g for all the command descriptions? i.e. the same as for the parent 'profiler' command? It's what we do elsewhere.

@@ +59,5 @@
> + */
> +gcli.addCommand({
> +  name: "profiler start",
> +  description: gcli.lookup("profilerStart"),
> +  returnType: "html",

I think you mean:

    returnType: "string",

?

@@ +65,5 @@
> +  params: [
> +    {
> +      name: "name",
> +      type: {
> +        name: "string"

This will work fine, but you can also do type:"string" if you'd like. (Same for 'profiler stop')

@@ +87,5 @@
> +      }
> +
> +      panel.switchToProfile(profile, function () profile.start());
> +      deferred.resolve(gcli.lookup("profilerStarting"));
> +      return deferred.promise;

Short for:

    let deferred = Promise.defer();
    deferred.resolve(gcli.lookup("profilerStarting"));
    return deferred.promise;

is:

    return Promise.resolve(gcli.lookup("profilerStarting"));

or in this case just:

    return gcli.lookup("profilerStarting");

(Same for 'profiler stop')

@@ +146,5 @@
> + */
> +gcli.addCommand({
> +  name: "profiler list",
> +  description: gcli.lookup("profilerList"),
> +  returnType: "html",

Surprised that this works (a recent bugfix made things pickier here). I think you mean "dom"?

@@ +180,5 @@
> + */
> +gcli.addCommand({
> +  name: "profiler show",
> +  description: gcli.lookup("profilerShow"),
> +  returnType: "html",

Probably should lose this line - returns undefined.

Comment 7

5 years ago
Comment on attachment 730356 [details] [diff] [review]
Add GCLI commands for the JavaScript Profiler

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

I have nothing to add to Joe's review.
Attachment #730356 - Flags: review?(dcamp)

Comment 8

5 years ago
Comment on attachment 730356 [details] [diff] [review]
Add GCLI commands for the JavaScript Profiler

You can treat that as an r+ with joe's comments addressed.
Attachment #730356 - Flags: review?(past) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 730910 [details] [diff] [review]
Add GCLI commands for the JavaScript Profiler

Addressed Joe's comments. Carrying over r+.
Attachment #730356 - Attachment is obsolete: true
Attachment #730910 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f942c99cfaec
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22

Updated

5 years ago
Depends on: 856797
Probably worth adding to bug 856797: you should not use "..." but the single unicode character "…" which is used consistently in Mozilla products. If you prefer a new bug I can open it.

And that's why is bad to land strings at the last moment on central ;-) (even if these are typos that you can fix without touching entities' names)
(Assignee)

Comment 13

5 years ago
This was not landed at the last moment.
(In reply to Anton Kovalyov (:anton) from comment #13)
> This was not landed at the last moment.

Sorry, you're absolutely right: "merge", not "land" (it landed 3 days ago, but for localizers it's like it landed 10 hours ago)
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130418 Firefox/22.0, build ID: 20130418004017
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130418 Firefox/22.0, build ID: 20130418004017
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20130418 Firefox/22.0, build ID: 20130418004017

Verified the functionality of GCLI profiler commands on the latest Firefox 22 (Aurora) build accross main platforms (Ubuntu 12.10, Mac 10.8, Win 8) and I found some issues for which I logged follow up bugs:
* Bug 863636 - Change error message when running "profiler close" (GCLI command) when profiler is not open
* Bug 863645 - "profiler list" shows error message 
* Bug 863653 - Set appropriate error message when using profiler GCLI commands with missing profile name paramete

Besides these, the commands seem to work fine.

Marking VERIFIED on Aurora.
status-firefox22: --- → verified
You need to log in before you can comment on or make changes to this bug.