Ensure jstermhelpers work in JS mode in GCLI

RESOLVED FIXED in Firefox 12

Status

defect
P2
normal
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Firefox 12
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
To clarify, this is only in JS mode. So {$('id') works, but we're not making $ into a GCLI command right now.
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Duplicate of this bug: 708237
Blocks: GCLI-12
No longer blocks: GCLI-SHIP
Bug triage, filter on PEGASUS.
Target Milestone: --- → Firefox 12
Bug triage, filter on PEGASUS.
Priority: -- → P2
Posted patch upload 1 (obsolete) — Splinter Review
Fixes a number of related bugs (see dependencies)
Attachment #588393 - Flags: review?(dcamp)
+          //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.
(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.
No longer blocks: GCLI-12
Duplicate of this bug: 676722
Posted patch upload 2 (obsolete) — Splinter Review
Removes comments as noted by Mihai
Attachment #588393 - Attachment is obsolete: true
Attachment #588393 - Flags: review?(dcamp)
Attachment #591785 - Flags: review?(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?
Attachment #591785 - Flags: review?(dcamp)
Posted patch upload 3 (obsolete) — Splinter Review
(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.
Attachment #591785 - Attachment is obsolete: true
Attachment #592099 - Flags: review?(dcamp)
Posted patch upload 4Splinter Review
Attachment #592099 - Attachment is obsolete: true
Attachment #592099 - Flags: review?(dcamp)
Attachment #592105 - Flags: review?(dcamp)
Comment on attachment 592105 [details] [diff] [review]
upload 4

Mihai, can you look at the web console part of this please?
Attachment #592105 - Flags: review?(mihai.sucan)
Attachment #592105 - Flags: review?(dcamp)
Attachment #592105 - Flags: review+
(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 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?
Attachment #592105 - Flags: review?(mihai.sucan) → review+
(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?
Yep, it makes sense. Thanks for your clarifications!
https://hg.mozilla.org/mozilla-central/rev/9a5736e0f721
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.