Last Comment Bug 770145 - GCLI needs a command to log function calls
: GCLI needs a command to log function calls
Status: RESOLVED FIXED
[gclicommands]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: GCLICMD
  Show dependency treegraph
 
Reported: 2012-07-02 05:52 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-07-16 05:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (8.59 KB, patch)
2012-07-13 04:01 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
v2 (8.46 KB, patch)
2012-07-13 04:57 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
past: review+
mihai.sucan: feedback+
Details | Diff | Splinter Review
v3 (8.61 KB, patch)
2012-07-13 08:19 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-02 05:52:50 PDT
Blair has created the command at:
https://github.com/Unfocused/gcli-commands/blob/master/calllog.mozcmd
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-13 04:01:29 PDT
Created attachment 641807 [details] [diff] [review]
v1
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-13 04:57:33 PDT
Created attachment 641822 [details] [diff] [review]
v2

Mihai - I'd like to change the destination of the logs from the error console to the web console. Also I'm being slightly lazy. ;-)
What's the best way to replace onEnterFrame to do that?

Also I'm thinking that I could add:
    let tab = context.environment.chromeDocument.defaultView.gBrowser.selectedTab
    HUDService.activateHUDForContext(tab);

To the end of the 'calllog start'.exec() to open the web console, so people can see where the output is going.
Comment 3 Panos Astithas [:past] 2012-07-13 06:09:08 PDT
Comment on attachment 641822 [details] [diff] [review]
v2

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

Looks good to me. Just a few nits that you may or may not want to consider.

::: browser/devtools/commandline/GcliCommands.jsm
@@ +331,5 @@
> +               "component javascript");
> +      Services.console.logMessage(msg);
> +    }
> +    catch(ex) {
> +      // Ignore

Are you sure this is what we want here? Wouldn't reporting errors help us make this more robust?

@@ +342,5 @@
> +    }
> +    return "[object " + value.class + "]";
> +  },
> +
> +  framePosition: function(frame) {

This method is unused.

@@ +353,5 @@
> +    return source + ":" + line;
> +  },
> +
> +  callDescription: function(frame) {
> +    let name = frame.callee.name || "<anonymous>";

In dbg-script-actors.js we also support the fn.displayName convention for naming functions. Not terribly important, but some frameworks use it AFAIK.

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-script-actors.js#1647

@@ +367,5 @@
> +  name: "calllog stop",
> +  description: gcli.lookup("calllogStopDesc"),
> +
> +  exec: function(args, context) {
> +    let global = Components.utils.getGlobalForObject(this);

This is redundant and can be removed.
Comment 4 Panos Astithas [:past] 2012-07-13 06:23:16 PDT
(In reply to Joe Walker from comment #2)
> Created attachment 641822 [details] [diff] [review]
> v2
> 
> Mihai - I'd like to change the destination of the logs from the error
> console to the web console. Also I'm being slightly lazy. ;-)
> What's the best way to replace onEnterFrame to do that?

I assume you mean replace Services.console.logMessage (onEnterFrame is the necessary debugger hook). Wouldn't contentWindow.console.log work?

> Also I'm thinking that I could add:
>     let tab =
> context.environment.chromeDocument.defaultView.gBrowser.selectedTab
>     HUDService.activateHUDForContext(tab);
> 
> To the end of the 'calllog start'.exec() to open the web console, so people
> can see where the output is going.

Good idea.
Comment 5 Mihai Sucan [:msucan] 2012-07-13 07:13:05 PDT
Comment on attachment 641822 [details] [diff] [review]
v2

This looks good to me. I suggest you use contentWindow.console.log() instead of the nsIConsoleService.

Otherwise you can do createElement() for the richlistitem and all of the things you want inside, then call outputMessage(), but things will get complicated and it's really prone to errors. If you want to land this patch in time for Firefox 16, I believe console.log() is the safest solution.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-13 08:19:31 PDT
Created attachment 641908 [details] [diff] [review]
v3
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-13 10:27:33 PDT
https://tbpl.mozilla.org/?tree=Fx-Team&rev=e7d2e3c6d861
Comment 8 Ed Morley [:emorley] 2012-07-16 05:27:12 PDT
https://hg.mozilla.org/mozilla-central/rev/e7d2e3c6d861

Note You need to log in before you can comment on or make changes to this bug.