Last Comment Bug 771526 - GCLI needs a command to log function calls in chrome content
: GCLI needs a command to log function calls in chrome content
Status: RESOLVED FIXED
[gclicommands][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
: developer.tools
Mentors:
Depends on: 776875
Blocks: GCLICMD 781856 786193
  Show dependency treegraph
 
Reported: 2012-07-06 08:25 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-08-28 02:56 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 WIP (14.67 KB, patch)
2012-07-24 04:58 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch (14.21 KB, patch)
2012-07-25 06:54 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Properly handle sandbox destruction in the event of an exception (14.39 KB, patch)
2012-07-26 10:32 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Rebased (15.18 KB, patch)
2012-08-06 06:40 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Rebased (14.66 KB, patch)
2012-08-06 06:50 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Addressed reviewers comments (16.29 KB, patch)
2012-08-10 10:07 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Review
Throw error on var not found (16.30 KB, patch)
2012-08-14 08:15 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-06 08:25:49 PDT
Blair has created the command at:
https://github.com/Unfocused/gcli-commands/blob/master/chrome-calllog.mozcmd
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-24 04:58:58 PDT
Created attachment 645260 [details] [diff] [review]
Patch 1 WIP

Need to find out where the leaks are coming from ... I suspect a debugger bug
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-24 16:01:26 PDT
dcamp says to try dbg.enabled = false;
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-25 06:54:38 PDT
Created attachment 645751 [details] [diff] [review]
Patch

No leaks, green on try
Comment 4 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-25 17:52:57 PDT
Comment on attachment 645751 [details] [diff] [review]
Patch

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

::: browser/devtools/commandline/commands/GcliCalllogChromeCommands.jsm
@@ +86,5 @@
> +      } catch(e) {
> +        // We need to save the message before cleaning up else e contains a dead
> +        // object.
> +        let msg = gcli.lookup("callLogChromeEvalException") + ": " + e;
> +        cleanUp();

If we get an exception adding a second or third call-logger, this will kill all other existing call-loggers - which seems wrong/unexpected.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-26 10:32:22 PDT
Created attachment 646195 [details] [diff] [review]
Properly handle sandbox destruction in the event of an exception

(In reply to Blair McBride (:Unfocused) from comment #4)
> Comment on attachment 645751 [details] [diff] [review]
> Patch
> 
> Review of attachment 645751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/commands/GcliCalllogChromeCommands.jsm
> @@ +86,5 @@
> > +      } catch(e) {
> > +        // We need to save the message before cleaning up else e contains a dead
> > +        // object.
> > +        let msg = gcli.lookup("callLogChromeEvalException") + ": " + e;
> > +        cleanUp();
> 
> If we get an exception adding a second or third call-logger, this will kill
> all other existing call-loggers - which seems wrong/unexpected.

Of course it would, fixed.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-06 06:40:41 PDT
Created attachment 649253 [details] [diff] [review]
Rebased

Rebased due to HUDService changes
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-06 06:50:07 PDT
Created attachment 649254 [details] [diff] [review]
Rebased

Remove dependency on alias command
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-06 12:11:24 PDT
Comment on attachment 649254 [details] [diff] [review]
Rebased

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

::: browser/devtools/commandline/commands/GcliCalllogChromeCommands.jsm
@@ +30,5 @@
> + * 'calllog chromestart' command
> + */
> +gcli.addCommand({
> +  name: "calllog chromestart",
> +  description: gcli.lookup("calllogChromeStartDesc"),

So I think these commands will be visible to any GCLI users right?

Can I suggest:
  get hidden() { return gcli.hiddenByChromePref(); },

And then we'll need to add to gcli.jsm to make that work

  let prefSvc = "@mozilla.org/preferences-service;1";
  XPCOMUtils.defineLazyGetter(this, "prefBranch", function() {
    let prefService = Cc[prefSvc].getService(Ci.nsIPrefService);
    return prefService.getBranch(null).QueryInterface(Ci.nsIPrefBranch2);
  });

  gcli.hiddenByChromePref = function() {
    // will need to check the name of the pref
    return !prefBranch.prefHasUserValue("devtools.chrome.enabled");
  };

And to do the latter you'll need to muck with GCLI, which should be something like:

$ git clone git@github.com:joewalker/gcli.git
$ cd gcli
$ npm install .
$ vi mozilla/gcli/index.js
  (add code above)
$ node gcli firefox $FIREFOX_HOME
  (if you get problems about fs.existsSync, see https://github.com/joewalker/gcli/pull/3)
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-10 10:07:24 PDT
Created attachment 650921 [details] [diff] [review]
Addressed reviewers comments

Also see https://github.com/mozilla/gcli/tree/bug-771526 for the gcli side (included in this patch)

Works fine except for the
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-10 10:16:38 PDT
... except for the help (bug 781856).
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-14 06:42:05 PDT
Comment on attachment 650921 [details] [diff] [review]
Addressed reviewers comments

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

I like it, thanks.

::: browser/devtools/commandline/CmdCalllogChrome.jsm
@@ +37,5 @@
> +    {
> +      name: "sourceType",
> +      type: {
> +        name: "selection",
> +        data: ["content-variable", "chrome-variable", "jsm", "javascript"]

If we were being posh we might consider having 4 parameters, so you could say:
>> calllog chromestart --sourcejsm blah.jsm

That way we could have a type which is the list of known JSMs and another which is a list of the known javascript files, etc. However that's probably quite hard, so I think the way you've done it makes sense.

@@ +62,5 @@
> +    } else if (args.sourceType == "content-variable") {
> +      if (args.source in contentWindow) {
> +        globalObj = Cu.getGlobalForObject(contentWindow[args.source]);
> +      } else {
> +        return gcli.lookup("callLogChromeVarNotFoundContent");

I think we can probably do:
  throw new Error(gcli.lookup("..."));

The output should still show up as normal, but GCLI will know something went wrong, and will be able to act accordingly.

::: browser/devtools/commandline/test/browser_cmd_calllog_chrome.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +* http://creativecommons.org/publicdomain/zero/1.0/ */

Just random context: I've made some tweaks to the test helpers in GCLI. See for example https://github.com/joewalker/gcli/blob/multinode-770213/lib/gclitest/testIncomplete.js#L96
I'll probably add testing of async results to this and then port it across. But we certainly should not delay new tests for me to do that.
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-14 08:15:24 PDT
Created attachment 651777 [details] [diff] [review]
Throw error on var not found

(In reply to Joe Walker from comment #11)
> Comment on attachment 650921 [details] [diff] [review]
> Addressed reviewers comments
> 
> Review of attachment 650921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like it, thanks.
> 
> ::: browser/devtools/commandline/CmdCalllogChrome.jsm
> @@ +37,5 @@
> > +    {
> > +      name: "sourceType",
> > +      type: {
> > +        name: "selection",
> > +        data: ["content-variable", "chrome-variable", "jsm", "javascript"]
> 
> If we were being posh we might consider having 4 parameters, so you could
> say:
> >> calllog chromestart --sourcejsm blah.jsm
> 
> That way we could have a type which is the list of known JSMs and another
> which is a list of the known javascript files, etc. However that's probably
> quite hard, so I think the way you've done it makes sense.
> 
> @@ +62,5 @@
> > +    } else if (args.sourceType == "content-variable") {
> > +      if (args.source in contentWindow) {
> > +        globalObj = Cu.getGlobalForObject(contentWindow[args.source]);
> > +      } else {
> > +        return gcli.lookup("callLogChromeVarNotFoundContent");
> 
> I think we can probably do:
>   throw new Error(gcli.lookup("..."));
> 
> The output should still show up as normal, but GCLI will know something went
> wrong, and will be able to act accordingly.
> 

Done

> ::: browser/devtools/commandline/test/browser_cmd_calllog_chrome.js
> @@ +1,2 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +* http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> Just random context: I've made some tweaks to the test helpers in GCLI. See
> for example
> https://github.com/joewalker/gcli/blob/multinode-770213/lib/gclitest/
> testIncomplete.js#L96
> I'll probably add testing of async results to this and then port it across.
> But we certainly should not delay new tests for me to do that.
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-24 22:57:38 PDT
https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7
Comment 14 Dave Camp (:dcamp) 2012-08-25 17:04:46 PDT
https://hg.mozilla.org/mozilla-central/rev/e5588de9374c
Comment 15 Francesco Lodolo [:flod] 2012-08-27 23:31:02 PDT
Considering what you did in bug 782820, this string should spell "JavaScript"
callLogChromeEvalException=Evaluated javascript threw the following exception
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-28 02:56:22 PDT
(In reply to Francesco Lodolo [:flod] from comment #15)
> Considering what you did in bug 782820, this string should spell "JavaScript"
> callLogChromeEvalException=Evaluated javascript threw the following exception

bug 786193 logged

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