Last Comment Bug 653142 - Create a pilot.jsm javascript module for use with GCLI/Ace
: Create a pilot.jsm javascript module for use with GCLI/Ace
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-27 09:01 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-06-30 05:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds console.jsm and pilot.jsm (18.31 KB, patch)
2011-05-11 08:16 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: feedback-
Details | Diff | Review
Adds pilot.jsm (13.00 KB, text/plain)
2011-05-12 08:46 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
rcampbell: feedback+
Details

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-04-27 09:01:54 PDT
Will probably use the require system in bug 653140
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-11 08:16:11 PDT
Created attachment 531636 [details] [diff] [review]
Adds console.jsm and pilot.jsm

Don't worry if you don't have time to get to this today.
Also don't worry about the console.jsm part - this will likely disappear with bug 656296
Comment 2 Kevin Dangoor 2011-05-11 10:45:25 PDT
This is critical path for getting the command line landed for Firefox 6, and Mihai is going to be out for the next 2 of the few days remaining for that release. Perhaps robcee or ddahl can take a look?
Comment 3 Mihai Sucan [:msucan] 2011-05-11 12:46:24 PDT
Comment on attachment 531636 [details] [diff] [review]
Adds console.jsm and pilot.jsm

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

The patch looks fine.

General comments which also apply to bug 653140:

- use arguments with the "a" prefix.
- add jsdoc comments to all of the methods.
- use let instead of var.
- give names to functions.
- add pilot/useragent, so that projects which use pilot/useragent (like Ace) can see the code runs on Firefox/Gecko. Obviously, you'll need to fake/simplify the pilot/useragent code.
- don't check things we don't need to, for example in preventDefault, or in createElement. We always have preventDefault() and createElementNS(). Also look in other places to simplify the code (these are just examples ;) ).
- don't use globals that are not available in the JSM, like document or window. Perhaps provide an API for users to set our document/window/etc globals.

Given the above comments, I can't yet give the patch feedback+. I am not sure which of these should be strict requirements or not. Please ask a reviewer.

::: toolkit/components/console/hudservice/console.jsm
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Please see bug 653140 comment 3 about the license blocks.

::: toolkit/components/console/hudservice/pilot.jsm
@@ +67,5 @@
> +  var dom = exports;
> +
> +  dom.createElement = function(tag, ns, doc) {
> +    if (!doc) {
> +      doc = document;

There is no document global in the jsm land. Same goes for line 118 and other places, if any, where you use document/window.

@@ +220,5 @@
> +    this._eventRegistry = this._eventRegistry || {};
> +
> +    var listeners = this._eventRegistry[eventName];
> +    if (!listeners) {
> +      var listeners = this._eventRegistry[eventName] = [];

No need for "var" here.

@@ +288,5 @@
> +    if (e.preventDefault)
> +      e.preventDefault();
> +    else
> +      e.returnValue = false;
> +  };

Sometimes you use brackets for oneliners, sometimes not. Please use brackets consistently.

@@ +305,5 @@
> +      else {
> +        if (e.axis && e.axis == e.HORIZONTAL_AXIS) {
> +          e.wheelX = (e.detail || 0) * 5;
> +          e.wheelY = 0;
> +        } else {

Please use }\nelse { consistently throughout the code.

@@ +385,5 @@
> +    for (var i = 0; i <= array.length; i++) {
> +      if (value === array[i]) {
> +        array.splice(i, 1);
> +      }
> +    }

You can use Array.filter() here.

@@ +439,5 @@
> +   * From dojo.isFunction()
> +   */
> +  typecheck.isFunction = function(it) {
> +    return it && Object.prototype.toString.call(it) === "[object Function]";
> +  };

Dojo credits? License and contributors?
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-12 08:46:33 PDT
Created attachment 531946 [details]
Adds pilot.jsm
Comment 5 Rob Campbell [:rc] (:robcee) 2011-05-16 05:55:16 PDT
Comment on attachment 531946 [details]
Adds pilot.jsm

Warning! Nits ahead!

from pilot.jsm

+ *
+ * Contributor(s):
+ *   Joe Walker (jwalker@mozilla.com) (original author)

We typically use <>s around email addresses. 

+/**
+ * This is a shim to allow ACE/GCLI to use Pilot without needing all the
+ * extra cross-platform stuff. So it's basically PilotLite.
+ * For documentation, see the original Pilot project.

Include a link here to the original project.

There seems to be three different mechanisms for exporting objects / namespaces in here: 
1) +  let dom = exports;
2) +  let EventEmitter = {}; ... +  exports.EventEmitter = EventEmitter;
3) +  exports.addListener = function event_addListener(aElement, aType, aCallback)

I guess the only one that's really inconsistent is the EventEmitter case where you're attaching a named property to your exports object. Maybe this makes more sense when it's in use, but I'm curious why that submodule's different.

+/**
+ * Define 'pilot/dom'
+ */

Some additional comments on what these inner modules are for might be useful.


in pilot/event:

+
+  exportsevent.preventDefault = function event_preventDefault(aEvent)
+  {
+    aEvent.preventDefault();
+  };

Looks like a replace fail on event / exports?

+  exports.inherits = (function()
+  {
+    let tempCtor = function() {};
+    return function(aCtor, aSuperCtor) {
+      tempCtor.prototype = aSuperCtor.prototype;
+      aCtor.super_ = aSuperCtor.prototype;
+      aCtor.prototype = new tempCtor();
+      aCtor.prototype.constructor = aCtor;
+    };
+  }());

that could use some explanation.

+  exports.implement = function oop_implement(aProto, aMixin)
+  {
+    exports.mixin(aProto, aMixin);
+  };

so, every subclass overrides all of the inherited properties of the superclass? Or is this more of a Java interface-like inheritance mechanism?
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-16 09:15:51 PDT
(In reply to comment #5)
> Comment on attachment 531946 [details]
> Adds pilot.jsm
> 
> Warning! Nits ahead!
> 
> from pilot.jsm
> 
> + *
> + * Contributor(s):
> + *   Joe Walker (jwalker@mozilla.com) (original author)
> 
> We typically use <>s around email addresses. 
> 
> +/**
> + * This is a shim to allow ACE/GCLI to use Pilot without needing all the
> + * extra cross-platform stuff. So it's basically PilotLite.
> + * For documentation, see the original Pilot project.
> 
> Include a link here to the original project.

Both done in my tree.

> There seems to be three different mechanisms for exporting objects /
> namespaces in here: 
> 1) +  let dom = exports;
> 2) +  let EventEmitter = {}; ... +  exports.EventEmitter = EventEmitter;
> 3) +  exports.addListener = function event_addListener(aElement, aType,
> aCallback)
> 
> I guess the only one that's really inconsistent is the EventEmitter case
> where you're attaching a named property to your exports object. Maybe this
> makes more sense when it's in use, but I'm curious why that submodule's
> different.

Style 3 is simple - it's compact. It's clear that something is being exported. However internally code has to use exports.foo to refer to other parts of the same module. Externally moduleName.foo is used. When the parts are independent this isn't an issue. For more complex cases it gets annoying so ....

Style 1 is a nice tweak. Slightly less obvious that something is being exported, but if you don't know what module you're working with, then you're hosed already. I generally prefer this style, unless the module is trivial. If someone wanted to yell at me for being inconsistent then this would be the one to kill, but I kind of like it, so for now at least, I would defend it's use.

Style 2 is for when something needs a simple name like a class. Calling 'new moduleName.Foo()' is really annoying and everyone wants 'new Foo()'. It's a pain because it has to be separately imported and exported - double the work, but worth it for important things like classes.


> +/**
> + * Define 'pilot/dom'
> + */
> 
> Some additional comments on what these inner modules are for might be useful.

The place for this is probably in Pilot, along with the other similar docs.


> in pilot/event:
> 
> +
> +  exportsevent.preventDefault = function event_preventDefault(aEvent)
> +  {
> +    aEvent.preventDefault();
> +  };
>
> Looks like a replace fail on event / exports?

Good spot. Thanks.


> +  exports.inherits = (function()
> +  {
> +    let tempCtor = function() {};
> +    return function(aCtor, aSuperCtor) {
> +      tempCtor.prototype = aSuperCtor.prototype;
> +      aCtor.super_ = aSuperCtor.prototype;
> +      aCtor.prototype = new tempCtor();
> +      aCtor.prototype.constructor = aCtor;
> +    };
> +  }());
> 
> that could use some explanation.

And probably deletion.
It's crockford/boodman inheritance, something that can probably be better done using Object.create or similar. I have a todo list item to test and delete this function. If I don't delete, I will comment.


> +  exports.implement = function oop_implement(aProto, aMixin)
> +  {
> +    exports.mixin(aProto, aMixin);
> +  };
> 
> so, every subclass overrides all of the inherited properties of the
> superclass? Or is this more of a Java interface-like inheritance mechanism?

This isn't my choice, but we're kind of stuck with it unless we get into re-factoring Ace. We clearly don't need both implement and mixin. The idea is that it's used like:

  lang.implement(Banana.prototype, Fruit);
Comment 7 Rob Campbell [:rc] (:robcee) 2011-05-17 06:58:50 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 531946 [details]
> > There seems to be three different mechanisms for exporting objects /
> > namespaces in here: 
> > 1) +  let dom = exports;
> > 2) +  let EventEmitter = {}; ... +  exports.EventEmitter = EventEmitter;
> > 3) +  exports.addListener = function event_addListener(aElement, aType,
> > aCallback)
> > 
> > I guess the only one that's really inconsistent is the EventEmitter case
> > where you're attaching a named property to your exports object. Maybe this
> > makes more sense when it's in use, but I'm curious why that submodule's
> > different.
> 
> Style 3 is simple - it's compact. It's clear that something is being
> exported. However internally code has to use exports.foo to refer to other
> parts of the same module. Externally moduleName.foo is used. When the parts
> are independent this isn't an issue. For more complex cases it gets annoying
> so ....
> 
> Style 1 is a nice tweak. Slightly less obvious that something is being
> exported, but if you don't know what module you're working with, then you're
> hosed already. I generally prefer this style, unless the module is trivial.
> If someone wanted to yell at me for being inconsistent then this would be
> the one to kill, but I kind of like it, so for now at least, I would defend
> it's use.
> 
> Style 2 is for when something needs a simple name like a class. Calling 'new
> moduleName.Foo()' is really annoying and everyone wants 'new Foo()'. It's a
> pain because it has to be separately imported and exported - double the
> work, but worth it for important things like classes.

ok, I just wanted to make sure the differences were intentional. I figured they were for the reasons you listed, but it's a bit jarring to see all three of them used in a single JSM (which usually has those same classes of exports but per file).

> > +/**
> > + * Define 'pilot/dom'
> > + */
> > 
> > Some additional comments on what these inner modules are for might be useful.
> 
> The place for this is probably in Pilot, along with the other similar docs.

I'd prefer to see them next to the code if possible. If I have to browse to another location, I've probably lost my train of thought.

> > +  exports.inherits = (function()
> > +  {
> > +    let tempCtor = function() {};
> > +    return function(aCtor, aSuperCtor) {
> > +      tempCtor.prototype = aSuperCtor.prototype;
> > +      aCtor.super_ = aSuperCtor.prototype;
> > +      aCtor.prototype = new tempCtor();
> > +      aCtor.prototype.constructor = aCtor;
> > +    };
> > +  }());
> > 
> > that could use some explanation.
> 
> And probably deletion.
> It's crockford/boodman inheritance, something that can probably be better
> done using Object.create or similar. I have a todo list item to test and
> delete this function. If I don't delete, I will comment.

OK. I recognized it, I just didn't like it. :)

> > +  exports.implement = function oop_implement(aProto, aMixin)
> > +  {
> > +    exports.mixin(aProto, aMixin);
> > +  };
> > 
> > so, every subclass overrides all of the inherited properties of the
> > superclass? Or is this more of a Java interface-like inheritance mechanism?
> 
> This isn't my choice, but we're kind of stuck with it unless we get into
> re-factoring Ace. We clearly don't need both implement and mixin. The idea
> is that it's used like:
> 
>   lang.implement(Banana.prototype, Fruit);

yeah, I understand how this is to be used, it's just sort of kludgey, and not real inheritance.

Banana and Apple both have their own copies of Fruit's properties. If I modify it in Banana, it doesn't change in Apple and so on.

I guess that's fine here as long as consumers understand what they're doing with it.

(marking this bug as assigned since you're already on it.)
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-05-18 09:33:14 PDT
Pilot is now inside GCLI.
https://github.com/mozilla/gcli/blob/master/lib/gcli/util.js
I've included changed from your feedback. Thanks.

lang.inherits() and lang.implement() have been removed

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