The default bug view has changed. See this FAQ.

Ensure jstermhelpers work in JS mode in GCLI

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Firefox 12
Points:
---
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: 711051
No longer blocks: 689605
Bug triage, filter on PEGASUS.
Target Milestone: --- → Firefox 12
Bug triage, filter on PEGASUS.
Priority: -- → P2
Blocks: 717622
Blocks: 717596
Blocks: 701711
Created attachment 588393 [details] [diff] [review]
upload 1

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: 711051
https://tbpl.mozilla.org/?tree=Try&rev=bb71eb7d4941
Duplicate of this bug: 676722
Created attachment 591785 [details] [diff] [review]
upload 2

Removes comments as noted by Mihai
Attachment #588393 - Attachment is obsolete: true
Attachment #588393 - Flags: review?(dcamp)
Attachment #591785 - Flags: review?(dcamp)

Comment 12

5 years ago
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)
https://tbpl.mozilla.org/?tree=Try&rev=e6cb0ce0cbf3
Created attachment 592099 [details] [diff] [review]
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.
Attachment #591785 - Attachment is obsolete: true
Attachment #592099 - Flags: review?(dcamp)
Created attachment 592105 [details] [diff] [review]
upload 4
Attachment #592099 - Attachment is obsolete: true
Attachment #592099 - Flags: review?(dcamp)
Attachment #592105 - Flags: review?(dcamp)

Comment 16

5 years ago
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?
https://tbpl.mozilla.org/?tree=Fx-Team&rev=9a5736e0f721
Whiteboard: [fixed-in-fx-team]
Yep, it makes sense. Thanks for your clarifications!
https://hg.mozilla.org/mozilla-central/rev/9a5736e0f721
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.