693269 - Ensure jstermhelpers work in JS mode in GCLI

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Joe Walker [:jwalker]]

JSTerm has a number of helpers:

$ = getElementById(...)     // ID
$$ = querySelectorAll(...)  // CSS Selector
$x = document.evaluate(...) // XPath
clear = console.clear
keys = Object.keys(...)
values = 
help = window.open('https://developer.mozilla.org/AppLinks/WebConsoleHelp')
$0 = Inspected Object
inspect = open object inspector
pprint = console.log++ ?
print = console.log ?

We should make JSTermHelpers work with GCLI.

Comment 1

[Joe Walker [:jwalker]]

To clarify, this is only in JS mode. So {$('id') works, but we're not making $ into a GCLI command right now.

Comment 2

[Joe Walker [:jwalker]]

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Comment 4

[Joe Walker [:jwalker]]

Bug triage, filter on PEGASUS.

Comment 5

[Joe Walker [:jwalker]]

Bug triage, filter on PEGASUS.

Comment 6

[Joe Walker [:jwalker]]

Attachment: upload 1

Fixes a number of related bugs (see dependencies)

Comment 7

[Mihai Sucan [:msucan]]

+          //if (!this._panelOpen) {
+          let propPanel = this.openPropertyPanel(aEvent.output.typed, aEvent.output.output, this);
+          propPanel.panel.setAttribute("hudId", this.hudId);
+          //  this._panelOpen = true;
+          //}

Why do you comment out the part with this._panelOpen? That prevents users from opening duplicate property panels for the same output.

Comment 8

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> +          //if (!this._panelOpen) {
> +          let propPanel = this.openPropertyPanel(aEvent.output.typed,
> aEvent.output.output, this);
> +          propPanel.panel.setAttribute("hudId", this.hudId);
> +          //  this._panelOpen = true;
> +          //}
> 
> Why do you comment out the part with this._panelOpen? That prevents users
> from opening duplicate property panels for the same output.

No good reason, other than debugging.
Will fix.

Comment 9

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=bb71eb7d4941

Comment 11

[Joe Walker [:jwalker]]

Attachment: upload 2

Removes comments as noted by Mihai

Comment 12

[Dave Camp (:dcamp)]

Comment on attachment 591785 [details] [diff] [review]
upload 2

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

::: browser/devtools/webconsole/HUDService.jsm
@@ +6978,5 @@
>      }
>  
> +    this.writeOutput(aEvent.output.typed, CATEGORY_INPUT);
> +
> +    // This is an experiment to see how much people yell when

... when your comments don't seem to have an ending? :)

@@ +7150,5 @@
> + * A fancy version of toString()
> + */
> +function nameObject(aObj) {
> +  if (aObj.constructor) {
> +    let matches = aObj.constructor.toString().match(/^function ([^(]*)\(/);

Why not constructor.name?

Comment 13

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=e6cb0ce0cbf3

Comment 14

[Joe Walker [:jwalker]]

Attachment: upload 3

(In reply to Dave Camp (:dcamp) from comment #12)
> Comment on attachment 591785 [details] [diff] [review]
> upload 2
> 
> Review of attachment 591785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +6978,5 @@
> >      }
> >  
> > +    this.writeOutput(aEvent.output.typed, CATEGORY_INPUT);
> > +
> > +    // This is an experiment to see how much people yell when
> 
> ... when your comments don't seem to have an ending? :)

Oops. It is really annoying when people
 
> @@ +7150,5 @@
> > + * A fancy version of toString()
> > + */
> > +function nameObject(aObj) {
> > +  if (aObj.constructor) {
> > +    let matches = aObj.constructor.toString().match(/^function ([^(]*)\(/);
> 
> Why not constructor.name?

Mostly because I'd forgotten that existed.
Both fixed.

Comment 15

[Joe Walker [:jwalker]]

Attachment: upload 4

Comment 16

[Dave Camp (:dcamp)]

Comment on attachment 592105 [details] [diff] [review]
upload 4

Mihai, can you look at the web console part of this please?

Comment 17

[Joe Walker [:jwalker]]

(In reply to Dave Camp (:dcamp) from comment #16)
> Comment on attachment 592105 [details] [diff] [review]
> upload 4
> 
> Mihai, can you look at the web console part of this please?

I think Mihai has already taken a quick look, but I'm happy for more comments (I fixed the one you made earlier)

I'd like to get this in for 12 if possible Mihai. Thanks.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 592105 [details] [diff] [review]
upload 4

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

Patch looks good! Only some questions below...

::: browser/devtools/webconsole/HUDService.jsm
@@ +4707,5 @@
>  
>    aJSTerm.sandbox.inspectrules = function JSTH_inspectrules(aNode)
>    {
>      aJSTerm.helperEvaluated = true;
> +    let doc = aJSTerm.inputNode.ownerDocument;

Why not aJSTerm.document? I see you added jsterm.document to have it synced with gcliterm.document. Or am I mistaken?

@@ +6828,5 @@
>    this.hudId = aHudId;
>    this.document = aDocument;
>    this.console = aConsole;
>    this.hintNode = aHintNode;
> +  this._window = this.context.get().QueryInterface(Ci.nsIDOMWindow);

Why do you store both the window object itself and the weak ref to the object? Is that needed?

::: browser/devtools/webconsole/test/browser_gcli_helpers.js
@@ +107,5 @@
> +  // Doesn't conform to check() format, bug 614561
> +  label = hud.outputNode.querySelector(".webconsole-msg-output");
> +  is(label.textContent.trim(), '0: "h"\n  1: "i"', 'pprint("hi") worked');
> +
> +  // Causes a memory leak. FIXME Bug 717892

Is this a memleak that happens only when GcliTerm is used?

Comment 19

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> Comment on attachment 592105 [details] [diff] [review]
> upload 4
> 
> Review of attachment 592105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good! Only some questions below...
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +4707,5 @@
> >  
> >    aJSTerm.sandbox.inspectrules = function JSTH_inspectrules(aNode)
> >    {
> >      aJSTerm.helperEvaluated = true;
> > +    let doc = aJSTerm.inputNode.ownerDocument;
> 
> Why not aJSTerm.document? I see you added jsterm.document to have it synced
> with gcliterm.document. Or am I mistaken?

You're right, and ideally this would be jsterm.document - I did the 2 things at different times, and was in 'dont change stuff' mode for the second bit. (At least I think that's what happened)

> @@ +6828,5 @@
> >    this.hudId = aHudId;
> >    this.document = aDocument;
> >    this.console = aConsole;
> >    this.hintNode = aHintNode;
> > +  this._window = this.context.get().QueryInterface(Ci.nsIDOMWindow);
> 
> Why do you store both the window object itself and the weak ref to the
> object? Is that needed?

I think we're not using the reference to this.context any more - I'll make a note to remove it.

> ::: browser/devtools/webconsole/test/browser_gcli_helpers.js
> @@ +107,5 @@
> > +  // Doesn't conform to check() format, bug 614561
> > +  label = hud.outputNode.querySelector(".webconsole-msg-output");
> > +  is(label.textContent.trim(), '0: "h"\n  1: "i"', 'pprint("hi") worked');
> > +
> > +  // Causes a memory leak. FIXME Bug 717892
> 
> Is this a memleak that happens only when GcliTerm is used?

Yes. Not sure why.

I'd like to get this in because the patch also fixes bug 701711, which in turn fixes bug 718857 which has some security implications. Makes sense?

Comment 20

[Joe Walker [:jwalker]]

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

Comment 21

[Mihai Sucan [:msucan]]

Yep, it makes sense. Thanks for your clarifications!

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9a5736e0f721