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: PTO - Michael Ratcliffe [:miker] [:mratcliffe]
: developer.tools
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 776875
Blocks: GCLICMD 781856 786193
  Show dependency treegraph
 
Reported: 2012-07-06 08:25 PDT by PTO - 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, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Patch (14.21 KB, patch)
2012-07-25 06:54 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Properly handle sandbox destruction in the event of an exception (14.39 KB, patch)
2012-07-26 10:32 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased (15.18 KB, patch)
2012-08-06 06:40 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased (14.66 KB, patch)
2012-08-06 06:50 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Addressed reviewers comments (16.29 KB, patch)
2012-08-10 10:07 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Splinter Review
Throw error on var not found (16.30 KB, patch)
2012-08-14 08:15 PDT, PTO - Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review

Description User image PTO - 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 User image PTO - 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 User image PTO - Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-24 16:01:26 PDT
dcamp says to try dbg.enabled = false;
Comment 3 User image PTO - 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image PTO - 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 User image PTO - 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 User image PTO - 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 User image 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 User image PTO - 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 User image PTO - Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-10 10:16:38 PDT
... except for the help (bug 781856).
Comment 11 User image 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 User image PTO - 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 User image 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 User image Dave Camp (:dcamp) 2012-08-25 17:04:46 PDT
https://hg.mozilla.org/mozilla-central/rev/e5588de9374c
Comment 15 User image 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 User image PTO - 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.