Last Comment Bug 693269 - Ensure jstermhelpers work in JS mode in GCLI
: Ensure jstermhelpers work in JS mode in GCLI
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 12
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
: 708237 (view as bug list)
Depends on:
Blocks: 701711 717596 717622
  Show dependency treegraph
 
Reported: 2011-10-10 04:00 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-01-28 06:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (31.29 KB, patch)
2012-01-13 05:42 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 2 (31.29 KB, patch)
2012-01-26 07:14 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 3 (31.33 KB, patch)
2012-01-27 04:42 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 4 (31.18 KB, patch)
2012-01-27 05:32 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
mihai.sucan: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-10 04:00:23 PDT
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] (needinfo me or ping on irc) 2011-10-10 04:01:33 PDT
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] (needinfo me or ping on irc) 2011-11-18 09:58:18 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 10:52:32 PST
*** Bug 708237 has been marked as a duplicate of this bug. ***
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-11 10:17:05 PST
Bug triage, filter on PEGASUS.
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-11 10:18:36 PST
Bug triage, filter on PEGASUS.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-13 05:42:26 PST
Created attachment 588393 [details] [diff] [review]
upload 1

Fixes a number of related bugs (see dependencies)
Comment 7 Mihai Sucan [:msucan] 2012-01-13 07:28:05 PST
+          //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] (needinfo me or ping on irc) 2012-01-13 08:14:46 PST
(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] (needinfo me or ping on irc) 2012-01-19 21:58:42 PST
https://tbpl.mozilla.org/?tree=Try&rev=bb71eb7d4941
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-24 03:46:12 PST
*** Bug 676722 has been marked as a duplicate of this bug. ***
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-26 07:14:54 PST
Created attachment 591785 [details] [diff] [review]
upload 2

Removes comments as noted by Mihai
Comment 12 Dave Camp (:dcamp) 2012-01-26 14:31:11 PST
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] (needinfo me or ping on irc) 2012-01-27 04:37:30 PST
https://tbpl.mozilla.org/?tree=Try&rev=e6cb0ce0cbf3
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-27 04:42:05 PST
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.
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-27 05:32:57 PST
Created attachment 592105 [details] [diff] [review]
upload 4
Comment 16 Dave Camp (:dcamp) 2012-01-27 08:02:27 PST
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] (needinfo me or ping on irc) 2012-01-27 08:22:10 PST
(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] 2012-01-27 08:32:24 PST
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] (needinfo me or ping on irc) 2012-01-27 10:21:27 PST
(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] (needinfo me or ping on irc) 2012-01-27 10:33:49 PST
https://tbpl.mozilla.org/?tree=Fx-Team&rev=9a5736e0f721
Comment 21 Mihai Sucan [:msucan] 2012-01-27 10:39:23 PST
Yep, it makes sense. Thanks for your clarifications!
Comment 22 Tim Taubert [:ttaubert] 2012-01-28 06:23:01 PST
https://hg.mozilla.org/mozilla-central/rev/9a5736e0f721

Note You need to log in before you can comment on or make changes to this bug.