The default bug view has changed. See this FAQ.

GCLI needs a command to log function calls in chrome content

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gclicommands][fixed-in-fx-team])

Attachments

(1 attachment, 6 obsolete attachments)

Blair has created the command at:
https://github.com/Unfocused/gcli-commands/blob/master/chrome-calllog.mozcmd
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Created attachment 645260 [details] [diff] [review]
Patch 1 WIP

Need to find out where the leaks are coming from ... I suspect a debugger bug
dcamp says to try dbg.enabled = false;
Depends on: 776875
Created attachment 645751 [details] [diff] [review]
Patch

No leaks, green on try
Attachment #645260 - Attachment is obsolete: true
Attachment #645751 - Flags: review?(jwalker)
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.
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.
Attachment #645751 - Attachment is obsolete: true
Attachment #645751 - Flags: review?(jwalker)
Attachment #646195 - Flags: review?(jwalker)
Created attachment 649253 [details] [diff] [review]
Rebased

Rebased due to HUDService changes
Attachment #646195 - Attachment is obsolete: true
Attachment #646195 - Flags: review?(jwalker)
Attachment #649253 - Flags: review?(jwalker)
Created attachment 649254 [details] [diff] [review]
Rebased

Remove dependency on alias command
Attachment #649253 - Attachment is obsolete: true
Attachment #649253 - Flags: review?(jwalker)
Attachment #649254 - Flags: review?(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 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)
Attachment #649254 - Flags: review?(jwalker)
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
Attachment #649254 - Attachment is obsolete: true
Attachment #650921 - Flags: review?(jwalker)
Blocks: 781856
... except for the help (bug 781856).
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.
Attachment #650921 - Flags: review?(jwalker) → review+
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.
Attachment #650921 - Attachment is obsolete: true
Whiteboard: [gclicommands] → [gclicommands][review+]
https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7
Whiteboard: [gclicommands][review+] → [gclicommands][fixed-in-fx-team]

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e5588de9374c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Considering what you did in bug 782820, this string should spell "JavaScript"
callLogChromeEvalException=Evaluated javascript threw the following exception
Blocks: 786193
(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
You need to log in before you can comment on or make changes to this bug.