771526 - GCLI needs a command to log function calls in chrome content

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Blair has created the command at:
https://github.com/Unfocused/gcli-commands/blob/master/chrome-calllog.mozcmd

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1 WIP

Need to find out where the leaks are coming from ... I suspect a debugger bug

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

dcamp says to try dbg.enabled = false;

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

No leaks, green on try

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Rebased due to HUDService changes

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Remove dependency on alias command

Comment 8

[Joe Walker [:jwalker]]

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 [e-mail]: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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

... except for the help (bug 781856).

Comment 11

[Joe Walker [:jwalker]]

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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7

Comment 14

[Dave Camp (:dcamp)]

https://hg.mozilla.org/mozilla-central/rev/e5588de9374c

Comment 15

[Francesco Lodolo [:flod]]

Considering what you did in bug 782820, this string should spell "JavaScript"
callLogChromeEvalException=Evaluated javascript threw the following exception

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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