Bug 788324 - Re-implement context-menu API #599

Closed
wants to merge 41 commits into
from

Projects

None yet

2 participants

@Mossop
Mozilla member

This is the new version of the context-menu API that I've been working on. It works and passes all tests but I want to think about future improvements before doing a full review, but I think it is about time I at least heard some feedback on the way I've done this. I've maintained API compatibility aside from a couple of pieces:

  • Currently contexts and content scripts are only valid for top-level Item and Menu objects. My code makes them work for Separators as well as anything in the menu hierarchy. This fixes bug 729843.
  • When a menu doesn't contain anything visible I've made it hide the menu.
  • Items are no longer sorted, instead they appear in the order created and all items for one add-on stay together. This fixes bug 784611.
  • Added an exported contentContextMenu which is a cut down Menu and holds all the top-level items. The parentMenu property for those items no returns this rather than null as they would before. This allows re-arranging menus (where before once added to a submenu there would be no way to move an item back to the top-level). It is also a stepping stone to allowing us to target other context menus (bug 795522 and bug 795523 f.e.).
  • content scripts are now first executed when the context menu requiring them is first opened. Previously as far as I can see they are run for every window and frame for every page ever opened which seems pretty costly and is also the source of a lot of issues (since the old code doesn't detect new frames well and waits until the page completes loading before executing).
  • I've changed the classes used for menu items to avoid conflicts with the old module. I used classes instead of ids for the separator and overflow menu to allow it to work with multiple context menus in the same window.

The code is set up to be able to target other context menus in the future and with a couple of line changes you can add things to the tab context menu already, but this needs more work and thought and I want to play with it before getting full review as it might require further changes I haven't thought of yet.

Mossop added some commits Sep 6, 2012
@Mossop Mossop Switch to using Object.defineProperty to set up collection properties…
… as __defineSetter__ doesn't work in some contexts.
81a30d0
@Mossop Mossop Initial clean-room implementation of context-menu that creates and re…
…moves menus as necessary. Tests adjusted to all for fast runs showing errors
2206e49
@Mossop Mossop Implement context support and get all currently interesting tests pas…
…sing.
6eac6c4
@Mossop Mossop Add contentScript support and fix all failing tests. a0e116a
@Mossop Mossop Hide private implementation details a80175a
@Mossop Mossop Some simplifications 5b6be5a
@Mossop Mossop Update docs from API changesd 497df75
@Mossop Mossop Bring back order checking tests and correct for changes a897c70
@Mossop Mossop Mention ordering and overflow in the docs 7c93f67
@Mossop Mossop Merge with master 4b31068
@Mossop Mossop Allow RegExp patterns to be passed to URLContext. 82d8f66
@Mossop Mossop Make menu testing simpler ready for better sub-menu tests. 6b07675
@Mossop Mossop Add tests that menus with no items are hidden c0b3dd8
@Mossop Mossop Add tests for sub-item contexts and clicks 1e84620
@Mossop Mossop Add code comments and fix some clarity issues b4caf07
@Mossop Mossop Make sure to group items from the same module instance. 97979b2
@Mossop Mossop Whitespace fixup 1f648c6
@Mossop Mossop Add tests that new tabs opened with _blank still work correctly with …
…the SelectionContext
cc6005c
@Mossop Mossop Make selection context work correctly when right clicking a form button 0d489ef
@Mossop Mossop Add tests for selections in inner and outer frames 98e78e4
@Mossop Mossop Use existing iframe for selection tests 70b2f7a
@Mossop Mossop Merge branch 'master' into context-menu b6a2f4b
@Mossop Mossop Handle destroying workers better 606bf87
@Mossop Mossop Only launch the app if there are actually tests to be run. 41c7c52
@Mossop Mossop Merge branch 'test-filter' into context-menu 7bed8b8
@Mossop Mossop Merge branch 'master' into context-menu 88064d6
@Mossop Mossop Merge branch 'master' into context-menu 634db7c
@Mossop Mossop Move window related functionality into a window wrapper class. 1cce5ca
@Mossop Mossop Expose root context menu throguh the API. 9d48d91
@Mossop Mossop Allow choosing the menu to add items to. c8fad3c
@Mossop Mossop Add support for multiple context menus. 0eda39d
@Mossop Mossop Fix warning message. 5f7cd85
@Mossop Mossop Rename classes to avoid conflicts with the old context-menu module. f51a81c
@Mossop Mossop Take away the tab context menu for now. b26d3b3
@Mossop Mossop Merge branch 'master' into context-menu afed5bb
@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/docs/context-menu.md
@@ -199,6 +206,11 @@ This example takes advantage of that fact. The listener can be assured that
Your item is shown only when all declarative contexts are current and your
context listener returns true.
+The content script is executed for every page that a context menu is shown for.
+It will be executed the first time it is needed (i.e. when the context menu is
+first shown and all of the declarative contexts for your item are current) and
+then remains active until you destroy your context menu item or the page is
+unloaded.
@Gozala
Gozala Oct 8, 2012

I have not looked into implementation yet, but this makes me feel that context menu will require sync message channel with content script, which intentionally was avoided in previous implementation, presumably for E10S.

If that's really a case we should recognize the fact that e10s compatibility will be broken. I also wonder if sync message API can even be provided for a fennec. @erikvold do you know if that's possible ?

@Gozala
Gozala Oct 8, 2012

BTW original implementation details also we're not quite compatible with E10S as messages had to be send in sync.

@Mossop
Mossop Oct 8, 2012

Yeah even the original context-menu used sync messages so this is unchanged really. We could make it async but that would probably break compatibility with the API as well as make it possible for the context menu to appear before we've heard from all the content scripts so lead to some flickering. I don't know if it's worth trying to solve that problem now or waiting until it is a real problem.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- this[optName] = opts[optName];
- optsToNotSet.forEach(function (opt) validateOpt(opts[opt], optRules[opt]));
- this._isInited = true;
-
- this._id = nextItemID++;
- this._parentMenu = null;
-
- // This makes the private properties accessible to anyone with access to
- // PRIVATE_PROPS_KEY. Barring loader tricks, only this file has has access
- // to it, so only this file has access to the private properties.
- const self = this;
- this.valueOf = function IBT_valueOf(key) {
- return key === PRIVATE_PROPS_KEY ? self : self._public;
- };
+let Context = Class({
+ adjustPopupNode: function adjustPopupNode(popupNode) {
@Gozala
Gozala Oct 8, 2012

I think moving doc comments for adjustPopupNode would make it little easier to follow.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- set _isTopLevel(val) {
- if (val)
- this._workerReg = new WorkerRegistry(this._public);
- else {
- this._workerReg.destroy();
- delete this._workerReg;
- }
- },
+ while (!(popupNode instanceof Ci.nsIDOMDocument)) {
+ if (NON_PAGE_CONTEXT_ELTS.some(function(type) popupNode instanceof type))
+ return false;
@Gozala
Gozala Oct 8, 2012

Adding some comments to explain intent of this code would improve readability.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- get data() {
- return this._data;
- },
+ try {
+ internal(this).patterns = patterns.map(function (p) new MatchPattern(p));
+ }
+ catch (err) {
+ console.error("patterns must be a string, regexp or an array of strings or regexps");
+ throw err;
@Gozala
Gozala Oct 8, 2012

Usually we don't log errors into console we just make that a message of an error thrown.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- return item._public;
+function filterOut(array, item) {
@Gozala
Gozala Oct 8, 2012

More descriptive name would have being helpful.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- },
-
- destroy: function MT_destroy() {
- while (this.items.length)
- this.items[0].destroy();
- this._destroyThisItem();
+// Shared option validation rules for Item and Menu
+let baseItemRules = {
+ parentMenu: {
+ is: ["object", "undefined"],
+ ok: function (v) {
+ if (!v)
+ return true;
+ return (v instanceof ItemContainer) || (v instanceof Menu);
+ },
+ msg: "parentMenu is specified must be a Menu."
@Gozala
Gozala Oct 8, 2012

Is is really necessary in this message string ?

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- if (item.parentMenu)
- item.parentMenu.removeItem(item);
- else if (!(item instanceof Separator) && privates._hasFinishedInit)
- browserManager.removeTopLevelItem(item);
-
- // Now add the item to this menu.
- this._items.push(item);
- privates._parentMenu = this._public;
- browserManager.addItemToMenu(item, this._public);
+ contentScriptFile: {
+ is: ["string", "array", "undefined"],
+ ok: function (v) {
+ if (!v)
+ return true;
+ let arr = Array.isArray(v) ? v : [v];
+ try {
@Gozala
Gozala Oct 8, 2012

I would prefer getScheme util function that returns null on exceptions over try catch here.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
+ is: ["string", "array", "undefined"],
+ ok: function (v) {
+ if (!v)
+ return true;
+ let arr = Array.isArray(v) ? v : [v];
+ try {
+ return arr.every(function (s) {
+ return getTypeOf(s) === "string" &&
+ URL(s).scheme === 'resource';
+ });
+ }
+ catch (err) {}
+ return false;
+ },
+ msg: "The 'contentScriptFile' option must be a local file URL or " +
+ "an array of local file URLs."
},
@Gozala
Gozala Oct 8, 2012

BTW we also do verify contentScript, contentScriptFile, onMessage and other listeners in bunch of other places. It would be great to share one validator code to reuse code & more importantly make sure we're consistent in requirements.

@Gozala
Gozala Oct 8, 2012

I can also see why you would not want to refactor all other such code, but at least refactoring these common validators into it's own module would make reuse in a future a lot easier.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- removeItem: function MT_removeItem(item) {
- let idx = this._items.indexOf(item);
- if (idx < 0)
- return;
- this._items.splice(idx, 1);
- privateItem(item)._parentMenu = null;
- browserManager.removeItemFromMenu(item, this._public);
+let labelledItemRules = mix(baseItemRules, {
+ label: {
+ map: function (v) v.toString(),
@Gozala
Gozala Oct 8, 2012

I think map: String would have being simpler, will do the same too.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- let menu = MenuTrait.create(Menu.prototype);
+let ContextWorker = Worker.compose({
+ // Returns the first string or truthy value returned by a context listener in
+ // the worker's port. If none return a string or truthy value or if there are
+ // no context listeners, returns false. popupNode is the node that was
+ // context-clicked.
+ isAnyContextCurrent: function isAnyContextCurrent(popupNode) {
+ let results = this._contentWorker.emitSync("context", popupNode);
+ if (results.length == 0)
+ return true;
+ for (let i = 0; i < results.length; i++) {
+ let val = results[i];
+ if (typeof val === "string" || val)
@Gozala
Gozala Oct 8, 2012

Our code convention is typeof(val)

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- let menu = MenuTrait.create(Menu.prototype);
+let ContextWorker = Worker.compose({
+ // Returns the first string or truthy value returned by a context listener in
+ // the worker's port. If none return a string or truthy value or if there are
+ // no context listeners, returns false. popupNode is the node that was
+ // context-clicked.
@Gozala
Gozala Oct 8, 2012

I think either API or documentation is over complicated. Namely by intuition I expect isFoo named functions to return booleans.
I also find cofusing that no values results.length === 0 returns true. I think it would be a lot simpler to have getCurrentContexts
that return array of matching contexts or null if there are no matches.

Maybe I'm missing something.

@Mossop
Mossop Oct 12, 2012

This was originally copied verbatim from the original implementation. I've made some changes to make it more readable and renamed the function while keeping the behaviour the same. Basically all that matters is that the first listener that returns true or a string means the context matches, then that string becomes the new label for the menu item.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- // We can't rely on _initBase to set the `items` property, because the menu
- // needs to be registered with and added to the browserManager before any
- // child items are added to it.
- menu._initMenu(options, optRules, ["items"]);
+ // Emits a click event in the worker's port. popupNode is the node that was
+ // context-clicked, and clickedItemData is the data of the item that was
+ // clicked.
+ fireClick: function fireClick(popupNode, clickedItemData) {
+ this._contentWorker.emitSync("click", popupNode, clickedItemData);
@Gozala
Gozala Oct 8, 2012

internals(this).contentWorker.emitSync("click", popupNode, clickedItemData); ??

@Mossop
Mossop Oct 12, 2012

I'm not sure what you're saying here, internals(this).contentWorker is undefined I think.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
-// The exported Separator constructor.
-function Separator() {
- let sep = ItemBaseTrait.create(Separator.prototype);
- sep._initBase({}, {}, []);
+ let result = worker.isAnyContextCurrent(popupNode);
+ if (typeof result === "string")
+ item.label = result;
@Gozala
Gozala Oct 8, 2012

This side effect here is really unexpected.

@Mossop
Mossop Oct 8, 2012

I agree but that is what context-menu does right now so it'd be breaking compatibility to change it.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- menu._public = Cortex(menu);
- browserManager.registerItem(menu._public);
- menu.items = options.items;
- menu._finishActiveItemInit();
+// Tests whether an item should be visible or not based on its contexts and
+// content scripts
+function isItemVisible(item, popupNode) {
@Gozala
Gozala Oct 8, 2012

I find this function to be very complicated and hard to read. I would really prefer something among these lines:

function isMatchingContext(context, node) {
  return context.length ? context.every(function(context) context.isCurrent(node))
                        : PageContext().isCurrent(node);
}

function isWorkerContextless(item, popupNode) {
  let worker = getItemWorkerForWindow(item, popupNode.ownerDocument.defaultView);
  return !worker || !worker.isAnyContextCurrent(popupNode);
}

function isItemVisible(item, popupNode) {
  let worker = null
  return isMatchingContext(popupNode) && !isWorkerContextless(worker)
}

P.S.: Ideas is to enable reading function without necessary diving into impl details, and dive into them only once the overall ideas in clear.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
}
+// Destroys any item's content scripts workers associated with the given window
+function destroyItemWorker(item, window) {
@Gozala
Gozala Oct 8, 2012

I think naming like destroyItemWorkerForWindow or even destroyItemFor would be less confusing.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- return false;
-
- let hasSelection = !win.getSelection().isCollapsed;
- if (!hasSelection) {
- // window.getSelection doesn't return a selection for text selected in a
- // form field (see bug 85686), so before returning false we want to check
- // if the popupNode is a text field.
- let { selectionStart, selectionEnd } = popupNode;
- hasSelection = !isNaN(selectionStart) &&
- !isNaN(selectionEnd) &&
- selectionStart !== selectionEnd;
- }
- return hasSelection;
- };
-}
+ let worker = internal(item).workerMap.get(window);
@Gozala
Gozala Oct 8, 2012

I've seen this several times. I think factoring it into own function would improve readability:
getItemWorkerFor(item, window)

@Mossop
Mossop Oct 12, 2012

This only exists in two functions, one is getItemWorkerForWindow which also creates a worker if one doesn't exist and destroyItemWorkerForWindow.

@Gozala Gozala commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
-function URLContext(patterns) {
- let opts = apiUtils.validateOptions({ patterns: patterns }, {
- patterns: {
- map: function (v) apiUtils.getTypeOf(v) === "array" ? v : [v],
- ok: function (v) v.every(function (p) typeof(p) === "string"),
- msg: "patterns must be a string or an array of strings."
+ worker = ContextWorker({
+ window: window,
+ contentScript: item.contentScript,
+ contentScriptFile: item.contentScriptFile,
+ onError: function (err) console.exception(err),
@Gozala
Gozala Oct 8, 2012

I believe these errors will be logged if no handler is provided, so this should be unnecessary.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
-exports.PageContext = apiUtils.publicConstructor(PageContext);
-exports.SelectorContext = apiUtils.publicConstructor(SelectorContext);
-exports.SelectionContext = apiUtils.publicConstructor(SelectionContext);
-exports.URLContext = apiUtils.publicConstructor(URLContext);
+ if (worker) {
+ let node = popupNode;
+ for (let context in item.context)
+ node = context.adjustPopupNode(node);
@Gozala
Gozala Oct 8, 2012

I find following a lot easier to follow:

let adjustedNode = item.context.reduce(function(node, context) context.adjustPopupNode(node), popupNode)
worker.fireClick(adjustedNode, clickedItem.data)
@Mossop
Mossop Oct 12, 2012

item.context isn't an array so reduce doesn't seem to be an option here unfortunately.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- is: ["string", "undefined", "null"]
- },
- contentScript: {
- is: ["string", "array", "undefined"],
- ok: function (v) {
- return apiUtils.getTypeOf(v) !== "array" ||
- v.every(function (s) typeof(s) === "string");
+// All things that appear in the context menu extend this
+let BaseItem = Class({
+ initialize: function initialize() {
+ addCollectionProperty(this, "context");
+
+ // Used to cache content script workers and the windows they have been
+ // created for
+ internal(this).workerMap = new WeakMap();
+ internal(this).workerWindows = [];
@Gozala
Gozala Oct 8, 2012

I guess in this case you could just use Map since workerWindows will hold a reference to WeakMap keys
preventing GC to claim values. Also Map's are iterable so having one Map would have replaced WeakMap and array.

@Mossop
Mossop Oct 8, 2012

Ah, I didn't know Map existed. MDN doesn't show a way to iterate one though.

@Gozala Gozala and 1 other commented on an outdated diff Oct 8, 2012
packages/addon-kit/lib/context-menu.js
- return apiUtils.getTypeOf(v) !== "array" ||
- v.every(function (s) typeof(s) === "string");
+// All things that appear in the context menu extend this
+let BaseItem = Class({
+ initialize: function initialize() {
+ addCollectionProperty(this, "context");
+
+ // Used to cache content script workers and the windows they have been
+ // created for
+ internal(this).workerMap = new WeakMap();
+ internal(this).workerWindows = [];
+
+ if ("context" in internal(this).options && internal(this).options.context) {
+ let contexts = internal(this).options.context;
+ if (Array.isArray(contexts)) {
+ for (let context of contexts)
@Gozala
Gozala Oct 8, 2012

I don't think for of is shipped yet.

@Mossop
Mossop Oct 8, 2012

It shipped in Firefox 13, if that isn't good enough then we can't use Map either.

@Gozala
Mozilla member

So far I have just skimmed through and provided various suggestions to improve code readability. I'll probably take another, deeper look at it later.

Mossop added some commits Oct 11, 2012
@Mossop Mossop Merge branch 'master' into context-menu
Conflicts:
	doc/module-source/sdk/context-menu.md
	lib/sdk/context-menu.js
c360758
@Mossop Mossop Addresses review comments. 553b54a
@Mossop Mossop Fix some intermittent test failures by making sure it is the main doc…
…ument that has loaded rather than the inner frame.
ac3b1aa
@Gozala Gozala and 1 other commented on an outdated diff Nov 20, 2012
doc/module-source/sdk/context-menu.md
@@ -31,6 +31,13 @@ user visits a certain page, don't create the item when that page loads, and
don't remove it when the page unloads. Rather, create your item only once and
supply a context that matches the target URL.
+Context menu items are displayed in the order created or in the case of sub
+menus the order added to the sub menu. Menu items for each add-on will be
+grouped together automatically. If the total number of menu items in the main
+context menu from all add-ons exceeds a certain number (normally 10) all of the
@Gozala
Gozala Nov 20, 2012

Not super important but maybe we should have an pref with max number limit that by default is set to 10.

@Mossop
Mossop Nov 21, 2012

There already is a pref controlling it, it was just never documented before so I've added a note about that.

@Gozala Gozala commented on the diff Nov 20, 2012
lib/sdk/context-menu.js
+function getScheme(spec) {
@Gozala
Gozala Nov 20, 2012

I think we should change URL.prototype.scheme getter so that it never throws and returns null on scheme instead.

@Mossop
Mossop Nov 21, 2012

The thing that throws here is URL(spec) as it may not be a valid url.

@Gozala Gozala commented on an outdated diff Nov 20, 2012
lib/sdk/context-menu.js
- get label() {
- return this._label;
- },
+ try {
@Gozala
Gozala Nov 20, 2012

Could you please add comment explaining in which cases this is supposed to throw.

@Gozala Gozala commented on the diff Nov 20, 2012
lib/sdk/context-menu.js
},
- set image(val) {
- this._image = validateOpt(val, this._optRules.image);
- if (this._isInited)
- browserManager.setItemImage(this, this._image);
- return this._image;
- },
+ adjustPopupNode: function adjustPopupNode(popupNode) {
@Gozala
Gozala Nov 20, 2012

Do we really need public method like this ? I think it would be better to just have findMatchingNode(selection, node) function used with in this module

@Gozala
Gozala Nov 20, 2012

On the second thought it may be even make more sense in util/dom module or alike.

@Mossop
Mossop Nov 23, 2012

This is something that already exists in context-menu, I decided to leave it to avoid breaking any potential consumers. We can remove it in a follow-up if it makes more sense though

@Gozala Gozala commented on the diff Nov 20, 2012
lib/sdk/context-menu.js
- while (from <= to) {
- let i = Math.floor((from + to) / 2);
- let comp = targetLabel.localeCompare(elts[i].getAttribute("label"));
- if (comp < 0)
- to = i - 1;
- else if (comp > 0)
- from = i + 1;
- else
- return elts[i];
- }
- return elts[from] || null;
-}
+// All things that appear in the context menu extend this
+let BaseItem = Class({
+ initialize: function initialize() {
+ addCollectionProperty(this, "context");
@Gozala
Gozala Nov 20, 2012

I think it's probably better to use util/list instead.

@Mossop
Mossop Nov 21, 2012

util/list doesn't have add/remove methods

@Gozala Gozala commented on an outdated diff Nov 20, 2012
lib/sdk/context-menu.js
-WorkerRegistry.prototype = {
+ destroy: function destroy() {
+ for (let [window,] of internal(this).workerMap)
@Gozala
Gozala Nov 20, 2012

Is that trailing , after window necessary ?

@Gozala Gozala commented on an outdated diff Nov 21, 2012
lib/sdk/context-menu.js
-// A type of Worker tailored to our uses.
-const ContextMenuWorker = Worker.compose({
- destroy: Worker.required,
+ destroy: function destroy() {
+ if (internal(this).parentMenu)
@Gozala
Gozala Nov 21, 2012

It looks like internal(this).parentMenu is already exposed via parentMenu won't it be better to just use this.parentMenu instead ? In addition there seems to be internal(this).options.parentMenu in mix with two other parentMenu-s it's really confusing.

@Gozala Gozala commented on an outdated diff Nov 21, 2012
lib/sdk/context-menu.js
},
- // Emits a click event in the worker's port. popupNode is the node that was
- // context-clicked, and clickedItemData is the data of the item that was
- // clicked.
- fireClick: function CMW_fireClick(popupNode, clickedItemData) {
- this._contentWorker.emitSync("click", popupNode, clickedItemData);
+ get contentScriptFile() {
@Gozala
Gozala Nov 21, 2012

There are bunch geaters here that just proxy to internal(this).options.propertyName. Could properties on options change ? If not can we just define non-writable / non-configurable properties in the constructor function to avoid all of these proxy-ing would also make code easier to read IMO.

@Gozala
Mozilla member

Nit: One more thing is that our general convention to export things is:

let Foo = ...
exports.Foo = Foo
@Gozala
Mozilla member

@Mossop This is awesome!! Let's land this as soon as we can!!

\o/ \o/ \o/ \o/

I made few minor suggestions, but will be more than happy to land as is! Or if you wanna address some of those that's fine too, I don't think we need another review for that.

@Mossop
Mozilla member

Thanks, I'll go through your comments and land it tomorrow.

@Mossop
Mozilla member

Pushed this with clean history as 800c001

@Mossop Mossop closed this Nov 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment