653140 - GCLI needs a commonjs require system

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

We would like to enable pilot.jsm to register modules have have them available to Firefox.

Comment 1

[Joe Walker [:jwalker]]

Attachment: Adds require.jsm

Comment 2

[Kevin Dangoor]

the comment that I just put in bug 653142 probably applies here as well. if we want this in quicker, we might want someone else to take the first pass review.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 531632 [details] [diff] [review]
Adds require.jsm

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

Patch looks fine, feedback+! Just please address the nits. ;)

::: toolkit/components/console/hudservice/require.jsm
@@ +13,5 @@
> + *
> + * The Original Code is GCLI.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla

Afaik, this needs to be The Mozilla Foundation.

@@ +14,5 @@
> + * The Original Code is GCLI.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla
> + * Portions created by the Initial Developer are Copyright (C) 2010

2011?

@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2010
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *      Joe Walker ([e-mail])

Contributors name need to be indented starting at "n".

@@ +40,5 @@
> +var debugDependencies = false;
> +
> +/**
> + * Define a module along with a payload
> + * @param moduleName a name for the payload

Please include the parameter type as well. The description should follow on a new line:

@param string aModuleName
       The payload name.

@@ +45,5 @@
> + * @param deps a dependency list (ignored, there for compatibility with
> + * commonjs)
> + * @param payload a function to call with (require, exports, module) params
> + */
> +function define(moduleName, deps, payload) {

Please use aArgumentName for all arguments.

@@ +46,5 @@
> + * commonjs)
> + * @param payload a function to call with (require, exports, module) params
> + */
> +function define(moduleName, deps, payload) {
> +  if (typeof moduleName !== "string") {

Strict type checking is not needed here, IIANM.

@@ +83,5 @@
> + * lookup moduleNames and resolve them by calling the definition function if
> + * needed.
> + */
> +Sandbox.prototype.require = function(moduleName) {
> +  var module = this.modules[moduleName];

Please use let instead of var. See line 107 as well.

@@ +92,5 @@
> +    return module;
> +  }
> +
> +  module = define.modules[moduleName];
> +  if (module == null) {

Why not if (!module) ?

@@ +101,5 @@
> +  if (debugDependencies) {
> +    console.log(this.depth + " Compiling module: " + moduleName);
> +  }
> +
> +  if (typeof module === "function") {

Strict type checking is needed?

Comment 4

[Joe Walker [:jwalker]]

Attachment: Adds require.jsm and console.jsm

console.jsm was part of the pilot bug (bug 653142), but was required by this bug, so I've included it here instead of there.

Hopefully bug 656296 should make console.jsm largely irrelevant shortly.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 531908 [details] [diff] [review]
Adds require.jsm and console.jsm

Rob - I've asked for your feedback too because the patch just got a lot bigger with the addition of console.jsm

Comment 6

[Shawn Wilsher :sdwilsh]

I'm just going to point out that the vast majority if jsm files in the tree start with a capital letter, not a lowercase one.  Same with the objects they export.

Comment 7

[Joe Walker [:jwalker]]

(In reply to comment #6)
> I'm just going to point out that the vast majority if jsm files in the tree
> start with a capital letter, not a lowercase one.  Same with the objects
> they export.

Lower case on exports first:
- Exporting console in Title case is fairly much a no-brainer. It wouldn't work in any other case.
- Exporting require/define in Title case is almost exactly the same - it's there to fit in with CommonJS modules (or at least AMD as defined by RequireJS).

The case of the JSMs comes from the case of the things they export. CommonJS has a lower-case only thing for module names, so I carried it over.

The filenames however, are not connected by any form of compatibility - it's just what I thought made sense given the case of the exports.

Advice appreciated.

Joe.

Comment 8

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #7)
> (In reply to comment #6)
> > I'm just going to point out that the vast majority if jsm files in the tree
> > start with a capital letter, not a lowercase one.  Same with the objects
> > they export.
> 
> Lower case on exports first:
> - Exporting console in Title case is fairly much a no-brainer. It wouldn't
> work in any other case.
> - Exporting require/define in Title case is almost exactly the same - it's
> there to fit in with CommonJS modules (or at least AMD as defined by
> RequireJS).
> 
> The case of the JSMs comes from the case of the things they export. CommonJS
> has a lower-case only thing for module names, so I carried it over.
> 
> The filenames however, are not connected by any form of compatibility - it's
> just what I thought made sense given the case of the exports.
> 
> Advice appreciated.

Not idea. JSMs aren't really stylistically compatible with RequireJS modules. Given that, I'd say the convention of Title-casing JSMs breaks.

I'd recommend sticking with lower-case filenames. It might suggest to someone looking that "hey, these things are different" (or that we're just inconsistent).

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #8)
> Not idea...

meant "ideal". typing...

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 531908 [details] [diff] [review]
Adds require.jsm and console.jsm

+ *
+ * Contributor(s):
+ *   Joe Walker ([e-mail])

use angle-brackets for email address containment.

Also, (Original Author)?

+
+/*
+ * This creates a console object that somewhat replicates Firebug's console
+ * object. It currently writes to dump(), but should write to the web
+ * console's chrome error section (when it has one)
+ */

Writing to nsIConsoleService should be future-compatible with a chrome console.

+ * String utility to ensure that strings are a specified length. Strings
+ * that are too long are truncated to the max length and the last char is
+ * set to "_". Strings that are too short are left padded with spaces.

Why not an ellipsis? i.e., … Or are these strings meant to be used as function names, JSON, or something else?

+function getCtorName(aObj)
+{
+  return Object.prototype.toString.call(aObj).slice(8, -1);

Not super-keen on splicing with hard-coded numbers, but should be ok. I had some concerns about objects in debug builds spitting out addresses, but apparently that's only for DOM nodes and they're (hopefully!) not using Object.prototype.toString().

+  if (typeof aThing == "object") {
+    let reply = '';
+    let type = getCtorName(aThing);
+    if (type == 'Error') {
+      reply += '  ' + aThing.message + "\n";
+      reply += logProperty('stack', aThing.stack);
+    }

Flipping back and forth between '' and "". Should be consistent, unless you're expecting to be wrapping one quote in the other type.

+    else {
+      let keys = Object.getOwnPropertyNames(aThing);
+      if (keys.length > 0) {
+        reply += type + "\n";

Referencing type outside of declaring block (if). type is undefined?

+      else {
+        reply += type + " (enumerated with for-in)\n";

Same here. Move declaration outside of if.

+  return function() {
+    let args = Array.prototype.slice.call(arguments, 0);
+    let data = args.map(function(arg) {

best way to get a copy of arguments, eh? Why do we still not have a copy/deepCopy?

////

+
diff --git a/toolkit/components/console/hudservice/require.jsm b/toolkit/components/console/hudservice/require.jsm

+ * Contributor(s):
+ *   Joe Walker ([e-mail])

same as above file.

+/**
+ * Define a module along with a payload

sdwilsh will probably want punctuation on these comments. (nitty)

+    console.error("dropping module because module name wasn't a string.");
+    console.trace();

In production, these'll probably be considered debugging errors. Might be better off throwing an error here.

+/**
+ * We invoke require in the context of a Sandbox so we can have multiple
+ * sets of modules running separate from each other.
+ */
+function Sandbox()
+{
+  this.modules = {};
+  this.depth = "";
+}

This is a bit of a "Fake" Sandbox in Firefox world. You could use a real Sandbox object from Components.utils. Otherwise, this name might be confusing to some. Not a deal-breaker for me, but ...

I think this is fine, overall, though I wonder how necessary the console.jsm is in light of the fact that most of its usage is really for debugging. It adds a lot of extra code for what is essentially some pretty masking around window.dump().

require() itself is pretty well-tested code and I'd have little issue with including it sans console pieces. If console.jsm's needed elsewhere, I'll consider it non-optional.

Let me know how you'd like to proceed and I'll finish this up.

Comment 11

[Joe Walker [:jwalker]]

Thanks for the useful feedback. Will address the console issues later, but to answer the later question - it is needed to make commonjs code work. We could have a simple console that drops debug info in the bin, but that seems like a waste.

I've renamed Sandbox to Domain.

I've also made it so that the packaged web GCLI uses the same module loader. There are only minor changes here.

Will provide interdiff later today.

Comment 12

[Joe Walker [:jwalker]]

To address the other comments:

- I take the point about nsIConsoleService, however when the WebConsole does do chrome errors, then we'll need to revisit this anyway to take out all the pretty-printing. Do you see nsIConsoleService having a significant benefit?

- "Why not an ellipsis" just because I thought _ was no less clear, and because not being unicode, it was less likely to go wrong somewhere.

- "Referencing type outside of declaring block (if). type is undefined" > I'm not understanding, sorry. 'type' is only used in the block in which it is defined isn't it?

- On use of Array.prototype.slice.call - I have a feeling that arguments is closer to being an array on Moz than everywhere else, but I don't know of a better way to making it be really an array.

- I'm unsure about replacing the console.error with a throw. I have a feeling that it used to be throw, but I changed it to console.error, because when it 'when off' it was a huge pain having everything die. Hmmm.

Comment 13

[Joe Walker [:jwalker]]

Attachment: upload 3

Comment 14

[Joe Walker [:jwalker]]

Attachment: upload 4

This is an implementation of the CommonJS AMD require system to allow Firefox to use GCLI in WebConsole.

Dave: Feel free to ping me in IRC - I'm joe_walker. I'm on UK time.

Comment 15

[Joe Walker [:jwalker]]

Dave - Have I got the right person to review this bug, should I ask someone else?
Thanks

Comment 16

[Joe Walker [:jwalker]]

Hi Dave, Please could you give me an ETA for this review? Many thanks.

Comment 17

[Dave Townsend [:mossop]]

Probably helps if I'm CCed here if you're going to be pinging me.

Couple of quick questions, what is GCLI and why does it need a commonjs require system? What is pilot.jsm?

Comment 18

[Joe Walker [:jwalker]]

Pilot is dead - you can ignore all references to it.

The WebConsole only accepts JavaScript right now. It has a number of commands ('help', 'clear' and so on) but they are a pain to use because you have to type " and () too often.

We think that we can provide fast and powerful user interface by having a toggle switch to flip between JS mode and command line mode. Eg:

> breakpoint line 20
> run line 50
> inspect #someid

And so on.

GCLI is a command line interpreter which works in a web browser, and was written for Bespin/Ace. We think it is a good fit for this task.

It needs a commonjs require system because it's an external project and uses commonjs AMD format modules.

Comment 19

[Dave Townsend [:mossop]]

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

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

How does all of this stuff compare to the CommonJS modules that the Add-on SDK uses? Are they interoperable to the point that the Add-on SDK could just start using this, or vice versa?

Will look at thus again after getting some more info. On the whole I'm kind of dubious about making this stuff a general module.

::: toolkit/components/console/hudservice/console.jsm
@@ +251,5 @@
> +    let args = Array.prototype.slice.call(arguments, 0);
> +    let data = args.map(function(arg) {
> +      return stringify(arg);
> +    });
> +    dump(aLevel + ": " + data.join(", ") + "\n");

We shouldn't be spamming anything to stdout unless a debugging pref is set.

@@ +271,5 @@
> +  return function() {
> +    dump(aLevel + "\n");
> +    let args = Array.prototype.slice.call(arguments, 0);
> +    args.forEach(function(arg) {
> +      dump(log(arg));

Ditto.

@@ +287,5 @@
> +  warn: createDumper("warn"),
> +  error: createMultiLineDumper("error"),
> +  trace: function Console_trace() {
> +    try {
> +      null.trace;

Please don't do this. Use things like Components.stack.

@@ +302,5 @@
> +  group: createDumper("group"),
> +  groupEnd: createDumper("groupEnd")
> +};
> +
> +let EXPORTED_SYMBOLS = [ "console" ];

I don't think I want this to appear in the app's modules. Can we just create a thin-wrapper around existing logging tools we have like log4moz?

::: toolkit/components/console/hudservice/require.jsm
@@ +41,5 @@
> +
> +/**
> + * Define a module along with a payload.
> + *
> + * @param {string} aModuleName

2 spaces after @param, here and elsewhere

@@ +54,5 @@
> +  if (typeof aModuleName != "string") {
> +    console.error("dropping module because module name wasn't a string.");
> +    console.trace();
> +    return;
> +  }

Shouldn't this throw if it fails?

@@ +65,5 @@
> +    console.log("define: " + aModuleName + " -> " + aPayload.toString()
> +        .slice(0, 40).replace(/\n/, '\\n').replace(/\r/, '\\r') + "...");
> +  }
> +
> +  define.modules[aModuleName] = aPayload;

What if a module is already defined?

@@ +72,5 @@
> +/**
> + * The global store of un-instantiated modules
> + */
> +define.modules = {};
> +

You aren't setting define.amd as per the CommonJS spec?

@@ +76,5 @@
> +
> +
> +/**
> + * We invoke require in the context of a Domain so we can have multiple
> + * sets of modules running separate from each other.

This needs expanding on, I don't really understand what it's doing or why it would be useful

@@ +99,5 @@
> + *        A name, or names for the payload
> + * @param {function|undefined} aCallback
> + *        Function to call when the deps are resolved
> + * @return {undefined|object}
> + *        The module required or undefined for array/callback method

Indent this one extra space, here and elsewhere

@@ +129,5 @@
> + */
> +Domain.prototype.lookup = function(aModuleName)
> +{
> +  let module = this.modules[aModuleName];
> +  if (module) {

Test if aModuleName in module instead

@@ +137,5 @@
> +    return module;
> +  }
> +
> +  module = define.modules[aModuleName];
> +  if (!module) {

Test if aModuleName in define.modules instead

@@ +147,5 @@
> +    console.log(this.depth + " Compiling module: " + aModuleName);
> +  }
> +
> +  if (typeof module == "function") {
> +    this.depth += ".";

This seems kind of ugly just for logging.

@@ +161,5 @@
> +  return module;
> +};
> +
> +/**
> + * Expose the Domain constructor and a global sandbox (on the define function

s/sandbox/domain/

@@ +173,5 @@
> + * Expose a default require function which is the require of the global
> + * sandbox to make it easy to use.
> + */
> +let require = define.globalDomain.require.bind(define.globalDomain);
> +let EXPORTED_SYMBOLS = [ "define", "require" ];

Put EXPORTED_SYMBOLS at the top of the module please.

Comment 20

[Joe Walker [:jwalker]]

(In reply to comment #19)
> Comment on attachment 535598 [details] [diff] [review] [review]
> upload 4
> 
> Review of attachment 535598 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> How does all of this stuff compare to the CommonJS modules that the Add-on
> SDK uses? Are they interoperable to the point that the Add-on SDK could just
> start using this, or vice versa?

It's possible that there are elements that we could technically share there are 2 issues:
- At a top level they're not the same - the add-on sdk uses synchronous modules where ours are asynchronous. (the difference being that we have a module header/footer to the module loader can decide when to build the module) Synchronous modules avoid the use of the header/footer but can't be used in a browser.


> Will look at thus again after getting some more info. On the whole I'm kind
> of dubious about making this stuff a general module.
> 
> ::: toolkit/components/console/hudservice/console.jsm
> @@ +251,5 @@
> > +    let args = Array.prototype.slice.call(arguments, 0);
> > +    let data = args.map(function(arg) {
> > +      return stringify(arg);
> > +    });
> > +    dump(aLevel + ": " + data.join(", ") + "\n");
> 
> We shouldn't be spamming anything to stdout unless a debugging pref is set.

This isn't quite spamming stdout because console is analogous to dump for GCLI - something to be used for debugging. GCLI shouldn't be spamming the console.


> > null.throwNPE()
>
> Please don't do this. Use things like Components.stack.

Will take a look. Thanks for the pointer.


> @@ +302,5 @@
> > +  group: createDumper("group"),
> > +  groupEnd: createDumper("groupEnd")
> > +};
> > +
> > +let EXPORTED_SYMBOLS = [ "console" ];
> 
> I don't think I want this to appear in the app's modules. Can we just create
> a thin-wrapper around existing logging tools we have like log4moz?

The goal of console is to be a temporary trivial wrapper that isn't used in live, hence I didn't use log4moz:
- temporary: the 'correct' solution is to have the output go to the web-console! However we can't currently do that because the web-console doesn't handle chrome errors. There is a separate bug for that.
- trivial: console doesn't require configuration, and already dump is behind a pref
- not in live: GCLI and Ace see console and dump in the same light, as quick debugging aids


> @@ +54,5 @@
> > +  if (typeof aModuleName != "string") {
> > +    console.error("dropping module because module name wasn't a string.");
> > +    console.trace();
> > +    return;
> > +  }
> 
> Shouldn't this throw if it fails?

Or maybe re-throw.
I've been both ways on this. I have a feeling that I found this version easier to debug. It shouldn't be important because the build system should prevent this from happening. I would need to investigate which what actually happens in practice, and to date describing the failure mode of a fairly trivial "can't happen" scenario hasn't been top priority.


> @@ +65,5 @@
> > +    console.log("define: " + aModuleName + " -> " + aPayload.toString()
> > +        .slice(0, 40).replace(/\n/, '\\n').replace(/\r/, '\\r') + "...");
> > +  }
> > +
> > +  define.modules[aModuleName] = aPayload;
> 
> What if a module is already defined?

There's 2 answers to that: technically that "can't" happen because the build system wouldn't let it through, and neither would RequireJS, and neither would the filesystem. The question you're asking is functionally equivalent to asking what if 2 files were stored under the same name.

The other answer is that a number of the checks in this file are at a similar level of paranoia (perhaps not to the 2 files with the same name level though) so I'll add in a check.


> @@ +72,5 @@
> > +/**
> > + * The global store of un-instantiated modules
> > + */
> > +define.modules = {};
> > +
> 
> You aren't setting define.amd as per the CommonJS spec?

The goal isn't to comply with the CommonJS spec. It's to load GCLI/Ace. I've not seen client code use define.amd, but it looks like it would be trivial to add if needed.


> @@ +76,5 @@
> > +
> > +
> > +/**
> > + * We invoke require in the context of a Domain so we can have multiple
> > + * sets of modules running separate from each other.
> 
> This needs expanding on, I don't really understand what it's doing or why it
> would be useful

JSMs are singletons. Domains allows us to optionally load a commonjs module twice with separate data each time. Perhaps you want 2 command lines with a different set of commands in each, for example.
Have documented better.


> @@ +129,5 @@
> > + */
> > +Domain.prototype.lookup = function(aModuleName)
> > +{
> > +  let module = this.modules[aModuleName];
> > +  if (module) {
> 
> Test if aModuleName in module instead

OK for both cases.


> @@ +147,5 @@
> > +    console.log(this.depth + " Compiling module: " + aModuleName);
> > +  }
> > +
> > +  if (typeof module == "function") {
> > +    this.depth += ".";
> 
> This seems kind of ugly just for logging.

It is ugly. If there is a simpler solution, then I'm all ears. Diagnosing dependency problems in CommonJS can be a pain, and this makes debugging things easy. I have put this behind "if (debugDependencies)" Which makes it less of a wrench to the reader.


> @@ +161,5 @@
> > +  return module;
> > +};
> > +
> > +/**
> > + * Expose the Domain constructor and a global sandbox (on the define function
> 
> s/sandbox/domain/

Fixed.


> @@ +173,5 @@
> > + * Expose a default require function which is the require of the global
> > + * sandbox to make it easy to use.
> > + */
> > +let require = define.globalDomain.require.bind(define.globalDomain);
> > +let EXPORTED_SYMBOLS = [ "define", "require" ];
> 
> Put EXPORTED_SYMBOLS at the top of the module please.

Done.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> > Will look at thus again after getting some more info. On the whole I'm kind
> > of dubious about making this stuff a general module.
> > 
> > ::: toolkit/components/console/hudservice/console.jsm
> > @@ +251,5 @@
> > > +    let args = Array.prototype.slice.call(arguments, 0);
> > > +    let data = args.map(function(arg) {
> > > +      return stringify(arg);
> > > +    });
> > > +    dump(aLevel + ": " + data.join(", ") + "\n");
> > 
> > We shouldn't be spamming anything to stdout unless a debugging pref is set.
> 
> This isn't quite spamming stdout because console is analogous to dump for
> GCLI - something to be used for debugging. GCLI shouldn't be spamming the
> console.

Except since you're making this a module shared by the whole app others may start to use it. 

> > @@ +302,5 @@
> > > +  group: createDumper("group"),
> > > +  groupEnd: createDumper("groupEnd")
> > > +};
> > > +
> > > +let EXPORTED_SYMBOLS = [ "console" ];
> > 
> > I don't think I want this to appear in the app's modules. Can we just create
> > a thin-wrapper around existing logging tools we have like log4moz?
> 
> The goal of console is to be a temporary trivial wrapper that isn't used in
> live, hence I didn't use log4moz:
> - temporary: the 'correct' solution is to have the output go to the
> web-console! However we can't currently do that because the web-console
> doesn't handle chrome errors. There is a separate bug for that.

We have an error console that does handle chrome errors pretty well.

> - trivial: console doesn't require configuration, and already dump is behind
> a pref

It is? What pref?

> - not in live: GCLI and Ace see console and dump in the same light, as quick
> debugging aids

If it's not going to be used normally then just a thin wrapper around the error console should suffice I think.

> > > +  if (typeof aModuleName != "string") {
> > > +    console.error("dropping module because module name wasn't a string.");
> > > +    console.trace();
> > > +    return;
> > > +  }
> > 
> > Shouldn't this throw if it fails?
> 
> Or maybe re-throw.
> I've been both ways on this. I have a feeling that I found this version
> easier to debug. It shouldn't be important because the build system should
> prevent this from happening. I would need to investigate which what actually
> happens in practice, and to date describing the failure mode of a fairly
> trivial "can't happen" scenario hasn't been top priority.

What build system are you talking about here?

> > @@ +72,5 @@
> > > +/**
> > > + * The global store of un-instantiated modules
> > > + */
> > > +define.modules = {};
> > > +
> > 
> > You aren't setting define.amd as per the CommonJS spec?
> 
> The goal isn't to comply with the CommonJS spec. It's to load GCLI/Ace. I've
> not seen client code use define.amd, but it looks like it would be trivial
> to add if needed.

If you're making require.jsm generally available then we should follow the spec.

> > @@ +76,5 @@
> > > +
> > > +
> > > +/**
> > > + * We invoke require in the context of a Domain so we can have multiple
> > > + * sets of modules running separate from each other.
> > 
> > This needs expanding on, I don't really understand what it's doing or why it
> > would be useful
> 
> JSMs are singletons. Domains allows us to optionally load a commonjs module
> twice with separate data each time. Perhaps you want 2 command lines with a
> different set of commands in each, for example.
> Have documented better.

So commonjs modules aren't singletons?

Comment 22

[Joe Walker [:jwalker]]

(In reply to comment #21)
> (In reply to comment #20)
> > > Will look at thus again after getting some more info. On the whole I'm kind
> > > of dubious about making this stuff a general module.
> > > 
> > > ::: toolkit/components/console/hudservice/console.jsm
> > > @@ +251,5 @@
> > > > +    let args = Array.prototype.slice.call(arguments, 0);
> > > > +    let data = args.map(function(arg) {
> > > > +      return stringify(arg);
> > > > +    });
> > > > +    dump(aLevel + ": " + data.join(", ") + "\n");
> > > 
> > > We shouldn't be spamming anything to stdout unless a debugging pref is set.
> > 
> > This isn't quite spamming stdout because console is analogous to dump for
> > GCLI - something to be used for debugging. GCLI shouldn't be spamming the
> > console.
> 
> Except since you're making this a module shared by the whole app others may
> start to use it. 

And anyone else should follow the same rules.
This generally works in the web world without the strict rules, so I don't see it being a problem here.


> > > @@ +302,5 @@
> > > > +  group: createDumper("group"),
> > > > +  groupEnd: createDumper("groupEnd")
> > > > +};
> > > > +
> > > > +let EXPORTED_SYMBOLS = [ "console" ];
> > > 
> > > I don't think I want this to appear in the app's modules. Can we just create
> > > a thin-wrapper around existing logging tools we have like log4moz?
> > 
> > The goal of console is to be a temporary trivial wrapper that isn't used in
> > live, hence I didn't use log4moz:
> > - temporary: the 'correct' solution is to have the output go to the
> > web-console! However we can't currently do that because the web-console
> > doesn't handle chrome errors. There is a separate bug for that.
> 
> We have an error console that does handle chrome errors pretty well.

But this isn't just for errors, it's for debug logging.
And the error console doesn't work if you've borked your firefox, for which you'd might need your logging.


> > - trivial: console doesn't require configuration, and already dump is behind
> > a pref
> 
> It is? What pref?

browser.dom.window.dump.enabled


> > > > +  if (typeof aModuleName != "string") {
> > > > +    console.error("dropping module because module name wasn't a string.");
> > > > +    console.trace();
> > > > +    return;
> > > > +  }
> > > 
> > > Shouldn't this throw if it fails?
> > 
> > Or maybe re-throw.
> > I've been both ways on this. I have a feeling that I found this version
> > easier to debug. It shouldn't be important because the build system should
> > prevent this from happening. I would need to investigate which what actually
> > happens in practice, and to date describing the failure mode of a fairly
> > trivial "can't happen" scenario hasn't been top priority.
> 
> What build system are you talking about here?

Dryice is a tool which packages the GCLI modules, checks dependencies, and optionally compresses the JS.


> > > @@ +72,5 @@
> > > > +/**
> > > > + * The global store of un-instantiated modules
> > > > + */
> > > > +define.modules = {};
> > > > +
> > > 
> > > You aren't setting define.amd as per the CommonJS spec?
> > 
> > The goal isn't to comply with the CommonJS spec. It's to load GCLI/Ace. I've
> > not seen client code use define.amd, but it looks like it would be trivial
> > to add if needed.
> 
> If you're making require.jsm generally available then we should follow the
> spec.

I think the CommonJS spec isn't quite the totem of wisdom that it might appear to be. I'll take a look, to see how easy it is and perhaps talk to James about Require's perspective. I'm reluctant to take time off making devtools better for a spec requirement that no-one will use, and certainly isn't being used now. But if it's easy, I'll do it.


> > > @@ +76,5 @@
> > > > +
> > > > +
> > > > +/**
> > > > + * We invoke require in the context of a Domain so we can have multiple
> > > > + * sets of modules running separate from each other.
> > > 
> > > This needs expanding on, I don't really understand what it's doing or why it
> > > would be useful
> > 
> > JSMs are singletons. Domains allows us to optionally load a commonjs module
> > twice with separate data each time. Perhaps you want 2 command lines with a
> > different set of commands in each, for example.
> > Have documented better.
> 
> So commonjs modules aren't singletons?

Don't have to be, no.

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > > Will look at thus again after getting some more info. On the whole I'm kind
> > > > of dubious about making this stuff a general module.
> > > > 
> > > > ::: toolkit/components/console/hudservice/console.jsm
> > > > @@ +251,5 @@
> > > > > +    let args = Array.prototype.slice.call(arguments, 0);
> > > > > +    let data = args.map(function(arg) {
> > > > > +      return stringify(arg);
> > > > > +    });
> > > > > +    dump(aLevel + ": " + data.join(", ") + "\n");
> > > > 
> > > > We shouldn't be spamming anything to stdout unless a debugging pref is set.
> > > 
> > > This isn't quite spamming stdout because console is analogous to dump for
> > > GCLI - something to be used for debugging. GCLI shouldn't be spamming the
> > > console.
> > 
> > Except since you're making this a module shared by the whole app others may
> > start to use it. 
> 
> And anyone else should follow the same rules.
> This generally works in the web world without the strict rules, so I don't
> see it being a problem here.

Bottom line is there doesn't seem to be a reason to make this a shared module so please don't. I'd much rather see the console implementation as a wrapper around log4moz (by my reckoning it would be a few lines of code compared to anything else you'd write) but if you want a cheap stub for now and have a bug on file to replace it at the earliest opportunity then that would be ok too.

> > > - trivial: console doesn't require configuration, and already dump is behind
> > > a pref
> > 
> > It is? What pref?
> 
> browser.dom.window.dump.enabled

That only controls the dump function for DOM windows, modules and components print to stdout irrespective of it.

Comment 24

[Dave Townsend [:mossop]]

Is the only reason we need this for the patch in bug 656668? It looks an awful lot like we could just write a very simple define function into the top of that file to take care of this far more simply.

Comment 25

[Joe Walker [:jwalker]]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #20)
> > > > > Will look at thus again after getting some more info. On the whole I'm kind
> > > > > of dubious about making this stuff a general module.
> > > > > 
> > > > > ::: toolkit/components/console/hudservice/console.jsm
> > > > > @@ +251,5 @@
> > > > > > +    let args = Array.prototype.slice.call(arguments, 0);
> > > > > > +    let data = args.map(function(arg) {
> > > > > > +      return stringify(arg);
> > > > > > +    });
> > > > > > +    dump(aLevel + ": " + data.join(", ") + "\n");
> > > > > 
> > > > > We shouldn't be spamming anything to stdout unless a debugging pref is set.
> > > > 
> > > > This isn't quite spamming stdout because console is analogous to dump for
> > > > GCLI - something to be used for debugging. GCLI shouldn't be spamming the
> > > > console.
> > > 
> > > Except since you're making this a module shared by the whole app others may
> > > start to use it. 
> > 
> > And anyone else should follow the same rules.
> > This generally works in the web world without the strict rules, so I don't
> > see it being a problem here.
> 
> Bottom line is there doesn't seem to be a reason to make this a shared
> module so please don't. I'd much rather see the console implementation as a
> wrapper around log4moz (by my reckoning it would be a few lines of code
> compared to anything else you'd write) but if you want a cheap stub for now
> and have a bug on file to replace it at the earliest opportunity then that
> would be ok too.

My gripe with log4moz is that it, like log4j, it seems to conflate 2 use-cases:
- Both APIs are good for after-the-fact understanding of library code
- But not for in-development debugging of unfinished code

The after-the-fact case requires configuration because you need to declare which library you want information from, and to what level. The output from this case is likely to be simple string based output, because the library is telling you about it's understanding and knows the best way to stringify things.

The same configuration is just annoying for the in-development case - anything that stops debugging line 'console.error("should not get here");' from being output just makes matters worse not better. Also, this case needs some degree of introspection. It's my opinion that there is better introspection in this console implementation than in Log4Moz.enumerateProperties


> > > > - trivial: console doesn't require configuration, and already dump is behind
> > > > a pref
> > > 
> > > It is? What pref?
> > 
> > browser.dom.window.dump.enabled
> 
> That only controls the dump function for DOM windows, modules and components
> print to stdout irrespective of it.

Ah - OK, thanks.
So I guess a pref makes sense. It could also toggle output between dump and the error console too. I'll add that in.

Thanks.

Comment 26

[Joe Walker [:jwalker]]

(In reply to comment #24)
> Is the only reason we need this for the patch in bug 656668? It looks an
> awful lot like we could just write a very simple define function into the
> top of that file to take care of this far more simply.

Yes/maybe Ace needs require too.

Comment 27

[Joe Walker [:jwalker]]

Thinking again - I'd rather get this in than spend lots of time debating this. I firmly believe that Log4Moz/dump/ErrorConsole don't provide a good println debugging environment, but it's not worth debating above getting GCLI working.

I'll find some way to add this all to gcli.jsm

Joe.

Comment 28

[Rob Campbell [:rc] (:robcee)]

that may be best. Having everything self-contained in the GCLI patch is probably going to make the review "easier". By easier, I mean less prone to needing rewrites of back-end support code.

Comment 29

[Dave Townsend [:mossop]]

(In reply to comment #28)
> that may be best. Having everything self-contained in the GCLI patch is
> probably going to make the review "easier". By easier, I mean less prone to
> needing rewrites of back-end support code.

It also means we have to think far less about the APIs we're creating which is really my biggest concern here. I don't like it but every API we add increases the burden of maintenance down the road. If we aren't adding any APIs then I can basically take my superreviewer hat off and just focus on the actual code.

Comment 30

[Joe Walker [:jwalker]]

I'd still like to get the rest of this signed off, so I'll re-submit a version which is called gcli.jsm, which contains just enough of GCLI to be meaningful (i.e. so we know it's loaded) and continue to add the rest of GCLI in another bug.

Thanks,

Comment 31

[Joe Walker [:jwalker]]

Attachment: upload 5

merged into single jsm called gcli.jsm

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 538013 [details] [diff] [review]
upload 5

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

Couple of small issues , particularly I'd like an answer to the last before r+ing

::: toolkit/components/console/hudservice/gcli.jsm
@@ +62,5 @@
> + * that are too long are truncated to the max length and the last char is
> + * set to "_". Strings that are too short are left padded with spaces.
> + *
> + * @param {string} aStr
> + *        The string to format to the correct length

Elsewhere in toolkit we indent by two spaces after @param (so it lines up with the indent after @return). If that isn't the case for devtools then ignore me.

@@ +69,5 @@
> + * @param {number} aMinLen (optional)
> + *        The minimum allowed length of the returned string. If undefined,
> + *        then aMaxLen will be used
> + * @return {string}
> + *        The original string formatted to fit the specified lengths

Likewise this line would get indented by an additional space to match the line above.

@@ +71,5 @@
> + *        then aMaxLen will be used
> + * @return {string}
> + *        The original string formatted to fit the specified lengths
> + */
> +function fmt(aStr, aMaxLen, aMinLen, options)

options should be aOptions and isn't in the comments

@@ +140,5 @@
> +  return fmt(str, 60, 0);
> +}
> +
> +/**
> + * A multi line stringification of an object, designed for use by humans

Mention that it includes all of the properties on the object.

@@ +228,5 @@
> +      return;
> +    }
> +    let at = line.lastIndexOf("@");
> +    let posn = line.substring(at + 1);
> +    posn = posn.replace(/resource:\/\/\/modules\//, "");

What about resource://gre/modules/...?

@@ +229,5 @@
> +    }
> +    let at = line.lastIndexOf("@");
> +    let posn = line.substring(at + 1);
> +    posn = posn.replace(/resource:\/\/\/modules\//, "");
> +    posn = posn.replace(/chrome:\/\/browser\/content\//, "");

What about chrome://global/... etc.?

@@ +531,5 @@
> +/*
> + * require GCLi so it can be exported as declared at the start
> + */
> +
> +let gcli = require("gcli/index");

Feels a little weird to define it just to require it immediately. Is something else going to be requiring it later?

Comment 33

[Joe Walker [:jwalker]]

Re: code formatting - it's hard to know, generally devtools follows the rest of Mozilla, however the code we're talking about just took a distinct step away from Mozilla towards GCLI which has a different, more browsery code style. I think that now the code lives in the GCLI repo, it should follow that style more.

> @@ +531,5 @@
> > +/*
> > + * require GCLi so it can be exported as declared at the start
> > + */
> > +
> > +let gcli = require("gcli/index");
> 
> Feels a little weird to define it just to require it immediately. Is
> something else going to be requiring it later?

Ace might, but that's not the point.

The section above is a fake, the real code will be created from the GCLI project, which uses define.
Our options are:
- have a very complex build system that not just manages dependencies but also re-writes the code to not use commonjs modules
- use commonjs modules

Now require doesn't have to be called require (we can easily change it's name), but it's all a bit academic at that point.

This is the next patch that adds the real GCLI:

https://hg.mozilla.org/users/jwalker_mozilla.com/gcli-patches/file/722ee2199c1a/bug656668-gcli.patch

The thing to note is that entire section is replaced by code from the GCLI project.

Comment 34

[Joe Walker [:jwalker]]

Attachment: upload 6

This fixes the issues you raised - I'm not sure where you stand on the code style issue. Now all the code comes from the GCLI repo, I've managed to unify the 2 versions of mini_require by adopting the GCLI style guidelines. Personally, I think this is a win.

Comment 35

[Joe Walker [:jwalker]]

Attachment: mylyn/context/zip

Comment 36

[Joe Walker [:jwalker]]

Excellent - thanks.

Comment 37

[Joe Walker [:jwalker]]

Are we sure that this doesn't need super-review?
We do:

EXPORTED_SYMBOLS = [ "gcli" ];

I think it possibly doesn't because this is a fairly experimental feature, and one that is useless without the UI, to be added later - i.e. the cost of getting it wrong at this point is low.

There is an error in the definition of gcli in the patch as it stands (aside from the fact that it clearly doesn't do anything!) I'll update the patch either for you to quickly check, or for super-review.

Comment 38

[Joe Walker [:jwalker]]

Attachment: upload 7

Comment 39

[Joe Walker [:jwalker]]

Comment on attachment 540572 [details] [diff] [review]
upload 7


Hi Dave - Please could you confirm that you don't think this needs super-review?

I've attached a new patch with 2 changes:

- The interface GCLI exposes has changed (actually shrunk) as a result of our internal reviews, so there are less methods than before - I can't see this being a problem

- There was a test that was unused - also removed, and I've been having bizarre leak reports from try that I'm fairly sure are not my fault, however I added some paranoid tidying up in the tests

Sorry to bother you again.
Joe.

Comment 40

[Joe Walker [:jwalker]]

Thanks Dave.

Comment 41

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 540572 [details] [diff] [review]
upload 7

http://hg.mozilla.org/projects/devtools/rev/5851c1b248b9

Comment 42

[Joe Walker [:jwalker]]

Thanks robcee!

Comment 43

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 540572 [details] [diff] [review]
upload 7

backed out in: http://hg.mozilla.org/projects/devtools/rev/676ba3f883f5

:(

see,

http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1308783773.1308789387.10632.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1308779573.1308783599.19178.gz&fulltext=1

etc, from:

http://tbpl.mozilla.org/?tree=Devtools&rev=fb02b664d02d

Comment 44

[Joe Walker [:jwalker]]

Attachment: upload 8

yeay memleak fixed

Comment 45

[Rob Campbell [:rc] (:robcee)]

hi!

joe, could you rebase this so it lives on top of the new webconsole directory?

It's been moved to browser/devtools/webconsole.

Thanks!

Comment 46

[Joe Walker [:jwalker]]

Attachment: [in-fx-team] upload 9

rebase, and change directory

Comment 47

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 550635 [details] [diff] [review]
[in-fx-team] upload 9

http://hg.mozilla.org/integration/fx-team/rev/3f5229b206d5

http://hg.mozilla.org/integration/fx-team/rev/71881e5b06ca

Comment 48

[Joe Walker [:jwalker]]

Thanks Rob.

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3f5229b206d5
http://hg.mozilla.org/mozilla-central/rev/71881e5b06ca