Last Comment Bug 656666 - Update HudService.jsm to allow GCLI integration
: Update HudService.jsm to allow GCLI integration
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on: 656668
Blocks: GCLI-API 690822
  Show dependency treegraph
 
Reported: 2011-05-12 10:16 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-10-14 06:07 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Updates HUDService.jsm to use GCLI (13.70 KB, patch)
2011-05-13 04:15 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 2 (14.51 KB, patch)
2011-05-23 10:42 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 3 (19.15 KB, patch)
2011-05-31 03:21 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 4 (31.89 KB, patch)
2011-08-25 04:28 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: feedback+
Details | Diff | Splinter Review
upload 5 (59.19 KB, patch)
2011-09-19 11:58 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: feedback+
Details | Diff | Splinter Review
upload 6 (59.51 KB, patch)
2011-09-22 13:48 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dao+bmo: review-
Details | Diff | Splinter Review
upload 7 (53.31 KB, patch)
2011-09-23 09:09 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dao+bmo: review-
Details | Diff | Splinter Review
upload 8 (49.97 KB, patch)
2011-09-26 10:41 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review-
l10n: feedback-
Details | Diff | Splinter Review
upload 9 (52.56 KB, patch)
2011-09-27 13:37 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review+
Details | Diff | Splinter Review
upload 10 (54.85 KB, patch)
2011-09-30 16:21 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dao+bmo: review+
l10n: feedback+
Details | Diff | Splinter Review
upload 11 (63.45 KB, patch)
2011-10-07 12:07 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
upload 12 (63.33 KB, patch)
2011-10-12 07:19 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-12 10:16:36 PDT
In order for GCLI to ship in web console we need to modify HUDService.jsm.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-13 04:15:58 PDT
Created attachment 532168 [details] [diff] [review]
Updates HUDService.jsm to use GCLI
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-23 10:42:32 PDT
Created attachment 534479 [details] [diff] [review]
upload 2

requires bug656668-gcli.patch and bug653140-require.patch
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-31 03:21:12 PDT
Created attachment 536263 [details] [diff] [review]
upload 3
Comment 4 Mihai Sucan [:msucan] 2011-08-12 09:54:35 PDT
Joe: when you have this patch ready I would also like to have a pass through the code (review or feedback), since the e10s work I am doing touches everything, and this is a pretty invasive set of changes for JSTerm. Thanks!
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-25 04:28:07 PDT
Created attachment 555699 [details] [diff] [review]
upload 4

Things I know this patch needs before we can commit it:
- UX approval of the method of displaying hints
- Removing any stray debug code
- Unification of CSS (post UX approval)
- Additional unit tests (GCLI has many unit tests, however not enough in the browser)
Comment 6 Mihai Sucan [:msucan] 2011-08-25 06:14:31 PDT
Comment on attachment 555699 [details] [diff] [review]
upload 4

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

Patch looks good. Feedback+ for the good direction this is going. Please address the comments below.

Also, please ask Dão for review, for the CSS changes, once UX review is done, once you have the patch ready for review. Looking forward to review the updated patch!

::: browser/devtools/webconsole/HUDService.jsm
@@ +2593,4 @@
>      // create Initial JS Workspace:
>      var context = Cu.getWeakReference(aContext);
>  
> +    let usegcli = false;

You do not need to change initializeJSTerm(). This method is going to be removed. It's also unused.

@@ +2979,5 @@
> + * @see Bug 592469
> + * @param nsIDOMNode aNode The node to search from
> + * @return string The HUD ID
> + */
> +function getHudIdFromNode(aNode) {

This method is not needed - once you remove the code changes from initializeJSTerm().

@@ +3477,5 @@
> +        this.jsterm = new JSTerm(context, aParentNode, mixin, this.console);
> +      }
> +      else {
> +        let hudId = getHudIdFromNode(aParentNode);
> +        let doc = aParentNode.ownerDocument;

Both of these lines are not needed. getHudIdFromNode() can be removed.

hudId is this.hudId.
doc is this.chromeDocument.

@@ +3512,5 @@
>        this.jsterm.context = Cu.getWeakReference(this.contentWindow);
>        this.jsterm.console = this.console;
>        this.jsterm.createSandbox();
> +
> +      this.gcliterm.reattachConsole(this.contentWindow, this.console);

This code assumes that jsterm is available, but the new line you added assume that gcliterm is *also* available. I doubt this code works properly. This needs to be fixed.

@@ +5452,5 @@
> +
> +    this.term = this.xulElementFactory("hbox");
> +    this.term.setAttribute("class", "jsterm-input-container");
> +    this.term.setAttribute("style", "direction: ltr;");
> +    //this.term.setAttribute("flex", "1");

Why is this commented out? Please uncomment if needed, or remove.

@@ +5625,5 @@
> +      aBody = aDocument.createTextNode(aBody.toString());
> +    }
> +    if (typeof aBody == "string") {
> +      aBody = aDocument.createTextNode(aBody);
> +    }

Please explain the reasoning for the changes you did in createMessageNode().

@@ +6763,5 @@
> +
> +Cu.import("resource:///modules/gcli.jsm");
> +let console = gcli._internal.console;
> +
> +Cu.import("resource:///modules/Commands.jsm");

I believe these should be at the top of the HUDService.jsm file, together with the rest of imports.

@@ +6771,5 @@
> + *
> + * Create a GcliTerminal or attach a GcliTerm input node to an existing output
> + * node, given by the parent node.
> + *
> + * @param {object} aContext

The coding style of this file doesn't use {} to wrap types for @params.

Also please add @constructor.

@@ +6787,5 @@
> +{
> +  this.context = aContext;
> +  this.hudId = aHudId;
> +  this.document = aDocument;
> +  this.console = aConsole;

this.console refers to the window.console API object in HeadsUpDisplay objects. In clearOutput() you use this.console as if it refers to the outputNode of a HUD object. This is confusing. Please fix.

@@ +6799,5 @@
> +    inputElement: this.inputNode,
> +    completeElement: this.completeNode,
> +    inputBackgroundElement: this.inputStack,
> +    hintElement: this.hintNode,
> +    completionPrompt: ''

Please use double quotes.

@@ +6815,5 @@
> +   *        the constructor expects the caller to create it. Yuck.
> +   * @param {object} aConsole
> +   *        Console object to use within the GcliTerm
> +   */
> +  reattachConsole: function(aContentWindow, aConsole) {

Coding style: please put the curly brace on a new line. Also please give the function a name.

@@ +6828,5 @@
> +   */
> +  createUI: function Gcli_createUI()
> +  {
> +    if (this.console == undefined) {
> +      throw new Error("this.console == undefined: can't happen");

This check is not needed. It won't happen. If it does, other parts of the code will throw anyway.

@@ +6857,5 @@
> +
> +  /**
> +   * Called by GCLI/canon when command line output changes
> +   */
> +  onCommandOutput: function Gcli_onCommandOutput(ev) {

Coding style: same as above, curly brace on new line. Also the function argument name prefix with "a". Variable names should be clearer. For example "event" instead of "ev". For an argument that would be "aEvent".

@@ +6873,5 @@
> +          '<div xmlns="http://www.w3.org/1999/xhtml"' +
> +          ' xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">' +
> +          output + '</div>');
> +
> +      output = this.document.createElementNS("http://www.w3.org/1999/xhtml", "div");

There's a constant, HTML_NS, you can use here.

@@ +6882,5 @@
> +
> +  /**
> +   * Setup the eval sandbox, should be called whenever we are attached
> +   */
> +  createSandbox: function Gcli_setupSandbox()

function Gcli_createSandbox()

@@ +6884,5 @@
> +   * Setup the eval sandbox, should be called whenever we are attached
> +   */
> +  createSandbox: function Gcli_setupSandbox()
> +  {
> +    var win = this.context.get().QueryInterface(Ci.nsIDOMWindow);

let instead of var.

@@ +6891,5 @@
> +    this.sandbox = new Cu.Sandbox(win, {
> +      sandboxPrototype: win,
> +      wantXrays: false
> +    });
> +    this.sandbox.console = this.console;

In jsterm we have jstermhelper functions. Some of them should be available here, in gcliterm as well, when we evaluate JS code.

Please open a follow up bug report about this. We need to fix jstermhelpers before we can entirely replace jsterm with gcliterm.

@@ +6914,5 @@
> +   *
> +   * @param {string} aExecuteString
> +   *        The String to evalutate
> +   */
> +  execute: function Gcli_execute(aExecuteString)

I don't find where this execute() method is invoked from. It seems unused.

@@ +6974,5 @@
> +
> +  /**
> +   * Remove all output from the HUD
> +   */
> +  clearOutput: function Gcli_clearOutput()

Same as above: this method seems unused.

@@ +6976,5 @@
> +   * Remove all output from the HUD
> +   */
> +  clearOutput: function Gcli_clearOutput()
> +  {
> +    let hudRef = HUDService.getHudReferenceForOutputNode(this.console);

getHudReferenceForOutputNode() has been removed, it's no longer available.

You can use getHudReferenceById(this.hudId).

::: browser/devtools/webconsole/test/browser/browser_webconsole_gcli_integrate.js
@@ +55,5 @@
> +
> +  let canon = require("gcli/canon");
> +  let tselcmd = canon.getCommand("tselarr");
> +  ok(tselcmd != null, "tselarr exists in the canon");
> +  ok(typeof tselcmd.getDescription === "function", "canon storing commands");

You can do:
is(typeof ..., "function", ...);

@@ +61,5 @@
> +
> +function testCallCommands() {
> +  let console = browser.contentWindow.wrappedJSObject.console;
> +
> +  let hudId = HUDService.displaysIndex()[0];

displaysIndex() has been removed, and other methods used by this test. Please update. :)

@@ +78,5 @@
> +  gcliterm._setInputValue("ec");
> +  event = browser.createEvent("KeyboardEvent");
> +  event.initKeyEvent("keyup", true, true, null, false,
> +                     false, false, false, 80, 0);
> +  gcliterm.inputNode.dispatchEvent(event);

Please use EventUtils.synthesizeKey() instead of synthetic keyboard events.

::: toolkit/themes/gnomestripe/global/webConsole.css
@@ +337,5 @@
> +  border-bottom: 2px dotted #999;
> +}
> +
> +.gcliterm-complete-node span.gcliERROR {
> +  color: #DDD; border-bottom: 2px dotted #F00;

Please put each property on a new line.

@@ +341,5 @@
> +  color: #DDD; border-bottom: 2px dotted #F00;
> +}
> +
> +.gcliterm-complete-node span.gcliPrompt {
> +  color: #66F; font-weight: bold;

Same as above.

(same comments apply to the other two stylesheets)
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-26 06:53:53 PDT
Thanks for the review. Most of your comments, I've addressed already, a few I'm working on. A few more, I've comments on - see below.
If I've missed it out, take it as done.


(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 555699 [details] [diff] [review]
> upload 4
> 
> Review of attachment 555699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. Feedback+ for the good direction this is going. Please
> address the comments below.
> 
> Also, please ask Dão for review, for the CSS changes, once UX review is
> done, once you have the patch ready for review. Looking forward to review
> the updated patch!
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +2593,4 @@
> >      // create Initial JS Workspace:
> >      var context = Cu.getWeakReference(aContext);
> >  
> > +    let usegcli = false;
> 
> You do not need to change initializeJSTerm(). This method is going to be
> removed. It's also unused.

Grumble :)
So why's it still there?
And is it worse unused, or unused AND broken?


> @@ +2979,5 @@
> > + * @see Bug 592469
> > + * @param nsIDOMNode aNode The node to search from
> > + * @return string The HUD ID
> > + */
> > +function getHudIdFromNode(aNode) {
> 
> This method is not needed - once you remove the code changes from
> initializeJSTerm().

See above.


> @@ +3477,5 @@
> > +        this.jsterm = new JSTerm(context, aParentNode, mixin, this.console);
> > +      }
> > +      else {
> > +        let hudId = getHudIdFromNode(aParentNode);
> > +        let doc = aParentNode.ownerDocument;
> 
> Both of these lines are not needed. getHudIdFromNode() can be removed.
> 
> hudId is this.hudId.
> doc is this.chromeDocument.

Fixed.
I agree - this is bizarre. I'll look into it.


> @@ +3512,5 @@
> >        this.jsterm.context = Cu.getWeakReference(this.contentWindow);
> >        this.jsterm.console = this.console;
> >        this.jsterm.createSandbox();
> > +
> > +      this.gcliterm.reattachConsole(this.contentWindow, this.console);
> 
> This code assumes that jsterm is available, but the new line you added
> assume that gcliterm is *also* available. I doubt this code works properly.
> This needs to be fixed.

This is bizarre. I'll work out what's going on.


> @@ +5625,5 @@
> > +      aBody = aDocument.createTextNode(aBody.toString());
> > +    }
> > +    if (typeof aBody == "string") {
> > +      aBody = aDocument.createTextNode(aBody);
> > +    }
> 
> Please explain the reasoning for the changes you did in createMessageNode().

It allows us to return DOM nodes for addition to the console output


> @@ +6787,5 @@
> > +{
> > +  this.context = aContext;
> > +  this.hudId = aHudId;
> > +  this.document = aDocument;
> > +  this.console = aConsole;
> 
> this.console refers to the window.console API object in HeadsUpDisplay
> objects. In clearOutput() you use this.console as if it refers to the
> outputNode of a HUD object. This is confusing. Please fix.

It's exactly as confusing as JSTerm ...

Perhaps the confusion is the reattachConsole() method?


> @@ +6828,5 @@
> > +   */
> > +  createUI: function Gcli_createUI()
> > +  {
> > +    if (this.console == undefined) {
> > +      throw new Error("this.console == undefined: can't happen");
> 
> This check is not needed. It won't happen. If it does, other parts of the
> code will throw anyway.

I know - it's mostly there because I threw away a bunch (more) redundant code
from JSTerm, and wanted to remember where it came from, and know if my logic was
wrong.
But it's gone now.


> @@ +6891,5 @@
> > +    this.sandbox = new Cu.Sandbox(win, {
> > +      sandboxPrototype: win,
> > +      wantXrays: false
> > +    });
> > +    this.sandbox.console = this.console;
> 
> In jsterm we have jstermhelper functions. Some of them should be available
> here, in gcliterm as well, when we evaluate JS code.
> 
> Please open a follow up bug report about this. We need to fix jstermhelpers
> before we can entirely replace jsterm with gcliterm.

Bug 671406


> @@ +6914,5 @@
> > +   *
> > +   * @param {string} aExecuteString
> > +   *        The String to evalutate
> > +   */
> > +  execute: function Gcli_execute(aExecuteString)
> 
> I don't find where this execute() method is invoked from. It seems unused.

It is unused, but won't be soon.


> @@ +6974,5 @@
> > +
> > +  /**
> > +   * Remove all output from the HUD
> > +   */
> > +  clearOutput: function Gcli_clearOutput()
> 
> Same as above: this method seems unused.

As above!
Comment 8 Mihai Sucan [:msucan] 2011-08-26 07:53:18 PDT
(In reply to Joe Walker from comment #7)
> Thanks for the review. Most of your comments, I've addressed already, a few
> I'm working on. A few more, I've comments on - see below.
> If I've missed it out, take it as done.

You're welcome. I am looking forward for the updated patch!


> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Comment on attachment 555699 [details] [diff] [review]
> > upload 4
> > 
> > Review of attachment 555699 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good. Feedback+ for the good direction this is going. Please
> > address the comments below.
> > 
> > Also, please ask Dão for review, for the CSS changes, once UX review is
> > done, once you have the patch ready for review. Looking forward to review
> > the updated patch!
> > 
> > ::: browser/devtools/webconsole/HUDService.jsm
> > @@ +2593,4 @@
> > >      // create Initial JS Workspace:
> > >      var context = Cu.getWeakReference(aContext);
> > >  
> > > +    let usegcli = false;
> > 
> > You do not need to change initializeJSTerm(). This method is going to be
> > removed. It's also unused.
> 
> Grumble :)
> So why's it still there?
> And is it worse unused, or unused AND broken?

It's still there because code cleanups are not often prioritized. :) The last code cleanup we did was big but it didn't include this part of the code. The e10s changes will remove this part of the code.

It's unused and it's broken. Your changes don't make it used, and don't "unbreak" it either. Hence, no need to update this part of the code.


> > @@ +5625,5 @@
> > > +      aBody = aDocument.createTextNode(aBody.toString());
> > > +    }
> > > +    if (typeof aBody == "string") {
> > > +      aBody = aDocument.createTextNode(aBody);
> > > +    }
> > 
> > Please explain the reasoning for the changes you did in createMessageNode().
> 
> It allows us to return DOM nodes for addition to the console output

That's the description of the createMessageNode() method: it creates DOM nodes that we add later to the Web Console output.

I asked about the specific changes you made in your patch. What's the reasoning? Please expand. Thank you!


> > @@ +6787,5 @@
> > > +{
> > > +  this.context = aContext;
> > > +  this.hudId = aHudId;
> > > +  this.document = aDocument;
> > > +  this.console = aConsole;
> > 
> > this.console refers to the window.console API object in HeadsUpDisplay
> > objects. In clearOutput() you use this.console as if it refers to the
> > outputNode of a HUD object. This is confusing. Please fix.
> 
> It's exactly as confusing as JSTerm ...
> 
> Perhaps the confusion is the reattachConsole() method?

Code's not great, indeed, but I believe there's no such confusion in JSTerm. In JSTerm this.console is well known to refer to the window.console API, and this.outputNode is always outputNode. GCLITerm in turn confuses the two.

The confusion is in clearOutput(), not in reattachConsole(). (both in GCLITerm) Please recheck. Thank you!
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-26 13:41:41 PDT
(In reply to Mihai Sucan [:msucan] from comment #8)
> (In reply to Joe Walker from comment #7)
> > Thanks for the review. Most of your comments, I've addressed already, a few
> > I'm working on. A few more, I've comments on - see below.
> > If I've missed it out, take it as done.
> 
> You're welcome. I am looking forward for the updated patch!
> 
> 
> > (In reply to Mihai Sucan [:msucan] from comment #6)
> > > Comment on attachment 555699 [details] [diff] [review]
> > > upload 4
> > > 
> > > Review of attachment 555699 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Patch looks good. Feedback+ for the good direction this is going. Please
> > > address the comments below.
> > > 
> > > Also, please ask Dão for review, for the CSS changes, once UX review is
> > > done, once you have the patch ready for review. Looking forward to review
> > > the updated patch!
> > > 
> > > ::: browser/devtools/webconsole/HUDService.jsm
> > > @@ +2593,4 @@
> > > >      // create Initial JS Workspace:
> > > >      var context = Cu.getWeakReference(aContext);
> > > >  
> > > > +    let usegcli = false;
> > > 
> > > You do not need to change initializeJSTerm(). This method is going to be
> > > removed. It's also unused.
> > 
> > Grumble :)
> > So why's it still there?
> > And is it worse unused, or unused AND broken?
> 
> It's still there because code cleanups are not often prioritized. :) The
> last code cleanup we did was big but it didn't include this part of the
> code. The e10s changes will remove this part of the code.
> 
> It's unused and it's broken. Your changes don't make it used, and don't
> "unbreak" it either. Hence, no need to update this part of the code.

I'm not complaining at you at all - sorry if it sounds that way - I think my complaint is about the process.
We're quick to get the right number of spaces between @param and the type, on the other hand we have things like this that actually costs. There's no doubt at all in my mind which deserves more attention, but the process actually prevents us from putting the attention in the right place.

One day I'll try to fix it, but that day isn't today.
I'll remove those changes.


> > > @@ +5625,5 @@
> > > > +      aBody = aDocument.createTextNode(aBody.toString());
> > > > +    }
> > > > +    if (typeof aBody == "string") {
> > > > +      aBody = aDocument.createTextNode(aBody);
> > > > +    }
> > > 
> > > Please explain the reasoning for the changes you did in createMessageNode().
> > 
> > It allows us to return DOM nodes for addition to the console output
> 
> That's the description of the createMessageNode() method: it creates DOM
> nodes that we add later to the Web Console output.
> 
> I asked about the specific changes you made in your patch. What's the
> reasoning? Please expand. Thank you!

I was attempting to describe what my changes did! - It allows us to add DOM nodes to the console in addition to strings. aBody can now be a DOM node.


> > > @@ +6787,5 @@
> > > > +{
> > > > +  this.context = aContext;
> > > > +  this.hudId = aHudId;
> > > > +  this.document = aDocument;
> > > > +  this.console = aConsole;
> > > 
> > > this.console refers to the window.console API object in HeadsUpDisplay
> > > objects. In clearOutput() you use this.console as if it refers to the
> > > outputNode of a HUD object. This is confusing. Please fix.
> > 
> > It's exactly as confusing as JSTerm ...
> > 
> > Perhaps the confusion is the reattachConsole() method?
> 
> Code's not great, indeed, but I believe there's no such confusion in JSTerm.
> In JSTerm this.console is well known to refer to the window.console API, and
> this.outputNode is always outputNode. GCLITerm in turn confuses the two.
> 
> The confusion is in clearOutput(), not in reattachConsole(). (both in
> GCLITerm) Please recheck. Thank you!

Will do.
Comment 10 Mihai Sucan [:msucan] 2011-08-26 13:48:33 PDT
(In reply to Joe Walker from comment #9)
> > > Grumble :)
> > > So why's it still there?
> > > And is it worse unused, or unused AND broken?
> > 
> > It's still there because code cleanups are not often prioritized. :) The
> > last code cleanup we did was big but it didn't include this part of the
> > code. The e10s changes will remove this part of the code.
> > 
> > It's unused and it's broken. Your changes don't make it used, and don't
> > "unbreak" it either. Hence, no need to update this part of the code.
> 
> I'm not complaining at you at all - sorry if it sounds that way - I think my
> complaint is about the process.

Hehe, no worries. I was just explaining why it's still there: it hasn't been cleaned-up (yet!).


> We're quick to get the right number of spaces between @param and the type,
> on the other hand we have things like this that actually costs. There's no
> doubt at all in my mind which deserves more attention, but the process
> actually prevents us from putting the attention in the right place.

You have a point, indeed. Good code quality makes it easier for others to make improvements. Unfortunately, the situation is given.


> > I asked about the specific changes you made in your patch. What's the
> > reasoning? Please expand. Thank you!
> 
> I was attempting to describe what my changes did! - It allows us to add DOM
> nodes to the console in addition to strings. aBody can now be a DOM node.

Ah! :) Thanks, that's clearer now!

Thanks for your comment and clarifications.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-19 11:58:25 PDT
Created attachment 560974 [details] [diff] [review]
upload 5


Feel free to take a look at browser/devtools/webconsole/Commands.jsm, but the commands should really be reviewed under separate bugs. Will split out later

Outstanding:
- Style changes on OSs other than windows
- Better testing
Comment 12 Mihai Sucan [:msucan] 2011-09-20 05:15:12 PDT
Comment on attachment 560974 [details] [diff] [review]
upload 5

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

Great work! Thanks for addressing my comments.


General comments:

- You should add a GcliTerm.destroy() method that is called when jsterm.destroy() is called.

If I am not mistaken, the current code will have memory leaks. For example, the effect of Commands.setDocument() is not undone.


- When I try to open the Web Console I get:

error
  undefined Missing module: i18n!gcli/nls/strings
error
  Error using module: gcli/firefox/index
  strings is null
    - stack = lookupKey("commands_eval_javascript")@resource:///modules/g_

Error: strings is null
Source File: resource:///modules/gcli.jsm
Line: 1481

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource:///modules/HUDService.jsm :: <TOP_LEVEL> :: line 57"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://browser/content/browser.js :: <TOP_LEVEL> :: line 14167"  data: no]

Not sure what's broken. I used the Gcli code from the dependency: bug 656668.


More comments below. Looking forward for the updated patch. Thanks!

::: browser/devtools/webconsole/Commands.jsm
@@ +126,5 @@
> + */
> +function ResourceType(typeSpec)
> +{
> +  if (typeSpec != null) {
> +    throw new Error("ResourceType can not be customized");

Why not?

(same question for FileType)

@@ +294,5 @@
> +  name: "version",
> +  description: "Show current FF version number",
> +  returnType: "string",
> +  exec: function(args, env) {
> +    return "You are running Firefox version 9.0a1 (2011-08-18)";

Is this command intended to exist further down the road? If yes, please update this to correctly display the version number. :)

::: browser/devtools/webconsole/HUDService.jsm
@@ +54,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource:///modules/NetworkHelper.jsm");
>  Cu.import("resource:///modules/PropertyPanel.jsm");
> +Cu.import("resource:///modules/gcli.jsm");
> +Cu.import("resource:///modules/Commands.jsm");

Please put these gcli and Commands objects in lazy getters, to avoid loading the JSMs when GCLI is not enabled.

Also, shouldn't Commands.jsm be ... GcliCommands.jsm? It's too generic.

@@ +1748,5 @@
>      // be leaks as some nodes has node.onclick = function; set and GC can't
>      // remove the nodes then.
> +    if (hud.jsterm) {
> +      hud.jsterm.clearOutput();
> +    }

Why not call the gcliterm.clearOutput() here? (when jsterm is not used)

There are some other places as well, where clearOutput() is called. Please check those and make sure the correct method is called.

@@ +3078,5 @@
> +      this.jsterm.inputNode.focus();
> +    }
> +    else {
> +      this.gcliterm.inputNode.focus();
> +    }

if (this.jsterm) {
  ...
}
else if (this.gcliterm) {
  ...
}

@@ +3196,5 @@
>          this.jsterm.inputNode.focus();
>        }
> +      else {
> +        this.gcliterm.inputNode.focus();
> +      }

Same as above.

@@ +3393,5 @@
> +    }
> +    else {
> +      if (this.gcliterm) {
> +        this.gcliterm.inputNode.focus();
> +      }

Same as above.

@@ +3458,5 @@
> +      }
> +      else {
> +        this.gcliterm = new GcliTerm(context, this.hudId, this.chromeDocument,
> +                                     this.console, this.hintNode);
> +        aParentNode.appendChild(this.gcliterm.element);

Would it make sense to do this.jsterm = new GcliTerm(...)?

It would ease your work within HUDService - you have most of the JSTerm API in GcliTerm anyway. You wouldn't have to add many ifs().

It would also ease updating the tests, which also rely on hud.jsterm methods. Thoughts?

@@ +6822,5 @@
> +
> +  this.createUI();
> +  this.createSandbox();
> +
> +  var window = this.context.get().QueryInterface(Ci.nsIDOMWindow);

s/var/let/

@@ +6849,5 @@
> +}
> +
> +GcliTerm.prototype = {
> +  /**
> +   * Remove the hint column from the display

Nit: some of the comments have a period at the end. Please be consistent. (I favor ending comments with a period.)

@@ +6853,5 @@
> +   * Remove the hint column from the display
> +   */
> +  hide: function() {
> +    this._prevDisplay = this.hintNode.parentNode.style.display;
> +    this.hintNode.parentNode.style.display = "none";

Why not use this.hintNode.parentNode.hidden = true?

in show() put it back to hidden = false.

@@ +6948,5 @@
> +  },
> +
> +  /**
> +   * Evaluates a string in the sandbox. The string is currently wrapped by a
> +   * with(window) { aString } construct, see bug 574033.

This comment doesn't hold true. :)

@@ +6989,5 @@
> +
> +  /**
> +   * Remove all output from the HUD
> +   */
> +  clearOutput: function Gcli_clearOutput()

This method doesn't look different from that in jsterm.

You could do clearOutput: JSTerm.prototype.clearOutput, like _formatResult and the rest of the methods.

@@ +7010,5 @@
> +  },
> +
> +  _formatResult: JSTerm.prototype.formatResult,
> +  _formatString: JSTerm.prototype.formatString,
> +  _getResultType: JSTerm.prototype.getResultType,

Are these methods used somewhere?

@@ +7020,5 @@
> +   *
> +   * @param string aNewValue
> +   *        The new value to set
> +   */
> +  _setInputValue: function Gcli_setInputValue(aNewValue)

Is this method used anywhere?

(also the comment doesn't hold true)

::: browser/devtools/webconsole/test/browser/browser_webconsole_gcli_integrate.js
@@ +9,5 @@
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/browser/test-console.html";
> +
> +registerCleanupFunction(function() {
> +    Services.prefs.setBoolPref("devtools.gcli.enable", false);

Services.prefs.clearUserPref("devtools.gcli.enable");

@@ +74,5 @@
> +  gcliterm.inputNode.focus();
> +  EventUtils.synthesizeKey("d");
> +  is(gcliterm.completeNode.value.replace(" ", ""), "", "Completion for \"ecd\"");
> +
> +  let outputNode = hudBox.querySelector(".hud-output-node");

You have:

let gcliterm = HUDService.getHudByWindow(content).gcliterm;

You can do:

let hud = HUDService.getHudByWindow(content);
let gcliterm = hud.gcliterm;

...

let outputNode = hud.outputNode;

(I suggest using elements directly from the HUD object, instead of querySelector() fun)
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-21 07:08:08 PDT
(In reply to Mihai Sucan [:msucan] from comment #12)
> Comment on attachment 560974 [details] [diff] [review]
> upload 5
> 
> Review of attachment 560974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work! Thanks for addressing my comments.
> 
> 
> General comments:
> 
> - You should add a GcliTerm.destroy() method that is called when
> jsterm.destroy() is called.
> 
> If I am not mistaken, the current code will have memory leaks. For example,
> the effect of Commands.setDocument() is not undone.

Done.

> - When I try to open the Web Console I get:
> 
> error
>   undefined Missing module: i18n!gcli/nls/strings
> error
>   Error using module: gcli/firefox/index
>   strings is null
>     - stack = lookupKey("commands_eval_javascript")@resource:///modules/g_
> 
> Error: strings is null
> Source File: resource:///modules/gcli.jsm
> Line: 1481
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" 
> nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
> resource:///modules/HUDService.jsm :: <TOP_LEVEL> :: line 57"  data: no]
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" 
> nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
> chrome://browser/content/browser.js :: <TOP_LEVEL> :: line 14167"  data: no]
> 
> Not sure what's broken. I used the Gcli code from the dependency: bug 656668.

Ah - You need the secret unreleased version of 656668.
I'll upload a new patch when I'm done with this review


> More comments below. Looking forward for the updated patch. Thanks!
> 
> ::: browser/devtools/webconsole/Commands.jsm
> @@ +126,5 @@
> > + */
> > +function ResourceType(typeSpec)
> > +{
> > +  if (typeSpec != null) {
> > +    throw new Error("ResourceType can not be customized");
> 
> Why not?
> 
> (same question for FileType)

Why can't ResourceType be customized? Because there is nothing to customize. NumberType is customizable, which means in addition to "type:'number'" you can do:
  type: { name:'number', max:10, min:0 }
ResourceType and FileType can't be customized in this way.

> @@ +294,5 @@
> > +  name: "version",
> > +  description: "Show current FF version number",
> > +  returnType: "string",
> > +  exec: function(args, env) {
> > +    return "You are running Firefox version 9.0a1 (2011-08-18)";
> 
> Is this command intended to exist further down the road? If yes, please
> update this to correctly display the version number. :)

It's not. I put it there when we were arguing about removing the version number from the about dialog :)
I'll take it out with the other commands.


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +54,5 @@
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource:///modules/NetworkHelper.jsm");
> >  Cu.import("resource:///modules/PropertyPanel.jsm");
> > +Cu.import("resource:///modules/gcli.jsm");
> > +Cu.import("resource:///modules/Commands.jsm");
> 
> Please put these gcli and Commands objects in lazy getters, to avoid loading
> the JSMs when GCLI is not enabled.

Done.


> Also, shouldn't Commands.jsm be ... GcliCommands.jsm? It's too generic.

Done.


> @@ +1748,5 @@
> >      // be leaks as some nodes has node.onclick = function; set and GC can't
> >      // remove the nodes then.
> > +    if (hud.jsterm) {
> > +      hud.jsterm.clearOutput();
> > +    }
> 
> Why not call the gcliterm.clearOutput() here? (when jsterm is not used)
> 
> There are some other places as well, where clearOutput() is called. Please
> check those and make sure the correct method is called.

Done. All the calls to clearOutput (except for JSTermHelper.clear - there is no JSTermHelper for GCLI) have calls to the gcli version.


> @@ +3078,5 @@
> > +      this.jsterm.inputNode.focus();
> > +    }
> > +    else {
> > +      this.gcliterm.inputNode.focus();
> > +    }
> 
> if (this.jsterm) {
>   ...
> }
> else if (this.gcliterm) {
>   ...
> }

Done.


> @@ +3196,5 @@
> >          this.jsterm.inputNode.focus();
> >        }
> > +      else {
> > +        this.gcliterm.inputNode.focus();
> > +      }
> 
> Same as above.

Done.


> @@ +3393,5 @@
> > +    }
> > +    else {
> > +      if (this.gcliterm) {
> > +        this.gcliterm.inputNode.focus();
> > +      }
> 
> Same as above.

Done.


> @@ +3458,5 @@
> > +      }
> > +      else {
> > +        this.gcliterm = new GcliTerm(context, this.hudId, this.chromeDocument,
> > +                                     this.console, this.hintNode);
> > +        aParentNode.appendChild(this.gcliterm.element);
> 
> Would it make sense to do this.jsterm = new GcliTerm(...)?
> 
> It would ease your work within HUDService - you have most of the JSTerm API
> in GcliTerm anyway. You wouldn't have to add many ifs().
> 
> It would also ease updating the tests, which also rely on hud.jsterm
> methods. Thoughts?

It's an interesting idea. The APIs are not identical, so it's not a no-brainer, but your benefits make sense none the less. I think the correct thing to do is to bear it in mind. 

We're not intending on maintaining both gcliterm and jsterm long-term so this problem will solve itself in the future anyway. It really comes down to short-term ease, I think.


> @@ +6822,5 @@
> > +
> > +  this.createUI();
> > +  this.createSandbox();
> > +
> > +  var window = this.context.get().QueryInterface(Ci.nsIDOMWindow);
> 
> s/var/let/

I really want to s/var/let/g ... :)
Done.


> @@ +6849,5 @@
> > +}
> > +
> > +GcliTerm.prototype = {
> > +  /**
> > +   * Remove the hint column from the display
> 
> Nit: some of the comments have a period at the end. Please be consistent. (I
> favor ending comments with a period.)

Done.


> @@ +6853,5 @@
> > +   * Remove the hint column from the display
> > +   */
> > +  hide: function() {
> > +    this._prevDisplay = this.hintNode.parentNode.style.display;
> > +    this.hintNode.parentNode.style.display = "none";
> 
> Why not use this.hintNode.parentNode.hidden = true?
> 
> in show() put it back to hidden = false.

Done.


> @@ +6948,5 @@
> > +  },
> > +
> > +  /**
> > +   * Evaluates a string in the sandbox. The string is currently wrapped by a
> > +   * with(window) { aString } construct, see bug 574033.
> 
> This comment doesn't hold true. :)

Done.


> @@ +6989,5 @@
> > +
> > +  /**
> > +   * Remove all output from the HUD
> > +   */
> > +  clearOutput: function Gcli_clearOutput()
> 
> This method doesn't look different from that in jsterm.
> 
> You could do clearOutput: JSTerm.prototype.clearOutput, like _formatResult
> and the rest of the methods.

It's not different. I've copied from JSTerm.


> @@ +7010,5 @@
> > +  },
> > +
> > +  _formatResult: JSTerm.prototype.formatResult,
> > +  _formatString: JSTerm.prototype.formatString,
> > +  _getResultType: JSTerm.prototype.getResultType,
> 
> Are these methods used somewhere?

No. Deleted along with _setInputValue


> @@ +7020,5 @@
> > +   *
> > +   * @param string aNewValue
> > +   *        The new value to set
> > +   */
> > +  _setInputValue: function Gcli_setInputValue(aNewValue)
> 
> Is this method used anywhere?
> 
> (also the comment doesn't hold true)

Done.

> :::
> browser/devtools/webconsole/test/browser/browser_webconsole_gcli_integrate.js
> @@ +9,5 @@
> > +
> > +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/browser/test-console.html";
> > +
> > +registerCleanupFunction(function() {
> > +    Services.prefs.setBoolPref("devtools.gcli.enable", false);
> 
> Services.prefs.clearUserPref("devtools.gcli.enable");

Done.


> @@ +74,5 @@
> > +  gcliterm.inputNode.focus();
> > +  EventUtils.synthesizeKey("d");
> > +  is(gcliterm.completeNode.value.replace(" ", ""), "", "Completion for \"ecd\"");
> > +
> > +  let outputNode = hudBox.querySelector(".hud-output-node");
> 
> You have:
> 
> let gcliterm = HUDService.getHudByWindow(content).gcliterm;
> 
> You can do:
> 
> let hud = HUDService.getHudByWindow(content);
> let gcliterm = hud.gcliterm;
> 
> ...
> 
> let outputNode = hud.outputNode;
> 
> (I suggest using elements directly from the HUD object, instead of
> querySelector() fun)

Done.
Comment 14 Mihai Sucan [:msucan] 2011-09-21 09:29:59 PDT
(In reply to Joe Walker from comment #13)
> > Error: uncaught exception: [Exception... "Component returned failure code:
> > 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" 
> > nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
> > chrome://browser/content/browser.js :: <TOP_LEVEL> :: line 14167"  data: no]
> > 
> > Not sure what's broken. I used the Gcli code from the dependency: bug 656668.
> 
> Ah - You need the secret unreleased version of 656668.
> I'll upload a new patch when I'm done with this review

Hehe, no worries. Looking forward for the unveiling of the new patch! :)


> > More comments below. Looking forward for the updated patch. Thanks!
> > 
> > ::: browser/devtools/webconsole/Commands.jsm
> > @@ +126,5 @@
> > > + */
> > > +function ResourceType(typeSpec)
> > > +{
> > > +  if (typeSpec != null) {
> > > +    throw new Error("ResourceType can not be customized");
> > 
> > Why not?
> > 
> > (same question for FileType)
> 
> Why can't ResourceType be customized? Because there is nothing to customize.
> NumberType is customizable, which means in addition to "type:'number'" you
> can do:
>   type: { name:'number', max:10, min:0 }
> ResourceType and FileType can't be customized in this way.

Thanks for your explanation. That makes sense now.


> > @@ +294,5 @@
> > > +  name: "version",
> > > +  description: "Show current FF version number",
> > > +  returnType: "string",
> > > +  exec: function(args, env) {
> > > +    return "You are running Firefox version 9.0a1 (2011-08-18)";
> > 
> > Is this command intended to exist further down the road? If yes, please
> > update this to correctly display the version number. :)
> 
> It's not. I put it there when we were arguing about removing the version
> number from the about dialog :)
> I'll take it out with the other commands.

Sure.

> > @@ +3458,5 @@
> > > +      }
> > > +      else {
> > > +        this.gcliterm = new GcliTerm(context, this.hudId, this.chromeDocument,
> > > +                                     this.console, this.hintNode);
> > > +        aParentNode.appendChild(this.gcliterm.element);
> > 
> > Would it make sense to do this.jsterm = new GcliTerm(...)?
> > 
> > It would ease your work within HUDService - you have most of the JSTerm API
> > in GcliTerm anyway. You wouldn't have to add many ifs().
> > 
> > It would also ease updating the tests, which also rely on hud.jsterm
> > methods. Thoughts?
> 
> It's an interesting idea. The APIs are not identical, so it's not a
> no-brainer, but your benefits make sense none the less. I think the correct
> thing to do is to bear it in mind. 
> 
> We're not intending on maintaining both gcliterm and jsterm long-term so
> this problem will solve itself in the future anyway. It really comes down to
> short-term ease, I think.

Agreed. For now we can leave it as it is.


Thanks for addressing the feedback comments! Looking forward to see the updated patches.
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-22 13:48:04 PDT
Created attachment 561873 [details] [diff] [review]
upload 6

I've just uploaded a patch to 656668 that goes with this one.

This should fix all your review comments and a few extra niggles that I found. For details you can see the commits marked 'Bug 656666 (review)' in this pull request: https://github.com/mozilla/gcli/pull/32

If you want to keep up with the latest top secret patches you can always take a look here: https://github.com/joewalker/gcli/tree/review/mozilla/patches
Comment 16 Dão Gottwald [:dao] 2011-09-22 13:54:13 PDT
Comment on attachment 561873 [details] [diff] [review]
upload 6

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -55,6 +55,7 @@
> <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
> <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
> <?xml-stylesheet href="chrome://global/skin/webConsole.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://browser/skin/devtools/gcli.css" type="text/css"?>
> <?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>

I think you should ASAP move the web console into an iframe ... :/

>+++ b/browser/themes/gnomestripe/browser/devtools/gcli.css

... because this file isn't acceptable in its current form. E.g. it violates <https://developer.mozilla.org/en/Writing_Efficient_CSS>. I'll care a lot less about this if it's limited to your private iframe rather than being loaded in browser.xul.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-23 09:09:39 PDT
Created attachment 562058 [details] [diff] [review]
upload 7


Dao - there was a ton of CSS in there that shouldn't have been - so it's 60% smaller now. Also I've simplified a few things so every selector now has a single class based rule.
Comment 18 Dão Gottwald [:dao] 2011-09-24 07:14:01 PDT
Comment on attachment 562058 [details] [diff] [review]
upload 7

>+.usegcli .jsterm-input-node,
>+.usegcli .jsterm-complete-node,

>+.gcliHints ul {

These are still violations. You may fix this, but I'd really prefer if the web console was separated from browser.xul sooner rather than later.

>+.gcliterm-input-node,
>+.gcliterm-complete-node {
>+  border: none;
>+  -moz-appearance: none;
>+  height: 100%;
>+  vertical-align: middle;
>+  background-color: transparent;
>+  font: 12px Consolas, Lucida Console, monospace;

Why do you specify a fixed font size here?
"Lucida Console" should be quoted.

>+  padding: 2px 0 0 16px;

This is adding padding to the left regardless of the UI direction. Some other rules do something similar. Is this correct for right-to-left languages?
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-24 08:09:04 PDT
Thanks for the review.

(In reply to Dão Gottwald [:dao] from comment #18)
> Comment on attachment 562058 [details] [diff] [review] [diff] [details] [review]
> upload 7
> 
> >+.usegcli .jsterm-input-node,
> >+.usegcli .jsterm-complete-node,
> 
> >+.gcliHints ul {
> 
> These are still violations. You may fix this, but I'd really prefer if the
> web console was separated from browser.xul sooner rather than later.

I was advised to handle preffed out CSS in this way (the usegcli class is inserted based on a pref). However, I'll mess with the JavaScript to alter those nodes directly in some way.

I raised bug 688981 to separate web console from browser.xul. We can handle this separately.


> >+.gcliterm-input-node,
> >+.gcliterm-complete-node {
> >+  border: none;
> >+  -moz-appearance: none;
> >+  height: 100%;
> >+  vertical-align: middle;
> >+  background-color: transparent;
> >+  font: 12px Consolas, Lucida Console, monospace;
> 
> Why do you specify a fixed font size here?
> "Lucida Console" should be quoted.

Because that's the way the existing web console does it:
https://hg.mozilla.org/integration/fx-team/file/5a45436b3c18/toolkit/themes/winstripe/global/webConsole.css#l138

I'm happy to quote "Lucida Console", but I think we should be using the same settings, and if this is actually wrong, fixing them both in a separate bug.


> >+  padding: 2px 0 0 16px;
> 
> This is adding padding to the left regardless of the UI direction. Some
> other rules do something similar. Is this correct for right-to-left
> languages?

Yes.
See bug 611549.
Comment 20 Mihai Sucan [:msucan] 2011-09-24 13:59:42 PDT
Joe / Dão: isn't there a way to just add the webConsole.css and gcli.css to the chrome window when the Web Console is open the first time? It should be less of a burden for the browser window when it's first open, and the vast majority of times users won't even load these style sheets (perf win).

This would be much easier to implement now ... until we move our code into an iframe - bug 688981. And the performance win should be equal.
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-26 07:51:51 PDT
    let mainWindow = this.chromeDocument.getElementById("main-window");
    if (!mainWindow.hasGcliCss) {
      let pi = this.chromeDocument.createProcessingInstruction("xml-stylesheet",
          "href='chrome://browser/skin/devtools/gcli.css' type='text/css'");
      let root = this.chromeDocument.getElementsByTagName('window')[0];
      root.parentNode.insertBefore(pi, root);
      mainWindow.hasGcliCss = true;
    }

I'm surprised that createProcessingInstruction exists. I thought the whole point of a processing instruction was that like a DTD, it was used in the parsing of a document, rather than being a runtime construct. However at least it gets us out of a hole.

What do we think of above?

We'll need a destroy function to avoid memory leaks, even though that's fairly silly given the context.

Dão: Do we have an numbers on how this slows things down?
Comment 22 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-26 10:41:08 PDT
Created attachment 562475 [details] [diff] [review]
upload 8

Now with dynamic reg, and no descendant rules
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-27 04:48:22 PDT
Closing bug 653119 and bug 653139 and directing l10n discusion to this bug.
Comment 24 Mihai Sucan [:msucan] 2011-09-27 05:14:35 PDT
Comment on attachment 562475 [details] [diff] [review]
upload 8

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

Thank you for addressing my feedback comments and for your answers to my questions.

- Some of the Web Console tests fail:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no displays found - Got 2, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no storage found
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no httpObserver found
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js | HUDService.deactivateHUDForContext(tab2) exception: TypeError: this.pi.parentNode is null
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_gcli_integrate.js | canon storing commands - Got undefined, expected function

Giving the patch r- for the PI problem and for the failing tests. More comments below. Please ask for l10n review as well.

Looking forward for the updated patch. Thank you!

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +42,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource:///modules/gcli.jsm");
> +
> +let bundleName = "chrome://browser/locale/gclicommands.properties";
> +let stringBundle = Services.strings.createBundle(bundleName);

You had a lazy getter for the stringBundle. Why did you change to no lazy getter?

(I am comparing "upload 5" to this patch)

@@ +66,5 @@
> +   * Undo the effects of Commands.setDocument()
> +   */
> +  unsetDocument: function Commands_setDocument() {
> +    document = undefined;
> +  }

Is this method needed? One can call Commands.setDocument().

@@ +81,5 @@
> +  try {
> +    return stringBundle.GetStringFromName(aName);
> +  } catch (ex) {
> +    Services.console.logStringMessage("Failure in lookup('" + aName + "')");
> +    throw new Error("Failure in lookup('" + aName + "')");

Why do you do Services.console.logStringMessage() and throw? The error console will end up showing two errors.

If you want to override the confusing error message of GetStringFromName() just throw a new Error() like you do now.

::: browser/devtools/webconsole/HUDService.jsm
@@ +82,5 @@
> +XPCOMUtils.defineLazyGetter(this, "Commands", function () {
> +  var obj = {};
> +  Cu.import("resource:///modules/GcliCommands.jsm", obj);
> +  return obj.Commands;
> +});

In the HUDService.jsm I'd like us to refer to GcliCommands instead of Commands. We already have a lot of variables with confusing names. :)

@@ +1488,5 @@
>      if (!aAnimated || hudRef.consolePanel) {
>        this.disableAnimation(hudId);
>      }
> +
> +    let mainWindow = aContext.ownerDocument.getElementById("main-window");

mainWindow is unused.

@@ +1526,5 @@
>  
>        window.focus();
>      }
> +
> +    this.pi.parentNode.removeChild(this.pi);

You need to clear the this.pi reference so it doesn't cause memleaks.

Still, this has some problems. HUDService is instanced once for the entire lifetime of the browser. You use this.pi as if there's one instance per chrome browser window, and this breaks things.

It's also a problem because the PI is added multiple times to the same chrome window.

Suggested solution: check if the chrome document already has the style sheet added. If not, add it.

In deactivateHUDForContext() we need to check if the style sheet is added, and remove it only if there's no other Web Console open in the given chrome window. The check we have in suspend() tells us if there's any Web Console open in *all* windows, so we can't use that check.

We need to track open HUDs per window. We could do the tracking on the PI node. Something like:

- When the PI node for the chrome window is created: pi._openHuds = [];

- Every time a HUD opens pi._openHuds.push(hudId);

(in activateHUDForContext())

Then in deactivateHUDForContext() we can filter out the hudId of the closed Web Console. When the array is empty we know we can remove the PI.

@@ +3448,5 @@
>    /**
> +   * The GcliTerm object that contains the console's GCLI
> +   *
> +   */
> +  gcliterm: null,

Nit: empty comment line.

@@ +3463,5 @@
>      var context = Cu.getWeakReference(aWindow);
>  
> +    let usegcli = false;
> +    try {
> +      usegcli = Services.prefs.getBoolPref("devtools.gcli.enable");

Please put this pref in browser/app/profile/firefox.js. Then you can remove the try-catch here.

@@ +3507,5 @@
>        this.jsterm.createSandbox();
> +    } else if (this.gcliterm) {
> +      this.gcliterm.reattachConsole(this.contentWindow, this.console);
> +    } else {
> +      this.createConsoleInput(this.contentWindow, this.consoleWrap, this.outputNode);

Nit: coding style. Add new lines after the curly braces.

if (...) {
...
}
else if (...) {
...
}
else {
...
}

(please keep consistency with the current code style of HUDService.jsm.)

@@ +6822,5 @@
> +/**
> + * GcliTerm
> + *
> + * Create a GcliTerminal or attach a GcliTerm input node to an existing output
> + * node, given by the parent node.

This comment is not accurate. The GcliTerm can only be attached to an existing output node. The other case is not supported (not even by JSTerm).

@@ +6827,5 @@
> + *
> + * @param object aContext
> + *        Usually a weak reference to an nsIDOMWindow, however this code
> + *        follows the convention of JSTerm where the context does not have to
> + *        be to a window.

Does JSTerm receive other types contexts? Only weak refs to window objects are used, afaik.

The intent was perhaps for other types to be allowed, but that's history that no longer matters. :)

(JSTerm will be phased out once GCLI comes in full force, hehe)

@@ +6852,5 @@
> +
> +  let window = this.context.get().QueryInterface(Ci.nsIDOMWindow);
> +
> +  this.show = this.show.bind(this);
> +  this.hide = this.hide.bind(this);

Why do you bind these methods here? This can cause unexpected behavior for a consumer of GcliTerm that would want to bind these methods to a different |this|. Bindings like these should be made by the consumers of GcliTerm, if they need anything like this.

@@ +6879,5 @@
> +GcliTerm.prototype = {
> +  /**
> +   * Remove the hint column from the display.
> +   */
> +  hide: function() {

Missing function name.

@@ +6886,5 @@
> +
> +  /**
> +   * Undo the effects of calling hide().
> +   */
> +  show: function() {

Same as above.

@@ +6907,5 @@
> +   * @param nsIDOMWindow aContentWindow
> +   *        Usually a nsIDOMWindow, however see the comments against the
> +   *        aContext parameter in the GcliTerm constructor. Also note the
> +   *        asymmetry between how this function creates the WeakReference where
> +   *        the constructor expects the caller to create it. Yuck.

You can fix the yuck. ;)

Where reattachConsole() is called just pass the content window object, without the weak ref. Do the same for the GcliTerm() constructor. We don't need to drag the yuckyness factor of JSTerm into GcliTerm.

::: browser/devtools/webconsole/test/browser/Makefile.in
@@ +109,4 @@
>  	browser_webconsole_bug_601177_log_levels.js \
>  	browser_webconsole_bug_597460_filter_scroll.js \
>  	browser_webconsole_gcli_require.js \
> +	browser_webconsole_gcli_integrate.js \

Nit: I made it a habit to include the bug number in the file name, or inside a comment within the file, so others can quickly track where it came from.

::: browser/locales/en-US/chrome/browser/gcli.properties
@@ +1,1 @@
> +# LOCALIZATION NOTE These strings are used inside the GCLI.

A localizer won't know what is GCLI. Please elaborate: where can the localizer find GCLI in Firefox?

(the same for gclicommands.properties)

::: browser/themes/gnomestripe/browser/devtools/gcli.css
@@ +85,5 @@
> +  margin-right: 10px;
> +  display: inline-block;
> +  overflow: hidden;
> +
> +  border-bottom: 0px !important;

Nit: unneeded empty line above.

@@ +173,5 @@
> +  padding: 2px;
> +}
> +
> +.gcliOption:hover {
> +  background-color: rgb(230, 230, 230);

Nit: some colors are hex (lower case, some upper case), rgb() and color names. Please aim for consistency. Thanks!

@@ +203,5 @@
> +  overflow-y: auto;
> +  max-width: 220px;
> +  overflow-x: hidden;
> +  margin: 0 10px;
> +  border-top: 0px !important;

s/0px/0/

::: browser/themes/winstripe/browser/jar.mn
@@ +232,4 @@
>          skin/classic/aero/browser/devtools/arrows.png                (devtools/arrows.png)
>          skin/classic/aero/browser/devtools/search.png                (devtools/search.png)
>          skin/classic/aero/browser/devtools/csshtmltree.css           (devtools/csshtmltree.css)
> +        skin/classic/aero/browser/devtools/gcli.css                  (devtools/gcli.css)

This only adds gcli.css for the Windows Vista+ users. If I am not mistaken you also need to add gcli.css for Windows XP users. Please check.

(confusing file...)
Comment 25 Mihai Sucan [:msucan] 2011-09-27 05:23:42 PDT
Did some user testing:

http://img.i7m.de/show/9mb2r.png

UI seems broken. I cannot go through the autocomplete options. Up/down doesn't work. Tab works. I cannot execute any command, I get an error:

Error: hud is null
Source File: resource:///modules/HUDService.jsm
Line: 5683
Comment 26 Axel Hecht [:Pike] 2011-09-27 05:52:40 PDT
Comment on attachment 562475 [details] [diff] [review]
upload 8

Please put the l10n files into a devtools-specific folder?

Also, please don't use acronyms without explaining them. GCLI, for example.

There were some typos in the comments, I found at least "too" vs "tool".

There should be an introduction comment to indicate that GCLI shouldn't be translated for some languages, something like:

"The correct localization of this file might be to keep it in English, or another language commonly spoken among web developers. You want to make that choice consistent across the developer tools. A good criteria is the language in which you'd find the best documentation on web development on the web."

Technical: "Del" as button label? If we already need to use acronyms in en-US, is that the right UI?
Comment 27 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-27 11:49:54 PDT
(In reply to Mihai Sucan [:msucan] from comment #24)
> Comment on attachment 562475 [details] [diff] [review] [diff] [details] [review]
> upload 8
> 
> Review of attachment 562475 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thank you for addressing my feedback comments and for your answers to my
> questions.
> 
> - Some of the Web Console tests fail:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no displays
> found - Got 2, expected 0
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no storage found
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_bug_595350_multiple_windows_and_tabs.js | no httpObserver
> found
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js |
> HUDService.deactivateHUDForContext(tab2) exception: TypeError:
> this.pi.parentNode is null
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_gcli_integrate.js | canon storing commands - Got
> undefined, expected function
> 
> Giving the patch r- for the PI problem and for the failing tests. More
> comments below. Please ask for l10n review as well.
> 
> Looking forward for the updated patch. Thank you!

With this commit, which isn't part of the patch you have, I get a clear run [1]:
https://github.com/joewalker/gcli/commit/04dd0b11be861f988a4c22e9d75e6d4e457cca94

There are definite fixes for the last 2 problems, and maybe for the earlier ones too.

I've also added community@localization.bugs, l10n@mozilla.com to the CC list and pointed to this bug from my l10n bugs.

[1] When I say 'clear run', there is more to be done with testing, including finding a way to run GCLIs unit tests in chrome, and fixing memory leaks (which I'm tracking under bug 689532)


> ::: browser/devtools/webconsole/GcliCommands.jsm
> @@ +42,5 @@
> > +Components.utils.import("resource://gre/modules/Services.jsm");
> > +Components.utils.import("resource:///modules/gcli.jsm");
> > +
> > +let bundleName = "chrome://browser/locale/gclicommands.properties";
> > +let stringBundle = Services.strings.createBundle(bundleName);
> 
> You had a lazy getter for the stringBundle. Why did you change to no lazy
> getter?
> 
> (I am comparing "upload 5" to this patch)

It served no purpose because loading this JSM causes the commands to be added which does a lookup.


> @@ +66,5 @@
> > +   * Undo the effects of Commands.setDocument()
> > +   */
> > +  unsetDocument: function Commands_setDocument() {
> > +    document = undefined;
> > +  }
> 
> Is this method needed? One can call Commands.setDocument().

It is technically redundant, however requiring users to do Commands.setDocument() supposes an understanding of how the commands are implemented.

However I did notice the poor function name!


> @@ +81,5 @@
> > +  try {
> > +    return stringBundle.GetStringFromName(aName);
> > +  } catch (ex) {
> > +    Services.console.logStringMessage("Failure in lookup('" + aName + "')");
> > +    throw new Error("Failure in lookup('" + aName + "')");
> 
> Why do you do Services.console.logStringMessage() and throw? The error
> console will end up showing two errors.
> 
> If you want to override the confusing error message of GetStringFromName()
> just throw a new Error() like you do now.

There's a reason for this I'm sure, but I can't think of it right now. It could be refactoring detritus from a time when I used dump. It should either not be there or commented as to why it's there, though, so I've taken it out, and we'll see.


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +82,5 @@
> > +XPCOMUtils.defineLazyGetter(this, "Commands", function () {
> > +  var obj = {};
> > +  Cu.import("resource:///modules/GcliCommands.jsm", obj);
> > +  return obj.Commands;
> > +});
> 
> In the HUDService.jsm I'd like us to refer to GcliCommands instead of
> Commands. We already have a lot of variables with confusing names. :)

Fixed.


> @@ +1488,5 @@
> >      if (!aAnimated || hudRef.consolePanel) {
> >        this.disableAnimation(hudId);
> >      }
> > +
> > +    let mainWindow = aContext.ownerDocument.getElementById("main-window");
> 
> mainWindow is unused.

Done.


> @@ +1526,5 @@
> >  
> >        window.focus();
> >      }
> > +
> > +    this.pi.parentNode.removeChild(this.pi);
> 
> You need to clear the this.pi reference so it doesn't cause memleaks.
> 
> Still, this has some problems. HUDService is instanced once for the entire
> lifetime of the browser. You use this.pi as if there's one instance per
> chrome browser window, and this breaks things.
> 
> It's also a problem because the PI is added multiple times to the same
> chrome window.
> 
> Suggested solution: check if the chrome document already has the style sheet
> added. If not, add it.
> 
> In deactivateHUDForContext() we need to check if the style sheet is added,
> and remove it only if there's no other Web Console open in the given chrome
> window. The check we have in suspend() tells us if there's any Web Console
> open in *all* windows, so we can't use that check.
> 
> We need to track open HUDs per window. We could do the tracking on the PI
> node. Something like:
> 
> - When the PI node for the chrome window is created: pi._openHuds = [];
> 
> - Every time a HUD opens pi._openHuds.push(hudId);
> 
> (in activateHUDForContext())
> 
> Then in deactivateHUDForContext() we can filter out the hudId of the closed
> Web Console. When the array is empty we know we can remove the PI.

Done.
ownerDocument.gcliCssProcInstr is something of a straw man.


> @@ +3448,5 @@
> >    /**
> > +   * The GcliTerm object that contains the console's GCLI
> > +   *
> > +   */
> > +  gcliterm: null,
> 
> Nit: empty comment line.

Spot where it's cut and paste from! Hint: 5 lines earlier ...
Fixed anyway :)


> @@ +3463,5 @@
> >      var context = Cu.getWeakReference(aWindow);
> >  
> > +    let usegcli = false;
> > +    try {
> > +      usegcli = Services.prefs.getBoolPref("devtools.gcli.enable");
> 
> Please put this pref in browser/app/profile/firefox.js. Then you can remove
> the try-catch here.

Bug 689610


> @@ +3507,5 @@
> >        this.jsterm.createSandbox();
> > +    } else if (this.gcliterm) {
> > +      this.gcliterm.reattachConsole(this.contentWindow, this.console);
> > +    } else {
> > +      this.createConsoleInput(this.contentWindow, this.consoleWrap, this.outputNode);
> 
> Nit: coding style. Add new lines after the curly braces.

Done.

> if (...) {
> ...
> }
> else if (...) {
> ...
> }
> else {
> ...
> }
> 
> (please keep consistency with the current code style of HUDService.jsm.)

Done.


> @@ +6822,5 @@
> > +/**
> > + * GcliTerm
> > + *
> > + * Create a GcliTerminal or attach a GcliTerm input node to an existing output
> > + * node, given by the parent node.
> 
> This comment is not accurate. The GcliTerm can only be attached to an
> existing output node. The other case is not supported (not even by JSTerm).

Another comment taken directly from JSTerm.
Also fixed.


> @@ +6827,5 @@
> > + *
> > + * @param object aContext
> > + *        Usually a weak reference to an nsIDOMWindow, however this code
> > + *        follows the convention of JSTerm where the context does not have to
> > + *        be to a window.
> 
> Does JSTerm receive other types contexts? Only weak refs to window objects
> are used, afaik.
> 
> The intent was perhaps for other types to be allowed, but that's history
> that no longer matters. :)
> 
> (JSTerm will be phased out once GCLI comes in full force, hehe)

Fixed. I also added some fullstops.


> @@ +6852,5 @@
> > +
> > +  let window = this.context.get().QueryInterface(Ci.nsIDOMWindow);
> > +
> > +  this.show = this.show.bind(this);
> > +  this.hide = this.hide.bind(this);
> 
> Why do you bind these methods here? This can cause unexpected behavior for a
> consumer of GcliTerm that would want to bind these methods to a different
> |this|. Bindings like these should be made by the consumers of GcliTerm, if
> they need anything like this.

Here's why I do it like this:
1. Because then the client doesn't need to know what |this| to use. Particularly for delegated event handlers (which this is) that's important. addEventListener has no concept of |this| so ultimately |this| *must* be client supplied. I think it's better to be consistent and say |this| for event listeners is always client supplied.
2a. Subclasses are inherently exposed to the implementations of their superclasses, so I'm not worried that subclasses won't know about the superclass implementation. They need to anyway.
2b. Anyone else that goes around randomly stealing functions without thinking about |this| deserves all the pain the get. :)
3. Ultimately it comes down to encapulation (which I think this style encourages) vs being able to play fast and lose with other peoples code (which this style discourages) I think that's a good trade-off.


> @@ +6879,5 @@
> > +GcliTerm.prototype = {
> > +  /**
> > +   * Remove the hint column from the display.
> > +   */
> > +  hide: function() {
> 
> Missing function name.

Done.


> @@ +6886,5 @@
> > +
> > +  /**
> > +   * Undo the effects of calling hide().
> > +   */
> > +  show: function() {
> 
> Same as above.

Done.
Also fixed the { placement.


> @@ +6907,5 @@
> > +   * @param nsIDOMWindow aContentWindow
> > +   *        Usually a nsIDOMWindow, however see the comments against the
> > +   *        aContext parameter in the GcliTerm constructor. Also note the
> > +   *        asymmetry between how this function creates the WeakReference where
> > +   *        the constructor expects the caller to create it. Yuck.
> 
> You can fix the yuck. ;)
> 
> Where reattachConsole() is called just pass the content window object,
> without the weak ref. Do the same for the GcliTerm() constructor. We don't
> need to drag the yuckyness factor of JSTerm into GcliTerm.

That means refactoring HUD_createConsoleInput a bit, which I was avoiding.
However since you ask - done.


> ::: browser/devtools/webconsole/test/browser/Makefile.in
> @@ +109,4 @@
> >  	browser_webconsole_bug_601177_log_levels.js \
> >  	browser_webconsole_bug_597460_filter_scroll.js \
> >  	browser_webconsole_gcli_require.js \
> > +	browser_webconsole_gcli_integrate.js \
> 
> Nit: I made it a habit to include the bug number in the file name, or inside
> a comment within the file, so others can quickly track where it came from.

For bug fixes that makes a lot of sense, but for enhancements, something like the feature page is going to be more useful, I think. I'll do something like that.


> ::: browser/locales/en-US/chrome/browser/gcli.properties
> @@ +1,1 @@
> > +# LOCALIZATION NOTE These strings are used inside the GCLI.
> 
> A localizer won't know what is GCLI. Please elaborate: where can the
> localizer find GCLI in Firefox?
> 
> (the same for gclicommands.properties)

Done.


> ::: browser/themes/gnomestripe/browser/devtools/gcli.css
> @@ +85,5 @@
> > +  margin-right: 10px;
> > +  display: inline-block;
> > +  overflow: hidden;
> > +
> > +  border-bottom: 0px !important;
> 
> Nit: unneeded empty line above.

Fixed.
Also fixed are the diplicate properties.


> @@ +173,5 @@
> > +  padding: 2px;
> > +}
> > +
> > +.gcliOption:hover {
> > +  background-color: rgb(230, 230, 230);
> 
> Nit: some colors are hex (lower case, some upper case), rgb() and color
> names. Please aim for consistency. Thanks!

I do consistently use #XYZ; style except for colors with transparency or which might be transparent (as is the case with the above) I can fix it for this, but I don't think there is any gain to be honest.


> ::: browser/themes/winstripe/browser/jar.mn
> @@ +232,4 @@
> >          skin/classic/aero/browser/devtools/arrows.png                (devtools/arrows.png)
> >          skin/classic/aero/browser/devtools/search.png                (devtools/search.png)
> >          skin/classic/aero/browser/devtools/csshtmltree.css           (devtools/csshtmltree.css)
> > +        skin/classic/aero/browser/devtools/gcli.css                  (devtools/gcli.css)
> 
> This only adds gcli.css for the Windows Vista+ users. If I am not mistaken
> you also need to add gcli.css for Windows XP users. Please check.
> 
> (confusing file...)

Hmm - that certainly not what I intended. Changed to what I did intend.
Comment 28 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-27 12:09:28 PDT
(In reply to Axel Hecht [:Pike] from comment #26)
> Comment on attachment 562475 [details] [diff] [review] [diff] [details] [review]
> upload 8
> 
> Please put the l10n files into a devtools-specific folder?

See bug 687851

> Also, please don't use acronyms without explaining them. GCLI, for example.

Fixed by removing the acronym.


> There were some typos in the comments, I found at least "too" vs "tool".

I've fixed what I've found.


> There should be an introduction comment to indicate that GCLI shouldn't be
> translated for some languages, something like:
> 
> "The correct localization of this file might be to keep it in English, or
> another language commonly spoken among web developers. You want to make that
> choice consistent across the developer tools. A good criteria is the
> language in which you'd find the best documentation on web development on
> the web."

I've added that. And raised bug 689685 to make sure we get this across all our strings files.


> Technical: "Del" as button label? If we already need to use acronyms in
> en-US, is that the right UI?

You're probably right. This is subject to UX review, and I've changed it to 'Delete'.

Thanks.
Comment 29 Mihai Sucan [:msucan] 2011-09-27 12:23:54 PDT
Thanks for your reply!

(In reply to Joe Walker from comment #27)
> > Looking forward for the updated patch. Thank you!
> 
> With this commit, which isn't part of the patch you have, I get a clear run
> [1]:
> https://github.com/joewalker/gcli/commit/
> 04dd0b11be861f988a4c22e9d75e6d4e457cca94
> 
> There are definite fixes for the last 2 problems, and maybe for the earlier
> ones too.

I used the latest patch from here and the latest patch from bug 656668 for all testing. Looking forward for the fixes to make it in the updated patches then. ;)

> > ::: browser/devtools/webconsole/GcliCommands.jsm
> > @@ +42,5 @@
> > > +Components.utils.import("resource://gre/modules/Services.jsm");
> > > +Components.utils.import("resource:///modules/gcli.jsm");
> > > +
> > > +let bundleName = "chrome://browser/locale/gclicommands.properties";
> > > +let stringBundle = Services.strings.createBundle(bundleName);
> > 
> > You had a lazy getter for the stringBundle. Why did you change to no lazy
> > getter?
> > 
> > (I am comparing "upload 5" to this patch)
> 
> It served no purpose because loading this JSM causes the commands to be
> added which does a lookup.

Makes sense then.


> > @@ +66,5 @@
> > > +   * Undo the effects of Commands.setDocument()
> > > +   */
> > > +  unsetDocument: function Commands_setDocument() {
> > > +    document = undefined;
> > > +  }
> > 
> > Is this method needed? One can call Commands.setDocument().
> 
> It is technically redundant, however requiring users to do
> Commands.setDocument() supposes an understanding of how the commands are
> implemented.
> 
> However I did notice the poor function name!

Good point.


> > @@ +81,5 @@
> > > +  try {
> > > +    return stringBundle.GetStringFromName(aName);
> > > +  } catch (ex) {
> > > +    Services.console.logStringMessage("Failure in lookup('" + aName + "')");
> > > +    throw new Error("Failure in lookup('" + aName + "')");
> > 
> > Why do you do Services.console.logStringMessage() and throw? The error
> > console will end up showing two errors.
> > 
> > If you want to override the confusing error message of GetStringFromName()
> > just throw a new Error() like you do now.
> 
> There's a reason for this I'm sure, but I can't think of it right now. It
> could be refactoring detritus from a time when I used dump. It should either
> not be there or commented as to why it's there, though, so I've taken it
> out, and we'll see.

Thanks!


> > @@ +1526,5 @@
> > >  
> > >        window.focus();
> > >      }
> > > +
> > > +    this.pi.parentNode.removeChild(this.pi);
> > 
> > You need to clear the this.pi reference so it doesn't cause memleaks.
> > 
> > Still, this has some problems. HUDService is instanced once for the entire
> > lifetime of the browser. You use this.pi as if there's one instance per
> > chrome browser window, and this breaks things.
> > 
> > It's also a problem because the PI is added multiple times to the same
> > chrome window.
> > 
> > Suggested solution: check if the chrome document already has the style sheet
> > added. If not, add it.
> > 
> > In deactivateHUDForContext() we need to check if the style sheet is added,
> > and remove it only if there's no other Web Console open in the given chrome
> > window. The check we have in suspend() tells us if there's any Web Console
> > open in *all* windows, so we can't use that check.
> > 
> > We need to track open HUDs per window. We could do the tracking on the PI
> > node. Something like:
> > 
> > - When the PI node for the chrome window is created: pi._openHuds = [];
> > 
> > - Every time a HUD opens pi._openHuds.push(hudId);
> > 
> > (in activateHUDForContext())
> > 
> > Then in deactivateHUDForContext() we can filter out the hudId of the closed
> > Web Console. When the array is empty we know we can remove the PI.
> 
> Done.
> ownerDocument.gcliCssProcInstr is something of a straw man.

Sure. Whatever works. :)


> > @@ +3448,5 @@
> > >    /**
> > > +   * The GcliTerm object that contains the console's GCLI
> > > +   *
> > > +   */
> > > +  gcliterm: null,
> > 
> > Nit: empty comment line.
> 
> Spot where it's cut and paste from! Hint: 5 lines earlier ...
> Fixed anyway :)

Yes, I know! :) Hehe


> > @@ +3463,5 @@
> > >      var context = Cu.getWeakReference(aWindow);
> > >  
> > > +    let usegcli = false;
> > > +    try {
> > > +      usegcli = Services.prefs.getBoolPref("devtools.gcli.enable");
> > 
> > Please put this pref in browser/app/profile/firefox.js. Then you can remove
> > the try-catch here.
> 
> Bug 689610

Cool! Please note that the bug should be fixed here. (I assume the separate bug is only for tracking purposes.)

> > @@ +6852,5 @@
> > > +
> > > +  let window = this.context.get().QueryInterface(Ci.nsIDOMWindow);
> > > +
> > > +  this.show = this.show.bind(this);
> > > +  this.hide = this.hide.bind(this);
> > 
> > Why do you bind these methods here? This can cause unexpected behavior for a
> > consumer of GcliTerm that would want to bind these methods to a different
> > |this|. Bindings like these should be made by the consumers of GcliTerm, if
> > they need anything like this.
> 
> Here's why I do it like this:
> 1. Because then the client doesn't need to know what |this| to use.
> Particularly for delegated event handlers (which this is) that's important.
> addEventListener has no concept of |this| so ultimately |this| *must* be
> client supplied. I think it's better to be consistent and say |this| for
> event listeners is always client supplied.
> 2a. Subclasses are inherently exposed to the implementations of their
> superclasses, so I'm not worried that subclasses won't know about the
> superclass implementation. They need to anyway.
> 2b. Anyone else that goes around randomly stealing functions without
> thinking about |this| deserves all the pain the get. :)
> 3. Ultimately it comes down to encapulation (which I think this style
> encourages) vs being able to play fast and lose with other peoples code
> (which this style discourages) I think that's a good trade-off.

My review comment is based on bug 585991 comment 16. I would add that it makes sense for me to allow methods that live on objects to have their |this| changed. By binding the |this| of the method ... you defeat the purpose of methods (as a concept). The method of the object becomes a "standalone" function that always acts on the bound |this|.

You pass the GcliTerm instance to Gcli, anyway. Just bind() there, however you need.

In an abstract view: it's not the "duty" of the GcliTerm author to "know" that show()/hide() will be used for addEventListener() or for other places where bind() is needed. Otherwise we'd have to bind() every method because it might be used for some similar reason.

Ultimately, this is nit-picking. :) I favor not to use bind() here, that's all. You can leave it as it is, if you believe that's best.


> > @@ +6907,5 @@
> > > +   * @param nsIDOMWindow aContentWindow
> > > +   *        Usually a nsIDOMWindow, however see the comments against the
> > > +   *        aContext parameter in the GcliTerm constructor. Also note the
> > > +   *        asymmetry between how this function creates the WeakReference where
> > > +   *        the constructor expects the caller to create it. Yuck.
> > 
> > You can fix the yuck. ;)
> > 
> > Where reattachConsole() is called just pass the content window object,
> > without the weak ref. Do the same for the GcliTerm() constructor. We don't
> > need to drag the yuckyness factor of JSTerm into GcliTerm.
> 
> That means refactoring HUD_createConsoleInput a bit, which I was avoiding.
> However since you ask - done.

Thank you!
Comment 30 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-27 13:32:28 PDT
(In reply to Mihai Sucan [:msucan] from comment #29)
> Thanks for your reply!
> 
> > > @@ +3463,5 @@
> > > >      var context = Cu.getWeakReference(aWindow);
> > > >  
> > > > +    let usegcli = false;
> > > > +    try {
> > > > +      usegcli = Services.prefs.getBoolPref("devtools.gcli.enable");
> > > 
> > > Please put this pref in browser/app/profile/firefox.js. Then you can remove
> > > the try-catch here.
> > 
> > Bug 689610
> 
> Cool! Please note that the bug should be fixed here. (I assume the separate
> bug is only for tracking purposes.)

It may be academic, in that I expect that I'll get to it before this lands, but I wanted to separate it out because I wasn't sure that it needed to. It's not broken now, and the more files these patches touch, the more rebasing that's needed.

Does it cause a problem as it is?
Comment 31 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-27 13:37:08 PDT
Created attachment 562855 [details] [diff] [review]
upload 9

Fixes from today's reviews.
Comment 32 Mihai Sucan [:msucan] 2011-09-27 13:56:33 PDT
(In reply to Joe Walker from comment #30)
> (In reply to Mihai Sucan [:msucan] from comment #29)
> > Thanks for your reply!
> > 
> > > > @@ +3463,5 @@
> > > > >      var context = Cu.getWeakReference(aWindow);
> > > > >  
> > > > > +    let usegcli = false;
> > > > > +    try {
> > > > > +      usegcli = Services.prefs.getBoolPref("devtools.gcli.enable");
> > > > 
> > > > Please put this pref in browser/app/profile/firefox.js. Then you can remove
> > > > the try-catch here.
> > > 
> > > Bug 689610
> > 
> > Cool! Please note that the bug should be fixed here. (I assume the separate
> > bug is only for tracking purposes.)
> 
> It may be academic, in that I expect that I'll get to it before this lands,
> but I wanted to separate it out because I wasn't sure that it needed to.
> It's not broken now, and the more files these patches touch, the more
> rebasing that's needed.
> 
> Does it cause a problem as it is?

No, but it's something that I would expect as part of the "basics": if we add a pref, we need it in firefox.js as well. It's not something that will cause rebase headaches (firefox.js is a simple file).

Thank you!
Comment 33 Mihai Sucan [:msucan] 2011-09-28 04:06:23 PDT
Comment on attachment 562855 [details] [diff] [review]
upload 9

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

Thanks for your update! Patch looks fine, r+! All tests pass locally.

Two comments still need to be addressed:

- profile/firefox.js update.
- the UI is still broken for me, see comment 25. This, I assume, still depends on updates for bug 656668.

More comments below. Thank you!

::: browser/devtools/webconsole/HUDService.jsm
@@ +1495,5 @@
> +    let procInstr = aContext.ownerDocument.gcliCssProcInstr;
> +    if (!procInstr) {
> +      procInstr = aContext.ownerDocument.createProcessingInstruction(
> +              "xml-stylesheet",
> +              "href='chrome://browser/skin/devtools/gcli.css' type='text/css'");

Nit: indentation is weird.

Maybe something like:

let chromeDoc = aContext.ownerDocument;
let procInstr = chromeDoc.gcliCssProcInstr;
if (!procInstr) {
  procInstr = chromeDoc.createProcessingInstruction("xml-stylesheet",
    "href='chrome://browser/skin/devtools/gcli.css' type='text/css'");
  procInstr.contexts = [];

  let root = chromeDoc.getElementsByTagName("window")[0];
  root.parentNode.insertBefore(procInstr, root);
  chromeDoc.gcliCssProcInstr = procInstr;
}
procInstr.contexts.push(hudId);

@@ +1502,5 @@
> +      let root = aContext.ownerDocument.getElementsByTagName('window')[0];
> +      root.parentNode.insertBefore(procInstr, root);
> +      aContext.ownerDocument.gcliCssProcInstr = procInstr;
> +    }
> +    procInstr.contexts.push(aContext);

Please push the hudIds instead of the xul:tab refs - less potential for memleaks and other negative fun.

@@ +1546,5 @@
> +    if (procInstr.contexts.length == 0 && procInstr.parentNode) {
> +      procInstr.parentNode.removeChild(procInstr);
> +      delete aContext.ownerDocument.gcliCssProcInstr;
> +    }
> +    delete procInstr;

This line is not needed. The variable will be GC'ed once the function call ends.
Comment 34 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-30 16:00:28 PDT
Try build:
https://tbpl.mozilla.org/?tree=Try&rev=c5a5cfe100ae
It looks clean to be. Some doubts as to WinXP debug, but the same leak appears to be happening elsewhere.
https://tbpl.mozilla.org/?tree=Fx-Team&rev=713f76a29f10
Comment 35 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-30 16:21:48 PDT
Created attachment 563884 [details] [diff] [review]
upload 10

I'm keen to get any further comments from dao/l10n. I think I've addressed all you comments so far.
Thanks.
Comment 36 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-30 16:23:37 PDT
Comment on attachment 563884 [details] [diff] [review]
upload 10

Sorry Axel - filled you in, in the wrong slot.
Comment 37 Dão Gottwald [:dao] 2011-10-03 08:23:44 PDT
Comment on attachment 563884 [details] [diff] [review]
upload 10

>+  background: url("chrome://global/skin/icons/commandline.png") 4px center no-repeat;

Please file a bug on moving this file to the devtools theme directory.
Comment 38 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-03 11:07:27 PDT
(In reply to Dão Gottwald [:dao] from comment #37)
> Please file a bug on moving this file to the devtools theme directory.

Bug 691419
Comment 39 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-03 11:08:16 PDT
Also thanks for the r+
Comment 40 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-05 13:46:15 PDT
Axel - might you be able to take another look at the attached patch?
Thanks.
Comment 41 Axel Hecht [:Pike] 2011-10-06 04:46:40 PDT
Comment on attachment 563884 [details] [diff] [review]
upload 10

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

Looks OK, I got one comment on the comment below.

Regarding file location, please don't land and move later. Our localizers are all on toolchains that don't support that. Neither do the tools detect file moves in en-US, nor are they cooperative for folks changing the source without the tool.
Also, even if the tools wouldn't make this horrible, you'd still make dozens of folks do useless work.

::: browser/locales/en-US/chrome/browser/gcli.properties
@@ +1,4 @@
> +# LOCALIZATION NOTE These strings are used inside the Web Console
> +# command line which is available from the Web Developer sub-menu
> +# -> 'Web Console'.
> +# The correct localization of this file might be to keep it in# English, or another language commonly spoken among web developers.# You want to make that choice consistent across the developer tools.# A good criteria is the language in which you'd find the best# documentation on web development on the web.

There are some linebreaks missing here?
Comment 42 Mihai Sucan [:msucan] 2011-10-06 05:03:41 PDT
Comment on attachment 563884 [details] [diff] [review]
upload 10

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

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +167,5 @@
> +/**
> + * 'console close' command
> + */
> +gcli.addCommand({
> +  name: "console clear",

Wrong comment. It says "console close" instead of "console clear".
Comment 43 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 05:42:30 PDT
(In reply to Axel Hecht [:Pike] from comment #41)
> Comment on attachment 563884 [details] [diff] [review] [diff] [details] [review]
> upload 10
> 
> Review of attachment 563884 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, I got one comment on the comment below.
> 
> Regarding file location, please don't land and move later. Our localizers
> are all on toolchains that don't support that. Neither do the tools detect
> file moves in en-US, nor are they cooperative for folks changing the source
> without the tool.
> Also, even if the tools wouldn't make this horrible, you'd still make dozens
> of folks do useless work.

OK. A per bug 687851, I'm moving these to:
  /browser/locales/en-US/chrome/browser/devtools/webconsole/gcli*.properties

> ::: browser/locales/en-US/chrome/browser/gcli.properties
> @@ +1,4 @@
> > +# LOCALIZATION NOTE These strings are used inside the Web Console
> > +# command line which is available from the Web Developer sub-menu
> > +# -> 'Web Console'.
> > +# The correct localization of this file might be to keep it in# English, or another language commonly spoken among web developers.# You want to make that choice consistent across the developer tools.# A good criteria is the language in which you'd find the best# documentation on web development on the web.
> 
> There are some linebreaks missing here?

Oops, yes - fixed.
Thanks.
Comment 44 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 12:07:06 PDT
Created attachment 565604 [details] [diff] [review]
upload 11
Comment 45 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 12:48:17 PDT
I've pushed this and bug 656668 to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d1870fcdc3ba

Assuming that's green, we can land this and 656668.
Comment 46 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-12 07:19:01 PDT
Created attachment 566525 [details] [diff] [review]
upload 12

Alter localization strings in line with bug 687851
Comment 47 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-12 07:52:08 PDT
Try log:
https://tbpl.mozilla.org/?tree=Try&rev=f2eedda104b1
Comment 48 Rob Campbell [:rc] (:robcee) 2011-10-12 10:13:21 PDT
upload 12: https://hg.mozilla.org/integration/fx-team/rev/c41530066c72
Comment 49 Rob Campbell [:rc] (:robcee) 2011-10-13 09:45:59 PDT
https://hg.mozilla.org/mozilla-central/rev/c41530066c72
Comment 50 Rimas Kudelis 2011-10-13 11:55:18 PDT
A moan from a localizer about the following string:

# LOCALIZATION NOTE (fieldSelectionSelect): When a command has a parameter
# that has a number of pre-defined options the user interface presents these
# in a drop-down menu, where the first 'option' is an indicator that a
# selection should be made. This string describes that first option.
fieldSelectionSelect=Select a %S ...

1) is there any reason why three dots have been used here instead of the unicode ellipsis character?
2) I guess there's no need for a space before the ellipsis. Is there?
3) I'm not sure "Select a %S" is flexible enough for localization. If we know beforehand what %S might be, and the set of these strings isn't big, I suggest to use separate properties for each %S. Also, an example of %S would be helpful (again, if it's known already, e.g. Select a class...)
Comment 51 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-14 06:03:16 PDT
(In reply to Rimas Kudelis from comment #50)
> A moan from a localizer about the following string:
> 
> # LOCALIZATION NOTE (fieldSelectionSelect): When a command has a parameter
> # that has a number of pre-defined options the user interface presents these
> # in a drop-down menu, where the first 'option' is an indicator that a
> # selection should be made. This string describes that first option.
> fieldSelectionSelect=Select a %S ...
> 
> 1) is there any reason why three dots have been used here instead of the
> unicode ellipsis character?
> 2) I guess there's no need for a space before the ellipsis. Is there?
> 3b) Also, an example of %S would
> be helpful (again, if it's known already, e.g. Select a class...)

We can fix all these easily, however not in this bug as it has landed. See bug 694542.

> 3a) I'm not sure "Select a %S" is flexible enough for localization. If we
> know beforehand what %S might be, and the set of these strings isn't big, I
> suggest to use separate properties for each %S.

The trouble is, %S isn't known beforehand.

Thanks.
Comment 52 Rimas Kudelis 2011-10-14 06:07:55 PDT
(In reply to Joe Walker from comment #51)
> (In reply to Rimas Kudelis from comment #50)
> > A moan from a localizer about the following string:
> > 
> > # LOCALIZATION NOTE (fieldSelectionSelect): When a command has a parameter
> > # that has a number of pre-defined options the user interface presents these
> > # in a drop-down menu, where the first 'option' is an indicator that a
> > # selection should be made. This string describes that first option.
> > fieldSelectionSelect=Select a %S ...
> > 
> > 1) is there any reason why three dots have been used here instead of the
> > unicode ellipsis character?
> > 2) I guess there's no need for a space before the ellipsis. Is there?
> > 3b) Also, an example of %S would
> > be helpful (again, if it's known already, e.g. Select a class...)
> 
> We can fix all these easily, however not in this bug as it has landed. See
> bug 694542.

Well, if 3b is not known, then I doubt the string could become clearer than it already is. Though if you think it can, that's good news for everyone.

> > 3a) I'm not sure "Select a %S" is flexible enough for localization. If we
> > know beforehand what %S might be, and the set of these strings isn't big, I
> > suggest to use separate properties for each %S.
> 
> The trouble is, %S isn't known beforehand.

Thanks for the explanation!

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