From bc82016ee13968f73216b272a4516ef5fd98c201 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 20 Feb 2015 12:39:56 +0000 Subject: [PATCH 01/63] runat-1128988: Merge increment() and decrement() into nudge() This is a simplification I've wanted to do for ages, those functions form part of the front interface so we really should get it right before we start using it in anger. Signed-off-by: Joe Walker --- docs/writing-types.md | 5 ++- lib/gcli/cli.js | 21 ++----------- lib/gcli/connectors/remoted.js | 41 +++++------------------- lib/gcli/languages/command.js | 4 +-- lib/gcli/test/testDate.js | 6 ++-- lib/gcli/types/date.js | 25 ++++----------- lib/gcli/types/delegate.js | 24 +++----------- lib/gcli/types/number.js | 34 ++++++++++---------- lib/gcli/types/selection.js | 57 +++++++++++++++++----------------- lib/gcli/types/types.js | 14 +++------ lib/gcli/ui/menu.js | 19 ++---------- lib/gcli/ui/terminal.js | 22 ++++--------- 12 files changed, 88 insertions(+), 184 deletions(-) diff --git a/docs/writing-types.md b/docs/writing-types.md index 4779edc8..ee9232d6 100644 --- a/docs/writing-types.md +++ b/docs/writing-types.md @@ -7,7 +7,7 @@ number of built in types: * string. This is a JavaScript string * number. A JavaScript number -* boolean. A Javascript boolean +* boolean. A JavaScript boolean * selection. This is an selection from a number of alternatives * delegate. This type could change depending on other factors, but is well defined when one of the conversion routines is called. @@ -51,8 +51,7 @@ All types must inherit from Type and have the following methods: In addition, defining the following functions can be helpful, although Type contains default implementations: -* increment(value) -* decrement(value) +* nudge(value, by) Type, Conversion and Status are all declared by commands.js. diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js index 0fdaee9f..532ba001 100644 --- a/lib/gcli/cli.js +++ b/lib/gcli/cli.js @@ -1446,26 +1446,9 @@ Requisition.prototype.complete = function(cursor, rank) { /** * Replace the current value with the lower value if such a concept exists. */ -Requisition.prototype.decrement = function(assignment) { +Requisition.prototype.nudge = function(assignment, by) { var ctx = this.executionContext; - var val = assignment.param.type.decrement(assignment.value, ctx); - return Promise.resolve(val).then(function(replacement) { - if (replacement != null) { - var val = assignment.param.type.stringify(replacement, ctx); - return Promise.resolve(val).then(function(str) { - var arg = assignment.arg.beget({ text: str }); - return this.setAssignment(assignment, arg); - }.bind(this)); - } - }.bind(this)); -}; - -/** - * Replace the current value with the higher value if such a concept exists. - */ -Requisition.prototype.increment = function(assignment) { - var ctx = this.executionContext; - var val = assignment.param.type.increment(assignment.value, ctx); + var val = assignment.param.type.nudge(assignment.value, by, ctx); return Promise.resolve(val).then(function(replacement) { if (replacement != null) { var val = assignment.param.type.stringify(replacement, ctx); diff --git a/lib/gcli/connectors/remoted.js b/lib/gcli/connectors/remoted.js index 92cc3586..2045b58e 100644 --- a/lib/gcli/connectors/remoted.js +++ b/lib/gcli/connectors/remoted.js @@ -153,13 +153,13 @@ Remoter.prototype.exposed = { }), /** - * Get the incremented value of some type + * Get the incremented/decremented value of some type * @return a promise of a string containing the new argument text */ - incrementType: method(function(typed, param) { + nudgeType: method(function(typed, by, param) { return this.requisition.update(typed).then(function() { var assignment = this.requisition.getAssignment(param); - return this.requisition.increment(assignment).then(function() { + return this.requisition.nudge(assignment, by).then(function() { var arg = assignment.arg; return arg == null ? undefined : arg.text; }); @@ -167,26 +167,8 @@ Remoter.prototype.exposed = { }, { request: { typed: Arg(0, "string"), // The command string - param: Arg(1, "string") // The name of the parameter to parse - }, - response: RetVal("string") - }), - - /** - * See incrementType - */ - decrementType: method(function(typed, param) { - return this.requisition.update(typed).then(function() { - var assignment = this.requisition.getAssignment(param); - return this.requisition.decrement(assignment).then(function() { - var arg = assignment.arg; - return arg == null ? undefined : arg.text; - }); - }.bind(this)); - }, { - request: { - typed: Arg(0, "string"), // The command string - param: Arg(1, "string") // The name of the parameter to parse + by: Arg(1, "number"), // +1/-1 for increment / decrement + param: Arg(2, "string") // The name of the parameter to parse }, response: RetVal("string") }), @@ -372,20 +354,13 @@ GcliFront.prototype.parseType = function(typed, param) { return this.connection.call('parseType', data); }; -GcliFront.prototype.incrementType = function(typed, param) { - var data = { - typed: typed, - param: param - }; - return this.connection.call('incrementType', data); -}; - -GcliFront.prototype.decrementType = function(typed, param) { +GcliFront.prototype.nudgeType = function(typed, by, param) { var data = { typed: typed, + by: by, param: param }; - return this.connection.call('decrementType', data); + return this.connection.call('nudgeType', by, data); }; GcliFront.prototype.getSelectionLookup = function(commandName, paramName) { diff --git a/lib/gcli/languages/command.js b/lib/gcli/languages/command.js index e8ec20af..6cfa4689 100644 --- a/lib/gcli/languages/command.js +++ b/lib/gcli/languages/command.js @@ -251,7 +251,7 @@ var commandLanguage = exports.commandLanguage = { // If the user is on a valid value, then we increment the value, but if // they've typed something that's not right we page through predictions if (this.assignment.getStatus() === Status.VALID) { - return this.requisition.increment(this.assignment).then(function() { + return this.requisition.nudge(this.assignment, 1).then(function() { this.textChanged(); this.focusManager.onInputChange(); return true; @@ -266,7 +266,7 @@ var commandLanguage = exports.commandLanguage = { */ handleDownArrow: function() { if (this.assignment.getStatus() === Status.VALID) { - return this.requisition.decrement(this.assignment).then(function() { + return this.requisition.nudge(this.assignment, -1).then(function() { this.textChanged(); this.focusManager.onInputChange(); return true; diff --git a/lib/gcli/test/testDate.js b/lib/gcli/test/testDate.js index 47566ed5..731b738f 100644 --- a/lib/gcli/test/testDate.js +++ b/lib/gcli/test/testDate.js @@ -42,15 +42,15 @@ exports.testMaxMin = function(options) { var date = types.createType({ name: 'date', max: max, min: min }); assert.is(date.getMax(), max, 'max setup'); - var incremented = date.increment(min); + var incremented = date.nudge(min, 1); assert.is(incremented, max, 'incremented'); }; exports.testIncrement = function(options) { var date = options.requisition.system.types.createType('date'); return date.parseString('now').then(function(conversion) { - var plusOne = date.increment(conversion.value); - var minusOne = date.decrement(plusOne); + var plusOne = date.nudge(conversion.value, 1); + var minusOne = date.nudge(plusOne, -1); // See comments in testParse var gap = new Date().getTime() - minusOne.getTime(); diff --git a/lib/gcli/types/date.js b/lib/gcli/types/date.js index 99a77fae..b3225905 100644 --- a/lib/gcli/types/date.js +++ b/lib/gcli/types/date.js @@ -227,35 +227,22 @@ exports.items = [ return Promise.resolve(new Conversion(value, arg)); }, - decrement: function(value, context) { + nudge: function(value, by, context) { if (!isDate(value)) { return new Date(); } var newValue = new Date(value); - newValue.setDate(value.getDate() - this.step); + newValue.setDate(value.getDate() + (by * this.step)); - if (newValue >= this.getMin(context)) { - return newValue; - } - else { + if (newValue < this.getMin(context)) { return this.getMin(context); } - }, - - increment: function(value, context) { - if (!isDate(value)) { - return new Date(); - } - - var newValue = new Date(value); - newValue.setDate(value.getDate() + this.step); - - if (newValue <= this.getMax(context)) { - return newValue; + else if (newValue > this.getMax(context)) { + return this.getMax(); } else { - return this.getMax(); + return newValue; } } } diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index 1e9cb99f..b893a964 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -53,18 +53,10 @@ exports.items = [ }.bind(this)); }, - decrement: function(value, context) { + nudge: function(value, by, context) { return this.getType(context).then(function(delegated) { - return delegated.decrement ? - delegated.decrement(value, context) : - undefined; - }.bind(this)); - }, - - increment: function(value, context) { - return this.getType(context).then(function(delegated) { - return delegated.increment ? - delegated.increment(value, context) : + return delegated.nudge ? + delegated.nudge(value, by, context) : undefined; }.bind(this)); }, @@ -115,14 +107,8 @@ exports.items = [ }); }, - decrement: function(value, context) { - return this.front.decrementType(context.typed, this.param).then(function(json) { - return { stringified: json.arg }; - }); - }, - - increment: function(value, context) { - return this.front.incrementType(context.typed, this.param).then(function(json) { + nudge: function(value, by, context) { + return this.front.nudgeType(context.typed, by, this.param).then(function(json) { return { stringified: json.arg }; }); } diff --git a/lib/gcli/types/number.js b/lib/gcli/types/number.js index bec67b18..5ed287d3 100644 --- a/lib/gcli/types/number.js +++ b/lib/gcli/types/number.js @@ -132,26 +132,28 @@ exports.items = [ return Promise.resolve(new Conversion(value, arg)); }, - decrement: function(value, context) { + nudge: function(value, by, context) { if (typeof value !== 'number' || isNaN(value)) { - return this.getMax(context) || 1; + if (by < 0) { + return this.getMax(context) || 1; + } + else { + var min = this.getMin(context); + return min != null ? min : 0; + } } - var newValue = value - this.step; - // Snap to the nearest incremental of the step - newValue = Math.ceil(newValue / this.step) * this.step; - return this._boundsCheck(newValue, context); - }, - increment: function(value, context) { - if (typeof value !== 'number' || isNaN(value)) { - var min = this.getMin(context); - return min != null ? min : 0; - } - var newValue = value + this.step; + var newValue = value + (by * this.step); + // Snap to the nearest incremental of the step - newValue = Math.floor(newValue / this.step) * this.step; - if (this.getMax(context) == null) { - return newValue; + if (by < 0) { + newValue = Math.ceil(newValue / this.step) * this.step; + } + else { + newValue = Math.floor(newValue / this.step) * this.step; + if (this.getMax(context) == null) { + return newValue; + } } return this._boundsCheck(newValue, context); }, diff --git a/lib/gcli/types/selection.js b/lib/gcli/types/selection.js index 1ab4fea7..2ed41ce0 100644 --- a/lib/gcli/types/selection.js +++ b/lib/gcli/types/selection.js @@ -322,19 +322,40 @@ SelectionType.prototype.getBlank = function(context) { }; /** - * For selections, up is down and black is white. It's like this, given a list - * [ a, b, c, d ], it's natural to think that it starts at the top and that - * going up the list, moves towards 'a'. However 'a' has the lowest index, so - * for SelectionType, up is down and down is up. - * Sorry. + * Increment and decrement are confusing for selections. +1 is -1 and -1 is +1. + * Given an array e.g. [ 'a', 'b', 'c' ] with the current selection on 'b', + * displayed to the user in the natural way, i.e.: + * + * 'a' + * 'b' <- highlighted as current value + * 'c' + * + * Pressing the UP arrow should take us to 'a', which decrements this index + * (compare pressing UP on a number which would increment the number) + * + * So for selections, we treat +1 as -1 and -1 as +1. */ -SelectionType.prototype.decrement = function(value, context) { +SelectionType.prototype.nudge = function(value, by, context) { return this.getLookup(context).then(function(lookup) { var index = this._findValue(lookup, value); if (index === -1) { - index = 0; + if (by < 0) { + // We're supposed to be doing a decrement (which means +1), but the + // value isn't found, so we reset the index to the top of the list + // which is index 0 + index = 0; + } + else { + // For an increment operation when there is nothing to start from, we + // want to start from the top, i.e. index 0, so the value before we + // 'increment' (see note above) must be 1. + index = 1; + } } - index++; + + // This is where we invert the sense of up/down (see doc comment) + index -= by; + if (index >= lookup.length) { index = 0; } @@ -342,26 +363,6 @@ SelectionType.prototype.decrement = function(value, context) { }.bind(this)); }; -/** - * See note on SelectionType.decrement() - */ -SelectionType.prototype.increment = function(value, context) { - return this.getLookup(context).then(function(lookup) { - var index = this._findValue(lookup, value); - if (index === -1) { - // For an increment operation when there is nothing to start from, we - // want to start from the top, i.e. index 0, so the value before we - // 'increment' (see note above) must be 1. - index = 1; - } - index--; - if (index < 0) { - index = lookup.length - 1; - } - return lookup[index].value; - }.bind(this)); -}; - /** * Walk through an array of { name:.., value:... } objects looking for a * matching value (using strict equality), returning the matched index (or -1 diff --git a/lib/gcli/types/types.js b/lib/gcli/types/types.js index 5b672c71..c5ceb8e9 100644 --- a/lib/gcli/types/types.js +++ b/lib/gcli/types/types.js @@ -995,18 +995,12 @@ Type.prototype.parseString = function(str, context) { Type.prototype.name = undefined; /** - * If there is some concept of a higher value, return it, + * If there is some concept of a lower or higher value, return it, * otherwise return undefined. + * @param by number indicating how much to nudge by, usually +1 or -1 which is + * caused by the user pressing the UP/DOWN keys with the cursor in this type */ -Type.prototype.increment = function(value, context) { - return undefined; -}; - -/** - * If there is some concept of a lower value, return it, - * otherwise return undefined. - */ -Type.prototype.decrement = function(value, context) { +Type.prototype.nudge = function(value, by, context) { return undefined; }; diff --git a/lib/gcli/ui/menu.js b/lib/gcli/ui/menu.js index 6ba00e76..52b41538 100644 --- a/lib/gcli/ui/menu.js +++ b/lib/gcli/ui/menu.js @@ -263,9 +263,9 @@ Menu.prototype.getChoiceIndex = function() { }; /** - * Highlight the next option + * Highlight the next (for by=1) or previous (for by=-1) option */ -Menu.prototype.incrementChoice = function() { +Menu.prototype.nudgeChoice = function(by) { if (this._choice == null) { this._choice = 0; } @@ -273,20 +273,7 @@ Menu.prototype.incrementChoice = function() { // There's an annoying up is down thing here, the menu is presented // with the zeroth index at the top working down, so the UP arrow needs // pick the choice below because we're working down - this._choice--; - this._updateHighlight(); -}; - -/** - * Highlight the previous option - */ -Menu.prototype.decrementChoice = function() { - if (this._choice == null) { - this._choice = 0; - } - - // See incrementChoice - this._choice++; + this._choice -= by; this._updateHighlight(); }; diff --git a/lib/gcli/ui/terminal.js b/lib/gcli/ui/terminal.js index 8ddf687e..204c13c7 100644 --- a/lib/gcli/ui/terminal.js +++ b/lib/gcli/ui/terminal.js @@ -420,7 +420,7 @@ Terminal.prototype.handleKeyUp = function(ev) { if (ev.keyCode === KeyEvent.DOM_VK_UP) { if (this.isMenuShowing) { - return this.incrementChoice(); + return this.nudgeChoice(1); } if (this.inputElement.value === '' || this._scrollingThroughHistory) { @@ -430,14 +430,14 @@ Terminal.prototype.handleKeyUp = function(ev) { return this.language.handleUpArrow().then(function(handled) { if (!handled) { - return this.incrementChoice(); + return this.nudgeChoice(1); } }.bind(this)); } if (ev.keyCode === KeyEvent.DOM_VK_DOWN) { if (this.isMenuShowing) { - return this.decrementChoice(); + return this.nudgeChoice(-1); } if (this.inputElement.value === '' || this._scrollingThroughHistory) { @@ -447,7 +447,7 @@ Terminal.prototype.handleKeyUp = function(ev) { return this.language.handleDownArrow().then(function(handled) { if (!handled) { - return this.decrementChoice(); + return this.nudgeChoice(-1); } }.bind(this)); } @@ -529,22 +529,12 @@ Terminal.prototype.unsetChoice = function() { return this.updateCompletion(); }; -/** - * Select the previous option in a list of choices - */ -Terminal.prototype.incrementChoice = function() { - if (this.field && this.field.menu) { - this.field.menu.incrementChoice(); - } - return this.updateCompletion(); -}; - /** * Select the next option in a list of choices */ -Terminal.prototype.decrementChoice = function() { +Terminal.prototype.nudgeChoice = function(by) { if (this.field && this.field.menu) { - this.field.menu.decrementChoice(); + this.field.menu.nudgeChoice(by); } return this.updateCompletion(); }; From 5408bc6bbfe3408b0a0dc5d4c592bb0a92933f9c Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 13:30:09 +0000 Subject: [PATCH 02/63] runat-1128988: Use paramName consistently for parameter names The confusion was causing problems, so it seems simplest to fix it. Signed-off-by: Joe Walker --- lib/gcli/connectors/remoted.js | 24 ++++++++++++------------ lib/gcli/types/delegate.js | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/gcli/connectors/remoted.js b/lib/gcli/connectors/remoted.js index 2045b58e..5c8286fd 100644 --- a/lib/gcli/connectors/remoted.js +++ b/lib/gcli/connectors/remoted.js @@ -132,9 +132,9 @@ Remoter.prototype.exposed = { * - message: The message to display to the user * - predictions: An array of suggested values for the given parameter */ - parseType: method(function(typed, param) { + parseType: method(function(typed, paramName) { return this.requisition.update(typed).then(function() { - var assignment = this.requisition.getAssignment(param); + var assignment = this.requisition.getAssignment(paramName); return Promise.resolve(assignment.predictions).then(function(predictions) { return { @@ -147,7 +147,7 @@ Remoter.prototype.exposed = { }, { request: { typed: Arg(0, "string"), // The command string - param: Arg(1, "string") // The name of the parameter to parse + paramName: Arg(1, "string") // The name of the parameter to parse }, response: RetVal("json") }), @@ -156,9 +156,9 @@ Remoter.prototype.exposed = { * Get the incremented/decremented value of some type * @return a promise of a string containing the new argument text */ - nudgeType: method(function(typed, by, param) { + nudgeType: method(function(typed, by, paramName) { return this.requisition.update(typed).then(function() { - var assignment = this.requisition.getAssignment(param); + var assignment = this.requisition.getAssignment(paramName); return this.requisition.nudge(assignment, by).then(function() { var arg = assignment.arg; return arg == null ? undefined : arg.text; @@ -166,9 +166,9 @@ Remoter.prototype.exposed = { }.bind(this)); }, { request: { - typed: Arg(0, "string"), // The command string - by: Arg(1, "number"), // +1/-1 for increment / decrement - param: Arg(2, "string") // The name of the parameter to parse + typed: Arg(0, "string"), // The command string + by: Arg(1, "number"), // +1/-1 for increment / decrement + paramName: Arg(2, "string") // The name of the parameter to parse }, response: RetVal("string") }), @@ -346,19 +346,19 @@ GcliFront.prototype.parseFile = function(typed, filetype, existing, matches) { return this.connection.call('parseFile', data); }; -GcliFront.prototype.parseType = function(typed, param) { +GcliFront.prototype.parseType = function(typed, paramName) { var data = { typed: typed, - param: param + paramName: paramName }; return this.connection.call('parseType', data); }; -GcliFront.prototype.nudgeType = function(typed, by, param) { +GcliFront.prototype.nudgeType = function(typed, by, paramName) { var data = { typed: typed, by: by, - param: param + paramName: paramName }; return this.connection.call('nudgeType', by, data); }; diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index b893a964..78bc7c49 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -85,7 +85,7 @@ exports.items = [ { item: 'type', name: 'remote', - param: undefined, + paramName: undefined, stringify: function(value, context) { // remote types are client only, and we don't attempt to transfer value @@ -100,7 +100,7 @@ exports.items = [ }, parse: function(arg, context) { - return this.front.parseType(context.typed, this.param).then(function(json) { + return this.front.parseType(context.typed, this.paramName).then(function(json) { var status = Status.fromString(json.status); var val = { stringified: arg.text }; return new Conversion(val, arg, status, json.message, json.predictions); @@ -108,7 +108,7 @@ exports.items = [ }, nudge: function(value, by, context) { - return this.front.nudgeType(context.typed, by, this.param).then(function(json) { + return this.front.nudgeType(context.typed, by, this.paramName).then(function(json) { return { stringified: json.arg }; }); } From 585f7b83953de5c0faeb9fb187b55ab6171c3d03 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 13:31:22 +0000 Subject: [PATCH 03/63] runat-1128988: Clarify some comments Signed-off-by: Joe Walker --- lib/gcli/system.js | 7 +++++-- lib/gcli/types/delegate.js | 2 +- lib/gcli/types/types.js | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index c62f8d8a..56716216 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -290,10 +290,13 @@ function addLocalFunctions(specs, front) { // all the remote types specs.forEach(function(commandSpec) { // HACK: Tack the front to the command so we know how to remove it - // in syncItems() below + // in syncItems() above commandSpec.front = front; - // TODO: syncItems() doesn't remove types, so do we need this? + // Tell the type instances for a command how to contact their counterparts + // Don't confuse this with setting the front on the commandSpec which is + // about associating a proxied command with it's source for later removal. + // This is actually going to be used by the type commandSpec.params.forEach(function(param) { if (typeof param.type !== 'string') { param.type.front = front; diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index 78bc7c49..b1815747 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -90,7 +90,7 @@ exports.items = [ stringify: function(value, context) { // remote types are client only, and we don't attempt to transfer value // objects to the client (we can't be sure the are jsonable) so it is a - // but strange to be asked to stringify a value object, however since + // bit strange to be asked to stringify a value object, however since // parse creates a Conversion with a (fake) value object we might be // asked to stringify that. We can stringify fake value objects. if (typeof value.stringified === 'string') { diff --git a/lib/gcli/types/types.js b/lib/gcli/types/types.js index c5ceb8e9..014b5934 100644 --- a/lib/gcli/types/types.js +++ b/lib/gcli/types/types.js @@ -947,8 +947,8 @@ function Type() { /** * Get a JSONable data structure that entirely describes this type. - * commandName and paramName are the names of the command and parameter which we - * are remoting to help the server get back to the remoted action. + * commandName and paramName are the names of the command and parameter which + * we are remoting to help the server get back to the remoted action. */ Type.prototype.getSpec = function(commandName, paramName) { throw new Error('Not implemented'); From d5be4854451e53822a6e03346af9e2e6ed1f41d0 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 13:33:34 +0000 Subject: [PATCH 04/63] runat-1128988: Fix some missing Function.bind() calls More testing of async as a result of the use in Firefox has flushed out some cases that were not correct. Signed-off-by: Joe Walker --- lib/gcli/testharness/examiner.js | 4 ++-- lib/gcli/types/delegate.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/gcli/testharness/examiner.js b/lib/gcli/testharness/examiner.js index edabd3c6..8384eaa5 100644 --- a/lib/gcli/testharness/examiner.js +++ b/lib/gcli/testharness/examiner.js @@ -350,10 +350,10 @@ Test.prototype.run = function(options) { var reply = this.func.apply(this.suite, [ options ]); Promise.resolve(reply).then(function() { resolve(); - }, function(err) { + }.bind(this), function(err) { assert.ok(false, 'Returned promise, rejected with: ' + toString(err)); resolve(); - }); + }.bind(this)); } catch (ex) { assert.ok(false, 'Exception: ' + toString(ex)); diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index b1815747..d5f40a75 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -103,14 +103,14 @@ exports.items = [ return this.front.parseType(context.typed, this.paramName).then(function(json) { var status = Status.fromString(json.status); var val = { stringified: arg.text }; - return new Conversion(val, arg, status, json.message, json.predictions); - }); + return new Conversion(undefined, arg, status, json.message, json.predictions); + }.bind(this)); }, nudge: function(value, by, context) { return this.front.nudgeType(context.typed, by, this.paramName).then(function(json) { return { stringified: json.arg }; - }); + }.bind(this)); } }, // 'blank' is a type for use with DelegateType when we don't know yet. From 08be1684d4444a526d1e6184ea6135d5e3152b44 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 13:37:58 +0000 Subject: [PATCH 05/63] runat-1128988: Command specs that want to use fronts need to be objects The server sends a block of JSON to the client describing the commands and on the client the parameter types are turned into proper Type objects and the 'front' object injected, but this can only happen if the spec is an object rather than just a string. The remote type should be able to remote itself trivially. Signed-off-by: Joe Walker --- lib/gcli/types/delegate.js | 8 ++++++++ lib/gcli/types/node.js | 12 +++++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index d5f40a75..2352e738 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -87,6 +87,14 @@ exports.items = [ name: 'remote', paramName: undefined, + getSpec: function(commandName, paramName) { + return { + name: 'remote', + commandName: commandName, + paramName: paramName + }; + }, + stringify: function(value, context) { // remote types are client only, and we don't attempt to transfer value // objects to the client (we can't be sure the are jsonable) so it is a diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index ae4bd701..51825b4a 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -94,8 +94,12 @@ exports.items = [ item: 'type', name: 'node', - getSpec: function() { - return 'node'; + getSpec: function(commandName, paramName) { + return { + name: 'remote', + commandName: commandName, + paramName: paramName + }; }, stringify: function(value, context) { @@ -168,9 +172,7 @@ exports.items = [ }, getSpec: function() { - return this.allowEmpty ? - { name: 'nodelist', allowEmpty: true } : - 'nodelist'; + return { name: 'nodelist', allowEmpty: this.allowEmpty }; }, getBlank: function(context) { From 345fdda87d0d78bc4d5e0b11c1e25b4daae6af55 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 13:43:14 +0000 Subject: [PATCH 06/63] runat-1128988: mockCommands added json converters that we added to basic I'm not clear why we thought that being able to display json was only something you'd want to do during testing, but since we later added them to the default set of converters, we don't need duplicates in test code. Signed-off-by: Joe Walker --- lib/gcli/converters/basic.js | 9 ++++++--- lib/gcli/test/mockCommands.js | 11 ----------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/lib/gcli/converters/basic.js b/lib/gcli/converters/basic.js index 2efd9abb..7ce0848f 100644 --- a/lib/gcli/converters/basic.js +++ b/lib/gcli/converters/basic.js @@ -57,9 +57,12 @@ exports.items = [ { item: 'converter', from: 'json', - to: 'dom', - exec: function(json, conversionContext) { - return nodeFromDataToString(JSON.stringify(json), conversionContext); + to: 'view', + exec: function(json, context) { + var html = JSON.stringify(json, null, ' ').replace(/\n/g, '
'); + return { + html: '
' + html + '
' + }; } }, { diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index 63a1edd1..4f4bf688 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -45,21 +45,10 @@ function createExec(name) { mockCommands.items = [ { item: 'converter', - from: 'json', - to: 'string', - exec: function(json, context) { - return JSON.stringify(json, null, ' '); } }, { item: 'converter', - from: 'json', - to: 'view', - exec: function(json, context) { - var html = JSON.stringify(json, null, ' ').replace(/\n/g, '
'); - return { - html: '
' + html + '
' - }; } }, { From 989537615525b0d5ac980b1681e12177d8506fc4 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Sat, 21 Feb 2015 14:10:45 +0000 Subject: [PATCH 07/63] runat-1128988: Strengthen the tests we can make using mockCommands Most of the test commands had an exec function generated by 'createExec' which previously outputted (synchronously) a block of text that described the arguments (using toString) This had the problem that we couldn't test the values individually, so now createExec returns a testCommandOutput structure containing the correct argument stringified version (when promises). A number of tests change to reflect this, and it needs an easy way to lookup parameters by name in Command. Signed-off-by: Joe Walker --- lib/gcli/commands/commands.js | 13 ++++++++ lib/gcli/test/mockCommands.js | 59 ++++++++++++++++++++++++++++++++--- lib/gcli/test/testDate.js | 8 ++--- lib/gcli/test/testExec.js | 24 +++++++------- lib/gcli/test/testInputter.js | 2 +- lib/gcli/test/testNode.js | 8 +++-- 6 files changed, 90 insertions(+), 24 deletions(-) diff --git a/lib/gcli/commands/commands.js b/lib/gcli/commands/commands.js index e76ea8b7..17643117 100644 --- a/lib/gcli/commands/commands.js +++ b/lib/gcli/commands/commands.js @@ -195,6 +195,19 @@ Command.prototype.toJson = function(customProps) { return json; }; +/** + * Easy way to lookup parameters by full name + */ +Command.prototype.getParameterByName = function(name) { + var reply; + this.params.forEach(function(param) { + if (param.name === name) { + reply = param; + } + }); + return reply; +}; + /** * Easy way to lookup parameters by short name */ diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index 4f4bf688..e965b10b 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -34,21 +34,70 @@ mockCommands.shutdown = function(requisition) { }; function createExec(name) { - return function(args, executionContext) { - var argsOut = Object.keys(args).map(function(key) { - return key + '=' + args[key]; - }).join(', '); - return 'Exec: ' + name + ' ' + argsOut; + return function(args, context) { + var promises = []; + + Object.keys(args).map(function(argName) { + var value = args[argName]; + var type = this.getParameterByName(argName).type; + var promise = Promise.resolve(type.stringify(value, context)).then(function(str) { + return { name: argName, value: str }; + }.bind(this)); + promises.push(promise); + }.bind(this)); + + return Promise.all(promises).then(function(data) { + var argValues = {}; + data.forEach(function(entry) { argValues[entry.name] = entry.value; }); + + return context.typedData('testCommandOutput', { + name: name, + args: argValues + }); + }.bind(this)); }; } mockCommands.items = [ { item: 'converter', + from: 'testCommandOutput', + to: 'dom', + exec: function(testCommandOutput, context) { + var view = context.createView({ + data: testCommandOutput, + html: '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
Exec: ${name}
${key}=${args[key]}
', + options: { + allowEval: true + } + }); + + return view.toDom(context.document); } }, { item: 'converter', + from: 'testCommandOutput', + to: 'string', + exec: function(testCommandOutput, context) { + var argsOut = Object.keys(testCommandOutput.args).map(function(key) { + return key + '=' + testCommandOutput.args[key]; + }).join(' '); + return 'Exec: ' + testCommandOutput.name + ' ' + argsOut; } }, { diff --git a/lib/gcli/test/testDate.js b/lib/gcli/test/testDate.js index 731b738f..f2fdc47b 100644 --- a/lib/gcli/test/testDate.js +++ b/lib/gcli/test/testDate.js @@ -102,7 +102,7 @@ exports.testInput = function(options) { }, exec: { output: [ /^Exec: tsdate/, /2001/, /1980/ ], - type: 'string', + type: 'testCommandOutput', error: false } }, @@ -148,7 +148,7 @@ exports.testInput = function(options) { }, exec: { output: [ /^Exec: tsdate/, /2001/, /1980/ ], - type: 'string', + type: 'testCommandOutput', error: false } }, @@ -189,7 +189,7 @@ exports.testInput = function(options) { }, exec: { output: [ /^Exec: tsdate/, new Date().getFullYear() ], - type: 'string', + type: 'testCommandOutput', error: false } }, @@ -229,7 +229,7 @@ exports.testInput = function(options) { }, exec: { output: [ /^Exec: tsdate/, new Date().getFullYear() ], - type: 'string', + type: 'testCommandOutput', error: false } } diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index 018e2b16..e41204b3 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -97,7 +97,7 @@ exports.testWithHelpers = function(options) { } }, exec: { - output: 'Exec: tsv optionType=string, optionValue=10' + output: 'Exec: tsv optionType=option1 optionValue=10' } }, { @@ -127,7 +127,7 @@ exports.testWithHelpers = function(options) { } }, exec: { - output: 'Exec: tsv optionType=number, optionValue=10' + output: 'Exec: tsv optionType=option2 optionValue=10' } }, // Delegated remote types can't transfer value types so we only test for @@ -139,7 +139,7 @@ exports.testWithHelpers = function(options) { args: { optionValue: { value: '10' } } }, exec: { - output: 'Exec: tsv optionType=string, optionValue=10' + output: 'Exec: tsv optionType=option1 optionValue=10' } }, { @@ -149,7 +149,7 @@ exports.testWithHelpers = function(options) { args: { optionValue: { value: 10 } } }, exec: { - output: 'Exec: tsv optionType=number, optionValue=10' + output: 'Exec: tsv optionType=option2 optionValue=10' } } ]); @@ -204,7 +204,7 @@ exports.testExecText = function(options) { } }, exec: { - output: 'Exec: tsr text=fred bloggs' + output: 'Exec: tsr text=fred\\ bloggs' } }, { @@ -229,7 +229,7 @@ exports.testExecText = function(options) { } }, exec: { - output: 'Exec: tsr text=fred bloggs' + output: 'Exec: tsr text=fred\\ bloggs' } }, { @@ -254,7 +254,7 @@ exports.testExecText = function(options) { } }, exec: { - output: 'Exec: tsr text=fred bloggs' + output: 'Exec: tsr text=fred\\ bloggs' } } ]); @@ -528,7 +528,7 @@ exports.testExecArray = function(options) { } }, exec: { - output: 'Exec: tselarr num=1, arr=' + output: 'Exec: tselarr num=1 arr=' } }, { @@ -549,7 +549,7 @@ exports.testExecArray = function(options) { } }, exec: { - output: 'Exec: tselarr num=1, arr=a' + output: 'Exec: tselarr num=1 arr=a' } }, { @@ -570,7 +570,7 @@ exports.testExecArray = function(options) { } }, exec: { - output: 'Exec: tselarr num=1, arr=a,b' + output: 'Exec: tselarr num=1 arr=a b' } } ]); @@ -597,7 +597,7 @@ exports.testExecMultiple = function(options) { } }, exec: { - output: 'Exec: tsm abc=a, txt=10, num=10' + output: 'Exec: tsm abc=a txt=10 num=10' } } ]); @@ -627,7 +627,7 @@ exports.testExecDefaults = function(options) { } }, exec: { - output: 'Exec: tsg solo=aaa, txt1=null, bool=false, txt2=d, num=42' + output: 'Exec: tsg solo=aaa txt1= bool=false txt2=d num=42' } } ]); diff --git a/lib/gcli/test/testInputter.js b/lib/gcli/test/testInputter.js index 7ca3be42..f19ef82c 100644 --- a/lib/gcli/test/testInputter.js +++ b/lib/gcli/test/testInputter.js @@ -65,7 +65,7 @@ exports.testOutput = function(options) { var ev1 = { keyCode: KeyEvent.DOM_VK_RETURN }; return terminal.handleKeyUp(ev1).then(function() { assert.ok(latestEvent != null, 'events this test'); - assert.is(latestData, 'Exec: tss ', 'last command is tss'); + assert.is(latestData.name, 'tss', 'last command is tss'); assert.is(terminal.getInputState().typed, '', diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index 8d3d7024..46d43972 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -177,8 +177,12 @@ exports.testNodeDom = function(options) { nodes: { status: 'VALID' }, nodes2: { status: 'VALID' } } - }, - post: function() { + } + }, + { + skipIf: options.isRemote, // arg values are unavailable remotely + setup: 'tse :root ', + post: function(output) { assert.is(requisition.getAssignment('node').value.tagName, 'HTML', 'root id'); From 37ceccbed36c975a68dafe619d5dc26b5511407a Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 11:00:43 +0000 Subject: [PATCH 08/63] runat-1128988: Support io.js io.js uses the full path to node in process.title. Signed-off-by: Joe Walker --- lib/gcli/commands/test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gcli/commands/test.js b/lib/gcli/commands/test.js index 84bdaf14..f117e434 100644 --- a/lib/gcli/commands/test.js +++ b/lib/gcli/commands/test.js @@ -70,7 +70,8 @@ exports.items = [ } else { options = { - isNode: (typeof(process) !== 'undefined' && process.title === 'node'), + isNode: (typeof(process) !== 'undefined' && + process.title.indexOf('node') != -1), isFirefox: false, isPhantomjs: false, isNoDom: true, From 2847992e81212547b6b228510b6a488058d81059 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 11:20:26 +0000 Subject: [PATCH 09/63] runat-1128988: Mock 'document' better We had various hacky ways to mock and setup the document against which the 'node' and 'resource' types acted, so we could run tests against NodeJS, the web, the web remoted to NodeJS and Firefox. With this change, all tests assume that the environment is setup correctly so there is some sort of a DOM. We remove the hacky fake DOM and replace it with a less hacky DOM from jsdom. Signed-off-by: Joe Walker --- lib/gcli/commands/mocks.js | 3 +++ lib/gcli/test/mockDocument.js | 45 +++++++++++++++++++++++++++++++++++ lib/gcli/test/testCli2.js | 12 ---------- lib/gcli/test/testExec.js | 33 ++++--------------------- lib/gcli/test/testNode.js | 11 --------- lib/gcli/test/testTypes.js | 11 --------- web/gcli/test/mockDocument.js | 26 ++++++++++++++++++++ 7 files changed, 78 insertions(+), 63 deletions(-) create mode 100644 lib/gcli/test/mockDocument.js create mode 100644 web/gcli/test/mockDocument.js diff --git a/lib/gcli/commands/mocks.js b/lib/gcli/commands/mocks.js index 16d2605b..e14c124a 100644 --- a/lib/gcli/commands/mocks.js +++ b/lib/gcli/commands/mocks.js @@ -19,6 +19,7 @@ var cli = require('../cli'); var mockCommands = require('../test/mockCommands'); var mockSettings = require('../test/mockSettings'); +var mockDocument = require('../test/mockDocument'); exports.items = [ { @@ -46,11 +47,13 @@ exports.items = [ on: function(requisition) { mockCommands.setup(requisition); mockSettings.setup(requisition.system); + mockDocument.setup(); }, off: function(requisition) { mockCommands.shutdown(requisition); mockSettings.shutdown(requisition.system); + mockDocument.shutdown(); } } ]; diff --git a/lib/gcli/test/mockDocument.js b/lib/gcli/test/mockDocument.js new file mode 100644 index 00000000..0f4eaa71 --- /dev/null +++ b/lib/gcli/test/mockDocument.js @@ -0,0 +1,45 @@ +/* + * Copyright 2012, Mozilla Foundation and contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +var nodetype = require('../types/node'); +var jsdom = require('jsdom').jsdom; + +var origDoc; + +/** + * Registration and de-registration. + */ +exports.setup = function() { + var document = jsdom('' + + '' + + '' + + ' ' + + ' ' + + '' + + '' + + '
' + + '' + + ''); + + origDoc = nodetype.getDocument(); + nodetype.setDocument(document); +}; + +exports.shutdown = function() { + nodetype.setDocument(origDoc); +}; diff --git a/lib/gcli/test/testCli2.js b/lib/gcli/test/testCli2.js index 3b242336..110ca699 100644 --- a/lib/gcli/test/testCli2.js +++ b/lib/gcli/test/testCli2.js @@ -18,18 +18,6 @@ var helpers = require('./helpers'); -var nodetype = require('../types/node'); - -exports.setup = function(options) { - if (options.window) { - nodetype.setDocument(options.window.document); - } -}; - -exports.shutdown = function(options) { - nodetype.unsetDocument(); -}; - exports.testSingleString = function(options) { return helpers.audit(options, [ { diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index e41204b3..e09fd127 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -18,27 +18,6 @@ var assert = require('../testharness/assert'); var helpers = require('./helpers'); -var nodetype = require('../types/node'); - -var mockBody = { - style: {} -}; - -var mockEmptyNodeList = { - length: 0, - item: function() { return null; } -}; - -var mockRootNodeList = { - length: 1, - item: function(i) { return mockBody; } -}; - -var mockDoc = { - querySelectorAll: function(css) { - return (css === ':root') ? mockRootNodeList : mockEmptyNodeList; - } -}; exports.testParamGroup = function(options) { var tsg = options.requisition.system.commands.get('tsg'); @@ -394,9 +373,6 @@ exports.testExecScript = function(options) { }; exports.testExecNode = function(options) { - var origDoc = nodetype.getDocument(); - nodetype.setDocument(mockDoc); - return helpers.audit(options, [ { skipIf: options.isNoDom || options.isRemote, @@ -413,19 +389,16 @@ exports.testExecNode = function(options) { args: { command: { name: 'tse' }, node: { - value: mockBody, arg: ' :root', status: 'VALID', message: '' }, nodes: { - value: mockEmptyNodeList, arg: '', status: 'VALID', message: '' }, nodes2: { - value: mockEmptyNodeList, arg: '', status: 'VALID', message: '' @@ -435,8 +408,10 @@ exports.testExecNode = function(options) { exec: { output: /^Exec: tse/ }, - post: function() { - nodetype.setDocument(origDoc); + post: function(output) { + assert.is(output.data.args.node, ':root', 'node should be :root'); + assert.is(output.data.args.nodes, undefined, 'node should be undefined'); + assert.is(output.data.args.nodes2, undefined, 'node should be undefined'); } } ]); diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index 46d43972..048393b4 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -18,17 +18,6 @@ var assert = require('../testharness/assert'); var helpers = require('./helpers'); -var nodetype = require('../types/node'); - -exports.setup = function(options) { - if (options.window) { - nodetype.setDocument(options.window.document); - } -}; - -exports.shutdown = function(options) { - nodetype.unsetDocument(); -}; exports.testNode = function(options) { return helpers.audit(options, [ diff --git a/lib/gcli/test/testTypes.js b/lib/gcli/test/testTypes.js index 62cac03e..60b6b553 100644 --- a/lib/gcli/test/testTypes.js +++ b/lib/gcli/test/testTypes.js @@ -19,17 +19,6 @@ var assert = require('../testharness/assert'); var util = require('../util/util'); var Promise = require('../util/promise').Promise; -var nodetype = require('../types/node'); - -exports.setup = function(options) { - if (options.window) { - nodetype.setDocument(options.window.document); - } -}; - -exports.shutdown = function(options) { - nodetype.unsetDocument(); -}; function forEachType(options, typeSpec, callback) { var types = options.requisition.system.types; diff --git a/web/gcli/test/mockDocument.js b/web/gcli/test/mockDocument.js new file mode 100644 index 00000000..9c82ca5c --- /dev/null +++ b/web/gcli/test/mockDocument.js @@ -0,0 +1,26 @@ +/* + * Copyright 2012, Mozilla Foundation and contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +/** + * Registration and de-registration. + */ +exports.setup = function() { +}; + +exports.shutdown = function() { +}; From b5d29526588c24556bb141e091f62b2923ccfda1 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 11:23:02 +0000 Subject: [PATCH 10/63] runat-1128988: Improve error handling Better error message, and make sure we don't swallow the promise rejection. Signed-off-by: Joe Walker --- lib/gcli/test/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/gcli/test/index.js b/lib/gcli/test/index.js index b49387b2..7480c66f 100644 --- a/lib/gcli/test/index.js +++ b/lib/gcli/test/index.js @@ -103,7 +103,7 @@ exports.run = function(terminal, isRemote) { examiner.reset(); setMocks(options, true).then(function() { - examiner.run(options).then(function() { + return examiner.run(options).then(function() { var name = options.isPhantomjs ? '\nPhantomJS' : 'Browser'; console.log(examiner.detailedResultLog(name)); @@ -115,8 +115,7 @@ exports.run = function(terminal, isRemote) { closeIfPhantomJs(); }); }); - }); - + }, util.errorHandler); }; /** @@ -126,7 +125,7 @@ function setMocks(options, state) { var command = 'mocks ' + (state ? 'on' : 'off'); return options.requisition.updateExec(command).then(function(data) { if (data.error) { - throw new Error('Failed to turn mocks on'); + throw new Error('Failed to toggle mocks'); } }); } From e289f808d38ae204e61bb712f14480e53634a370 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 11:24:12 +0000 Subject: [PATCH 11/63] runat-1128988: Mock 'document' better (Part 2) For the remote case, we're calling "mocks on" on the server, but we still need to register the mockCommand converters on the client. Signed-off-by: Joe Walker --- lib/gcli/test/index.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/gcli/test/index.js b/lib/gcli/test/index.js index 7480c66f..18eb8553 100644 --- a/lib/gcli/test/index.js +++ b/lib/gcli/test/index.js @@ -22,6 +22,7 @@ var KeyEvent = require('../util/util').KeyEvent; var test = require('../commands/test'); var helpers = require('./helpers'); +var mockCommands = require('./mockCommands'); var createTerminalAutomator = require('./automators/terminal').createTerminalAutomator; require('./suite'); @@ -124,6 +125,20 @@ exports.run = function(terminal, isRemote) { function setMocks(options, state) { var command = 'mocks ' + (state ? 'on' : 'off'); return options.requisition.updateExec(command).then(function(data) { + // We're calling "mocks on" on the server, but we still need to + // register the mockCommand converters on the client + var requiredConverters = mockCommands.items.filter(function(item) { + return item.item === 'converter'; + }); + + if (state) { + options.requisition.system.addItems(requiredConverters); + } + else { + options.requisition.system.removeItems(requiredConverters); + + } + if (data.error) { throw new Error('Failed to toggle mocks'); } From 06023e359f877631cc307aeae46bf2184c5fd467 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 11:31:47 +0000 Subject: [PATCH 12/63] runat-1128988: Remove isNoDom checks The majority of the isNoDom checks are now irrelevant because there is always a DOM that works for us to test against. Signed-off-by: Joe Walker --- lib/gcli/test/testCli1.js | 1 - lib/gcli/test/testCli2.js | 1 - lib/gcli/test/testCompletion2.js | 1 - lib/gcli/test/testIntro.js | 1 - lib/gcli/test/testNode.js | 1 - lib/gcli/test/testPref2.js | 4 ++-- lib/gcli/test/testResource.js | 9 --------- lib/gcli/test/testTypes.js | 5 ----- 8 files changed, 2 insertions(+), 21 deletions(-) diff --git a/lib/gcli/test/testCli1.js b/lib/gcli/test/testCli1.js index a1ca21a5..4ddd34b5 100644 --- a/lib/gcli/test/testCli1.js +++ b/lib/gcli/test/testCli1.js @@ -244,7 +244,6 @@ exports.testTsv = function(options) { } }, { - skipRemainingIf: options.isNoDom, name: '|tsv option', setup: function() { return helpers.setInput(options, 'tsv option', 0); diff --git a/lib/gcli/test/testCli2.js b/lib/gcli/test/testCli2.js index 110ca699..648d3806 100644 --- a/lib/gcli/test/testCli2.js +++ b/lib/gcli/test/testCli2.js @@ -340,7 +340,6 @@ exports.testSingleFloat = function(options) { } }, { - skipRemainingIf: options.isNoDom, name: 'tsf x (cursor=4)', setup: function() { return helpers.setInput(options, 'tsf x', 4); diff --git a/lib/gcli/test/testCompletion2.js b/lib/gcli/test/testCompletion2.js index 1bb9168b..bd487fe8 100644 --- a/lib/gcli/test/testCompletion2.js +++ b/lib/gcli/test/testCompletion2.js @@ -146,7 +146,6 @@ exports.testNoTab = function(options) { } }, { - skipIf: options.isNoDom, name: '', setup: function() { // Doing it this way avoids clearing the input buffer diff --git a/lib/gcli/test/testIntro.js b/lib/gcli/test/testIntro.js index 025c5099..72e092e8 100644 --- a/lib/gcli/test/testIntro.js +++ b/lib/gcli/test/testIntro.js @@ -43,7 +43,6 @@ exports.testIntroStatus = function(options) { }, { setup: 'intro', - skipIf: options.isNoDom, check: { typed: 'intro', markup: 'VVVVV', diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index 048393b4..c96d681e 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -22,7 +22,6 @@ var helpers = require('./helpers'); exports.testNode = function(options) { return helpers.audit(options, [ { - skipRemainingIf: options.isNoDom, setup: 'tse ', check: { input: 'tse ', diff --git a/lib/gcli/test/testPref2.js b/lib/gcli/test/testPref2.js index 79b8af37..5975dc5e 100644 --- a/lib/gcli/test/testPref2.js +++ b/lib/gcli/test/testPref2.js @@ -45,7 +45,6 @@ exports.testPrefExec = function(options) { } }, { - skipRemainingIf: options.isNoDom, setup: 'pref set tempNumber 4', check: { input: 'pref set tempNumber 4', @@ -94,7 +93,8 @@ exports.testPrefExec = function(options) { }, { skipRemainingIf: function commandPrefListMissing() { - return options.requisition.system.commands.get('pref list') == null; + return options.isNoDom || + options.requisition.system.commands.get('pref list') == null; }, setup: 'pref list tempNum', check: { diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index 4148d637..1e96bd79 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -89,10 +89,6 @@ exports.testStylePredictions = function(options) { }; exports.testAllPredictions2 = function(options) { - if (options.isNoDom) { - assert.log('Skipping checks due to nodom document.stylsheets support.'); - return; - } var types = options.requisition.system.types; var scriptRes = types.createType({ name: 'resource', include: 'text/javascript' }); @@ -110,11 +106,6 @@ exports.testAllPredictions2 = function(options) { }; exports.testAllPredictions3 = function(options) { - if (options.isNoDom) { - assert.log('Skipping checks due to nodom document.stylsheets support.'); - return; - } - var types = options.requisition.system.types; var res1 = types.createType({ name: 'resource' }); return res1.getLookup().then(function(options1) { diff --git a/lib/gcli/test/testTypes.js b/lib/gcli/test/testTypes.js index 60b6b553..2fef0fc6 100644 --- a/lib/gcli/test/testTypes.js +++ b/lib/gcli/test/testTypes.js @@ -62,11 +62,6 @@ function forEachType(options, typeSpec, callback) { } exports.testDefault = function(options) { - if (options.isNoDom) { - assert.log('Skipping tests due to issues with resource type.'); - return; - } - return forEachType(options, {}, function(type) { var context = options.requisition.executionContext; var blank = type.getBlank(context).value; From 709687c54ae6a2e2a86b9f2f8d9e2b55eb30614f Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 14:06:01 +0000 Subject: [PATCH 13/63] runat-1128988: Fix 'node' so it works properly remotely This is mostly tweaks to tests. The node type doesn't call createEmptyNodeList where it doesn't need to, and the NodeList type now sets up the ability to convert back to a string properly so the remote tests can at least prove that the value (which isn't available remotely) could be stringified into something expected. Signed-off-by: Joe Walker --- lib/gcli/test/testExec.js | 4 +-- lib/gcli/test/testNode.js | 60 ++++++++++++++++++--------------------- lib/gcli/types/node.js | 4 +-- 3 files changed, 30 insertions(+), 38 deletions(-) diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index e09fd127..ff2a6932 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -410,8 +410,8 @@ exports.testExecNode = function(options) { }, post: function(output) { assert.is(output.data.args.node, ':root', 'node should be :root'); - assert.is(output.data.args.nodes, undefined, 'node should be undefined'); - assert.is(output.data.args.nodes2, undefined, 'node should be undefined'); + assert.is(output.data.args.nodes, 'Error', 'nodes should be Error'); + assert.is(output.data.args.nodes2, 'Error', 'nodes2 should be Error'); } } ]); diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index c96d681e..7643f344 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -129,8 +129,6 @@ exports.testNode = function(options) { }; exports.testNodeDom = function(options) { - var requisition = options.requisition; - return helpers.audit(options, [ { skipRemainingIf: options.isNoDom, @@ -170,10 +168,10 @@ exports.testNodeDom = function(options) { { skipIf: options.isRemote, // arg values are unavailable remotely setup: 'tse :root ', + exec: { + }, post: function(output) { - assert.is(requisition.getAssignment('node').value.tagName, - 'HTML', - 'root id'); + assert.is(output.args.node.tagName, 'HTML', ':root tagName'); } }, { @@ -202,11 +200,8 @@ exports.testNodeDom = function(options) { }; exports.testNodes = function(options) { - var requisition = options.requisition; - return helpers.audit(options, [ { - skipRemainingIf: options.isNoDom, setup: 'tse :root --nodes *', check: { input: 'tse :root --nodes *', @@ -221,10 +216,18 @@ exports.testNodes = function(options) { nodes2: { status: 'VALID' } } }, - post: function() { - assert.is(requisition.getAssignment('node').value.tagName, - 'HTML', - '#gcli-input id'); + exec: { + }, + post: function(output) { + if (!options.isRemote) { + assert.is(output.args.node.tagName, 'HTML', ':root tagName'); + assert.ok(output.args.nodes.length > 3, 'nodes length'); + assert.is(output.args.nodes2.length, 0, 'nodes2 length'); + } + + assert.is(output.data.args.node, ':root', 'node data'); + assert.is(output.data.args.nodes, '*', 'nodes data'); + assert.is(output.data.args.nodes2, 'Error', 'nodes2 data'); } }, { @@ -243,10 +246,18 @@ exports.testNodes = function(options) { nodes2: { arg: ' --nodes2 div', status: 'VALID' } } }, - post: function() { - assert.is(requisition.getAssignment('node').value.tagName, - 'HTML', - 'root id'); + exec: { + }, + post: function(output) { + if (!options.isRemote) { + assert.is(output.args.node.tagName, 'HTML', ':root tagName'); + assert.is(output.args.nodes.length, 0, 'nodes length'); + assert.is(output.args.nodes2[0].tagName, 'DIV', 'div tagName'); + } + + assert.is(output.data.args.node, ':root', 'node data'); + assert.is(output.data.args.nodes, 'Error', 'nodes data'); + assert.is(output.data.args.nodes2, 'div', 'nodes2 data'); } }, { @@ -273,13 +284,6 @@ exports.testNodes = function(options) { }, nodes2: { arg: '', status: 'VALID', message: '' } } - }, - post: function() { - /* - assert.is(requisition.getAssignment('nodes2').value.constructor.name, - 'NodeList', - '#gcli-input id'); - */ } }, { @@ -301,16 +305,6 @@ exports.testNodes = function(options) { nodes: { arg: '', status: 'VALID', message: '' }, nodes2: { arg: ' --nodes2 ffff', status: 'VALID', message: '' } } - }, - post: function() { - /* - assert.is(requisition.getAssignment('nodes').value.constructor.name, - 'NodeList', - '#gcli-input id'); - assert.is(requisition.getAssignment('nodes2').value.constructor.name, - 'NodeList', - '#gcli-input id'); - */ } }, ]); diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index 51825b4a..949636bb 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -114,7 +114,6 @@ exports.items = [ if (arg.text === '') { reply = new Conversion(undefined, arg, Status.INCOMPLETE); - reply.matches = util.createEmptyNodeList(doc); } else { var nodes; @@ -192,7 +191,6 @@ exports.items = [ try { if (arg.text === '') { reply = new Conversion(undefined, arg, Status.INCOMPLETE); - reply.matches = util.createEmptyNodeList(doc); } else { var nodes = doc.querySelectorAll(arg.text); @@ -202,6 +200,7 @@ exports.items = [ l10n.lookup('nodeParseNone')); } else { + nodes.__gcliQuery = arg.text; reply = new Conversion(nodes, arg, Status.VALID, ''); } @@ -211,7 +210,6 @@ exports.items = [ catch (ex) { reply = new Conversion(undefined, arg, Status.ERROR, l10n.lookup('nodeParseSyntax')); - reply.matches = util.createEmptyNodeList(doc); } return Promise.resolve(reply); From 30b612e8722c879ed2e785a64f482867d5c5115e Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 5 Mar 2015 15:02:37 +0000 Subject: [PATCH 14/63] runat-1128988: Remove isNoDom Several tests were skipped when there wasn't a DOM to test against. This removes all those test skips. Given the recent fixes, most of the skips could just be removed. Some of the skips are not down to weaknesses in jsdom, but in NodeJS or the requisitionAutomator which is used only in NodeJS, so some of this is s/isNoDom/isNode. Some of the problems were confusion as to an id in the document that we were testing against. #gcli-input was removed some time ago, so we're now using #gcli-root which still exists. Finally I added a trivial converter for the 'pref list' command so it would work on the command line, and be tested properly. Signed-off-by: Joe Walker --- lib/gcli/commands/preflist.js | 20 ++++++++++++++++++-- lib/gcli/commands/server/firefox.js | 2 +- lib/gcli/commands/test.js | 1 - lib/gcli/test/helpers.js | 26 +++++++++++++------------- lib/gcli/test/mockDocument.js | 4 ++-- lib/gcli/test/testCli2.js | 19 +++++-------------- lib/gcli/test/testDate.js | 2 +- lib/gcli/test/testExec.js | 2 +- lib/gcli/test/testHelp.js | 7 +++---- lib/gcli/test/testJs.js | 8 ++++---- lib/gcli/test/testNode.js | 1 - lib/gcli/test/testPref2.js | 3 +-- lib/gcli/test/testResource.js | 6 +++--- 13 files changed, 52 insertions(+), 49 deletions(-) diff --git a/lib/gcli/commands/preflist.js b/lib/gcli/commands/preflist.js index b6b1fadc..9652b03e 100644 --- a/lib/gcli/commands/preflist.js +++ b/lib/gcli/commands/preflist.js @@ -22,7 +22,7 @@ var Promise = require('../util/promise').Promise; /** * Format a list of settings for display */ -var prefsData = { +var prefsViewConverter = { item: 'converter', from: 'prefsData', to: 'view', @@ -97,6 +97,22 @@ var prefsData = { } }; +/** + * Format a list of settings for display + */ +var prefsStringConverter = { + item: 'converter', + from: 'prefsData', + to: 'string', + exec: function(prefsData, conversionContext) { + var reply = ''; + prefsData.settings.forEach(function(setting) { + reply += setting.name + ' -> ' + setting.value + '\n'; + }); + return reply; + } +}; + /** * 'pref list' command */ @@ -194,4 +210,4 @@ PrefList.prototype.onSetClick = function(ev) { this.conversionContext.update(typed); }; -exports.items = [ prefsData, prefList ]; +exports.items = [ prefsViewConverter, prefsStringConverter, prefList ]; diff --git a/lib/gcli/commands/server/firefox.js b/lib/gcli/commands/server/firefox.js index f7fc0ce0..7a802ded 100644 --- a/lib/gcli/commands/server/firefox.js +++ b/lib/gcli/commands/server/firefox.js @@ -133,7 +133,7 @@ function createCommonJsToJsTestFilter() { '\n' + 'var exports = {};\n' + '\n' + - 'var TEST_URI = "data:text/html;charset=utf-8,

gcli-' + name + '

";\n' + + 'var TEST_URI = "data:text/html;charset=utf-8,

gcli-' + name + '

";\n' + '\n' + 'function test() {\n' + ' return Task.spawn(*function() {\n' + diff --git a/lib/gcli/commands/test.js b/lib/gcli/commands/test.js index f117e434..6f78a376 100644 --- a/lib/gcli/commands/test.js +++ b/lib/gcli/commands/test.js @@ -74,7 +74,6 @@ exports.items = [ process.title.indexOf('node') != -1), isFirefox: false, isPhantomjs: false, - isNoDom: true, requisition: new Requisition(context.system) }; options.automator = createRequisitionAutomator(options.requisition); diff --git a/lib/gcli/test/helpers.js b/lib/gcli/test/helpers.js index c4529b35..0b4d2f0a 100644 --- a/lib/gcli/test/helpers.js +++ b/lib/gcli/test/helpers.js @@ -380,15 +380,15 @@ helpers._check = function(options, name, checks) { var outstanding = []; var suffix = name ? ' (for \'' + name + '\')' : ''; - if (!options.isNoDom && 'input' in checks) { + if (!options.isNode && 'input' in checks) { assert.is(helpers._actual.input(options), checks.input, 'input' + suffix); } - if (!options.isNoDom && 'cursor' in checks) { + if (!options.isNode && 'cursor' in checks) { assert.is(helpers._actual.cursor(options), checks.cursor, 'cursor' + suffix); } - if (!options.isNoDom && 'current' in checks) { + if (!options.isNode && 'current' in checks) { assert.is(helpers._actual.current(options), checks.current, 'current' + suffix); } @@ -396,18 +396,18 @@ helpers._check = function(options, name, checks) { assert.is(helpers._actual.status(options), checks.status, 'status' + suffix); } - if (!options.isNoDom && 'markup' in checks) { + if (!options.isNode && 'markup' in checks) { assert.is(helpers._actual.markup(options), checks.markup, 'markup' + suffix); } - if (!options.isNoDom && 'hints' in checks) { + if (!options.isNode && 'hints' in checks) { var hintCheck = function(actualHints) { assert.is(actualHints, checks.hints, 'hints' + suffix); }; outstanding.push(helpers._actual.hints(options).then(hintCheck)); } - if (!options.isNoDom && 'predictions' in checks) { + if (!options.isNode && 'predictions' in checks) { var predictionsCheck = function(actualPredictions) { helpers.arrayIs(actualPredictions, checks.predictions, @@ -416,7 +416,7 @@ helpers._check = function(options, name, checks) { outstanding.push(helpers._actual.predictions(options).then(predictionsCheck)); } - if (!options.isNoDom && 'predictionsContains' in checks) { + if (!options.isNode && 'predictionsContains' in checks) { var containsCheck = function(actualPredictions) { checks.predictionsContains.forEach(function(prediction) { var index = actualPredictions.indexOf(prediction); @@ -434,26 +434,26 @@ helpers._check = function(options, name, checks) { } /* TODO: Fix this - if (!options.isNoDom && 'tooltipState' in checks) { + if (!options.isNode && 'tooltipState' in checks) { assert.is(helpers._actual.tooltipState(options), checks.tooltipState, 'tooltipState' + suffix); } */ - if (!options.isNoDom && 'outputState' in checks) { + if (!options.isNode && 'outputState' in checks) { assert.is(helpers._actual.outputState(options), checks.outputState, 'outputState' + suffix); } - if (!options.isNoDom && 'options' in checks) { + if (!options.isNode && 'options' in checks) { helpers.arrayIs(helpers._actual.options(options), checks.options, 'options' + suffix); } - if (!options.isNoDom && 'error' in checks) { + if (!options.isNode && 'error' in checks) { assert.is(helpers._actual.message(options), checks.error, 'error' + suffix); } @@ -515,7 +515,7 @@ helpers._check = function(options, name, checks) { 'arg.' + paramName + '.status' + suffix); } - if (!options.isNoDom && 'message' in check) { + if (!options.isNode && 'message' in check) { if (typeof check.message.test === 'function') { assert.ok(check.message.test(assignment.message), 'arg.' + paramName + '.message' + suffix); @@ -573,7 +573,7 @@ helpers._exec = function(options, name, expected) { var context = requisition.conversionContext; var convertPromise; - if (options.isNoDom) { + if (options.isNode) { convertPromise = output.convert('string', context); } else { diff --git a/lib/gcli/test/mockDocument.js b/lib/gcli/test/mockDocument.js index 0f4eaa71..d9da00d3 100644 --- a/lib/gcli/test/mockDocument.js +++ b/lib/gcli/test/mockDocument.js @@ -28,11 +28,11 @@ exports.setup = function() { var document = jsdom('' + '' + '' + - ' ' + + ' ' + ' ' + '' + '' + - '
' + + '
' + '' + ''); diff --git a/lib/gcli/test/testCli2.js b/lib/gcli/test/testCli2.js index 648d3806..bed7b4fe 100644 --- a/lib/gcli/test/testCli2.js +++ b/lib/gcli/test/testCli2.js @@ -369,21 +369,14 @@ exports.testSingleFloat = function(options) { }; exports.testElementWeb = function(options) { - var inputElement = options.isNoDom ? - null : - options.window.document.getElementById('gcli-input'); - return helpers.audit(options, [ { - skipIf: function gcliInputElementExists() { - return inputElement == null; - }, - setup: 'tse #gcli-input', + setup: 'tse #gcli-root', check: { - input: 'tse #gcli-input', + input: 'tse #gcli-root', hints: ' [options]', - markup: 'VVVVVVVVVVVVVVV', - cursor: 15, + markup: 'VVVVVVVVVVVVVV', + cursor: 14, current: 'node', status: 'VALID', predictions: [ ], @@ -391,8 +384,7 @@ exports.testElementWeb = function(options) { args: { command: { name: 'tse' }, node: { - value: inputElement, - arg: ' #gcli-input', + arg: ' #gcli-root', status: 'VALID', message: '' }, @@ -407,7 +399,6 @@ exports.testElementWeb = function(options) { exports.testElement = function(options) { return helpers.audit(options, [ { - skipRemainingIf: options.isNoDom, setup: 'tse', check: { input: 'tse', diff --git a/lib/gcli/test/testDate.js b/lib/gcli/test/testDate.js index f2fdc47b..540c5666 100644 --- a/lib/gcli/test/testDate.js +++ b/lib/gcli/test/testDate.js @@ -240,7 +240,7 @@ exports.testIncrDecr = function(options) { return helpers.audit(options, [ { // createRequisitionAutomator doesn't fake UP/DOWN well enough - skipRemainingIf: options.isNoDom, + skipRemainingIf: options.isNode, setup: 'tsdate 2001-01-01', check: { input: 'tsdate 2001-01-02', diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index ff2a6932..9fa06036 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -375,7 +375,7 @@ exports.testExecScript = function(options) { exports.testExecNode = function(options) { return helpers.audit(options, [ { - skipIf: options.isNoDom || options.isRemote, + skipIf: options.isRemote, setup: 'tse :root', check: { input: 'tse :root', diff --git a/lib/gcli/test/testHelp.js b/lib/gcli/test/testHelp.js index 18a0c093..22311a88 100644 --- a/lib/gcli/test/testHelp.js +++ b/lib/gcli/test/testHelp.js @@ -77,8 +77,7 @@ exports.testHelpExec = function(options) { return helpers.audit(options, [ { skipRemainingIf: function commandHelpMissing() { - return options.isNoDom || - options.requisition.system.commands.get('help') == null; + return options.requisition.system.commands.get('help') == null; }, setup: 'help', check: { @@ -98,7 +97,7 @@ exports.testHelpExec = function(options) { args: { search: { value: 'nomatch' } } }, exec: { - output: /No commands starting with 'nomatch'$/ + output: /No commands starting with 'nomatch'/ } }, { @@ -121,7 +120,7 @@ exports.testHelpExec = function(options) { args: { search: { value: 'a b' } } }, exec: { - output: /No commands starting with 'a b'$/ + output: /No commands starting with 'a b'/ } }, { diff --git a/lib/gcli/test/testJs.js b/lib/gcli/test/testJs.js index 09d4e85a..a48d1ad4 100644 --- a/lib/gcli/test/testJs.js +++ b/lib/gcli/test/testJs.js @@ -23,7 +23,7 @@ var javascript = require('../types/javascript'); var tempWindow; exports.setup = function(options) { - if (options.isNoDom) { + if (options.isNode) { return; } @@ -40,7 +40,7 @@ exports.setup = function(options) { }; exports.shutdown = function(options) { - if (options.isNoDom) { + if (options.isNode) { return; } @@ -50,7 +50,7 @@ exports.shutdown = function(options) { }; function jsTestAllowed(options) { - return options.isRemote || options.isNoDom || + return options.isRemote || options.isNode || options.requisition.system.commands.get('{') == null; } @@ -324,7 +324,7 @@ exports.testDocument = function(options) { }; exports.testDonteval = function(options) { - if (!options.isNoDom) { + if (!options.isNode) { // nodom causes an eval here, maybe that's node/v8? assert.ok('donteval' in options.window, 'donteval exists'); } diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index 7643f344..e56e8509 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -131,7 +131,6 @@ exports.testNode = function(options) { exports.testNodeDom = function(options) { return helpers.audit(options, [ { - skipRemainingIf: options.isNoDom, setup: 'tse :root', check: { input: 'tse :root', diff --git a/lib/gcli/test/testPref2.js b/lib/gcli/test/testPref2.js index 5975dc5e..7a1e827c 100644 --- a/lib/gcli/test/testPref2.js +++ b/lib/gcli/test/testPref2.js @@ -93,8 +93,7 @@ exports.testPrefExec = function(options) { }, { skipRemainingIf: function commandPrefListMissing() { - return options.isNoDom || - options.requisition.system.commands.get('pref list') == null; + return options.requisition.system.commands.get('pref list') == null; }, setup: 'pref list tempNum', check: { diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index 1e96bd79..1c31f9cb 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -39,7 +39,7 @@ exports.shutdown = function(options) { }; exports.testAllPredictions1 = function(options) { - if (options.isFirefox || options.isNoDom) { + if (options.isFirefox || options.isNode) { assert.log('Skipping checks due to firefox document.stylsheets support.'); return; } @@ -55,7 +55,7 @@ exports.testAllPredictions1 = function(options) { }; exports.testScriptPredictions = function(options) { - if (options.isFirefox || options.isNoDom) { + if (options.isFirefox || options.isNode) { assert.log('Skipping checks due to firefox document.stylsheets support.'); return; } @@ -72,7 +72,7 @@ exports.testScriptPredictions = function(options) { }; exports.testStylePredictions = function(options) { - if (options.isFirefox || options.isNoDom) { + if (options.isFirefox || options.isNode) { assert.log('Skipping checks due to firefox document.stylsheets support.'); return; } From 0e84444cfccd80f2f19d646776873bdf3cdbc149 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 6 Mar 2015 16:28:26 +0000 Subject: [PATCH 15/63] runat-1128988: Consistently return a promise from loadModule() Fix for https://github.com/joewalker/gecko-dev/commit/2a963b09ad8b77d39378c5d351c591831f153bd7 Signed-off-by: Joe Walker --- lib/gcli/system.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 56716216..9d22ffbb 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -132,6 +132,8 @@ exports.createSystem = function(options) { catch (ex) { console.error('Failed to load module ' + name + ': ' + ex); console.error(ex.stack); + + return Promise.resolve(); } }; From 33d8e7ca437aa85e180925240dbbba425bbea078 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 6 Mar 2015 16:29:24 +0000 Subject: [PATCH 16/63] runat-1128988: Clarify comment Fix for https://github.com/joewalker/gcli/commit/bc82016ee13968f73216b272a4516ef5fd98c201#commitcomment-10060854 Signed-off-by: Joe Walker --- docs/writing-types.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/writing-types.md b/docs/writing-types.md index ee9232d6..ed2f7543 100644 --- a/docs/writing-types.md +++ b/docs/writing-types.md @@ -49,8 +49,9 @@ All types must inherit from Type and have the following methods: */ name: 'example', -In addition, defining the following functions can be helpful, although Type +In addition, defining the following function can be helpful, although Type contains default implementations: + * nudge(value, by) Type, Conversion and Status are all declared by commands.js. From d374a91e232d039ca6ab907fd0beaf187d273948 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 10 Mar 2015 14:40:03 +0000 Subject: [PATCH 17/63] runat-1128988: Replace document/globalObject with windowHolder Previously each time we navigated to a new page we'd have to update the document/global in GCLI. A windowHolder which uses a getter property to dynamically return the correct document/global is much easier to use. We're using windowHolder rather than documentHolder simply because you could confuse a documentHolder and a window, but you can't confuse a windowHolder and a document so easily. The javascript type isn't used in Firefox. It wasn't clear that the donteval test was working properly, and I didn't want to invest time to it right now, so we're skipping those tests. Signed-off-by: Joe Walker --- lib/gcli/test/mockDocument.js | 11 +++++--- lib/gcli/test/testJs.js | 48 ++++++++++++++++++++-------------- lib/gcli/test/testKeyboard1.js | 12 --------- lib/gcli/test/testResource.js | 15 ----------- lib/gcli/types/javascript.js | 37 ++++++++++++-------------- lib/gcli/types/node.js | 35 ++++++++++++------------- lib/gcli/types/resource.js | 34 +++++++++++------------- 7 files changed, 82 insertions(+), 110 deletions(-) diff --git a/lib/gcli/test/mockDocument.js b/lib/gcli/test/mockDocument.js index d9da00d3..886cee94 100644 --- a/lib/gcli/test/mockDocument.js +++ b/lib/gcli/test/mockDocument.js @@ -19,12 +19,14 @@ var nodetype = require('../types/node'); var jsdom = require('jsdom').jsdom; -var origDoc; +var origWindowHolder; /** * Registration and de-registration. */ exports.setup = function() { + origWindowHolder = nodetype.getWindowHolder(); + var document = jsdom('' + '' + '' + @@ -36,10 +38,11 @@ exports.setup = function() { '' + ''); - origDoc = nodetype.getDocument(); - nodetype.setDocument(document); + nodetype.setWindowHolder({ + window: document.defaultView, + }); }; exports.shutdown = function() { - nodetype.setDocument(origDoc); + nodetype.setWindowHolder(origWindowHolder); }; diff --git a/lib/gcli/test/testJs.js b/lib/gcli/test/testJs.js index a48d1ad4..47e761b9 100644 --- a/lib/gcli/test/testJs.js +++ b/lib/gcli/test/testJs.js @@ -20,37 +20,45 @@ var assert = require('../testharness/assert'); var helpers = require('./helpers'); var javascript = require('../types/javascript'); -var tempWindow; +// Store the original windowHolder +var tempWindowHolder; + +// Mock windowHolder to check that we're not trespassing on 'donteval' +var mockWindowHolder = { + window: { + document: {} + }, +}; +mockWindowHolder.window = mockWindowHolder; +Object.defineProperty(mockWindowHolder.window, 'donteval', { + get: function() { + assert.ok(false, 'donteval should not be used'); + return { cant: '', touch: '', 'this': '' }; + }, + enumerable: true, + configurable: true +}); exports.setup = function(options) { - if (options.isNode) { + if (!jsTestAllowed(options)) { return; } - tempWindow = javascript.getGlobalObject(); - Object.defineProperty(options.window, 'donteval', { - get: function() { - assert.ok(false, 'donteval should not be used'); - return { cant: '', touch: '', 'this': '' }; - }, - enumerable: true, - configurable : true - }); - javascript.setGlobalObject(options.window); + tempWindowHolder = javascript.getWindowHolder(); + javascript.setWindowHolder(mockWindowHolder); }; exports.shutdown = function(options) { - if (options.isNode) { + if (!jsTestAllowed(options)) { return; } - javascript.setGlobalObject(tempWindow); - tempWindow = undefined; - delete options.window.donteval; + javascript.setWindowHolder(tempWindowHolder); }; function jsTestAllowed(options) { - return options.isRemote || options.isNode || + return options.isRemote || // We're directly accessing the javascript type + options.isNode || options.requisition.system.commands.get('{') == null; } @@ -324,14 +332,14 @@ exports.testDocument = function(options) { }; exports.testDonteval = function(options) { - if (!options.isNode) { + if (jsTestAllowed(options)) { // nodom causes an eval here, maybe that's node/v8? - assert.ok('donteval' in options.window, 'donteval exists'); + assert.ok('donteval' in mockWindowHolder.window, 'donteval exists'); } return helpers.audit(options, [ { - skipRemainingIf: jsTestAllowed, + skipRemainingIf: true, // Commented out until we fix non-enumerable props setup: '{ don', check: { input: '{ don', diff --git a/lib/gcli/test/testKeyboard1.js b/lib/gcli/test/testKeyboard1.js index 2d2e1986..e6580c97 100644 --- a/lib/gcli/test/testKeyboard1.js +++ b/lib/gcli/test/testKeyboard1.js @@ -19,18 +19,6 @@ var javascript = require('../types/javascript'); var helpers = require('./helpers'); -var tempWindow; - -exports.setup = function(options) { - tempWindow = javascript.getGlobalObject(); - javascript.setGlobalObject(options.window); -}; - -exports.shutdown = function(options) { - javascript.setGlobalObject(tempWindow); - tempWindow = undefined; -}; - exports.testSimple = function(options) { return helpers.audit(options, [ { diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index 1c31f9cb..e3776dcb 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -23,21 +23,6 @@ var util = require('../util/util'); var resource = require('../types/resource'); var Status = require('../types/types').Status; - -var tempDocument; - -exports.setup = function(options) { - tempDocument = resource.getDocument(); - if (options.window) { - resource.setDocument(options.window.document); - } -}; - -exports.shutdown = function(options) { - resource.setDocument(tempDocument); - tempDocument = undefined; -}; - exports.testAllPredictions1 = function(options) { if (options.isFirefox || options.isNode) { assert.log('Skipping checks due to firefox document.stylsheets support.'); diff --git a/lib/gcli/types/javascript.js b/lib/gcli/types/javascript.js index a62d49cc..d3318385 100644 --- a/lib/gcli/types/javascript.js +++ b/lib/gcli/types/javascript.js @@ -24,37 +24,32 @@ var Type = require('./types').Type; var Status = require('./types').Status; /** - * The object against which we complete, which is usually 'window' if it exists - * but could be something else in non-web-content environments. + * windowHolder is a way to get at a window/document/etc. The page that we're + * looking at could change, so a call to windowHolder.window could change + * between calls from the event loop. */ -var globalObject; -if (typeof window !== 'undefined') { - globalObject = window; +var windowHolder; +if (typeof document !== 'undefined') { + windowHolder = { + window: document.defaultView, + }; } /** - * Setter for the object against which JavaScript completions happen - */ -exports.setGlobalObject = function(obj) { - globalObject = obj; -}; - -/** - * Getter for the object against which JavaScript completions happen, for use - * in testing + * Setter for the document that contains the nodes we're matching */ -exports.getGlobalObject = function() { - return globalObject; +exports.setWindowHolder = function(wh) { + windowHolder = wh; }; /** - * Remove registration of object against which JavaScript completions happen + * Getter for the document that contains the nodes we're matching + * Most for changing things back to how they were for unit testing */ -exports.unsetGlobalObject = function() { - globalObject = undefined; +exports.getWindowHolder = function() { + return windowHolder; }; - /** * 'javascript' handles scripted input */ @@ -82,7 +77,7 @@ JavascriptType.MAX_COMPLETION_MATCHES = 10; JavascriptType.prototype.parse = function(arg, context) { var typed = arg.text; - var scope = globalObject; + var scope = (windowHolder == null) ? null : windowHolder.window; // No input is undefined if (typed === '') { diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index 949636bb..cfd7a0a3 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -25,34 +25,30 @@ var Conversion = require('./types').Conversion; var BlankArgument = require('./types').BlankArgument; /** - * The object against which we complete, which is usually 'window' if it exists - * but could be something else in non-web-content environments. + * windowHolder is a way to get at a window/document/etc. The page that we're + * looking at could change, so a call to windowHolder.window could change + * between calls from the event loop. */ -var doc; +var windowHolder; if (typeof document !== 'undefined') { - doc = document; + windowHolder = { + window: document.defaultView, + }; } /** * Setter for the document that contains the nodes we're matching */ -exports.setDocument = function(document) { - doc = document; -}; - -/** - * Undo the effects of setDocument() - */ -exports.unsetDocument = function() { - doc = undefined; +exports.setWindowHolder = function(wh) { + windowHolder = wh; }; /** * Getter for the document that contains the nodes we're matching * Most for changing things back to how they were for unit testing */ -exports.getDocument = function() { - return doc; +exports.getWindowHolder = function() { + return windowHolder; }; /** @@ -60,7 +56,7 @@ exports.getDocument = function() { * NodeListType to allow terminal to tell us which nodes should be highlighted */ function onEnter(assignment) { - assignment.highlighter = new Highlighter(doc); + assignment.highlighter = new Highlighter(windowHolder.window.document); assignment.highlighter.nodelist = assignment.conversion.matches; } @@ -118,7 +114,7 @@ exports.items = [ else { var nodes; try { - nodes = doc.querySelectorAll(arg.text); + nodes = windowHolder.window.document.querySelectorAll(arg.text); if (nodes.length === 0) { reply = new Conversion(undefined, arg, Status.INCOMPLETE, l10n.lookup('nodeParseNone')); @@ -175,7 +171,8 @@ exports.items = [ }, getBlank: function(context) { - var emptyNodeList = (doc == null ? [] : util.createEmptyNodeList(doc)); + var emptyNodeList = (windowHolder == null) ? [] : + util.createEmptyNodeList(windowHolder.window.document); return new Conversion(emptyNodeList, new BlankArgument(), Status.VALID); }, @@ -193,7 +190,7 @@ exports.items = [ reply = new Conversion(undefined, arg, Status.INCOMPLETE); } else { - var nodes = doc.querySelectorAll(arg.text); + var nodes = windowHolder.window.document.querySelectorAll(arg.text); if (nodes.length === 0 && !this.allowEmpty) { reply = new Conversion(undefined, arg, Status.INCOMPLETE, diff --git a/lib/gcli/types/resource.js b/lib/gcli/types/resource.js index fc939bf8..d539418a 100644 --- a/lib/gcli/types/resource.js +++ b/lib/gcli/types/resource.js @@ -23,35 +23,30 @@ exports.clearResourceCache = function() { }; /** - * The object against which we complete, which is usually 'window' if it exists - * but could be something else in non-web-content environments. + * windowHolder is a way to get at a window/document/etc. The page that we're + * looking at could change, so a call to windowHolder.window could change + * between calls from the event loop. */ -var doc; +var windowHolder; if (typeof document !== 'undefined') { - doc = document; + windowHolder = { + window: document.defaultView, + }; } /** * Setter for the document that contains the nodes we're matching */ -exports.setDocument = function(document) { - doc = document; -}; - -/** - * Undo the effects of setDocument() - */ -exports.unsetDocument = function() { - ResourceCache.clear(); - doc = undefined; +exports.setWindowHolder = function(wh) { + windowHolder = wh; }; /** * Getter for the document that contains the nodes we're matching * Most for changing things back to how they were for unit testing */ -exports.getDocument = function() { - return doc; +exports.getWindowHolder = function() { + return windowHolder; }; /** @@ -105,10 +100,11 @@ CssResource.prototype.loadContents = function() { CssResource._getAllStyles = function() { var resources = []; - if (doc == null) { + if (windowHolder == null) { return resources; } + var doc = windowHolder.window.document; Array.prototype.forEach.call(doc.styleSheets, function(domSheet) { CssResource._getStyle(domSheet, resources); }); @@ -183,11 +179,11 @@ ScriptResource.prototype.loadContents = function() { }; ScriptResource._getAllScripts = function() { - if (doc == null) { + if (windowHolder == null) { return []; } - var scriptNodes = doc.querySelectorAll('script'); + var scriptNodes = windowHolder.window.document.querySelectorAll('script'); var resources = Array.prototype.map.call(scriptNodes, function(scriptNode) { var resource = ResourceCache.get(scriptNode); if (!resource) { From b04f95a586548afaab637d581577e737c3fc7b1f Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 13:51:34 +0000 Subject: [PATCH 18/63] runat-1128988: Pass Errors through protocol.js correctly In Output.toJson we were assuming that `JSON.stringify(new Error('x'))` would give us something useful. It actually just gives `{}`, so we manually extract the properties of a standard Error. We were using `error:true` to denote an error, but this was conflicting with a special property in protocol.js, so we're using isError instead. (Commands that fail are considered non-exceptional. Exceptions in GCLI are for when GCLI itself fails) The converters now need to cope with errors that are objects that look like Errors (but not actually errors). In the process of debugging this I refactored the code that kicks off a remote execution in system.js. Signed-off-by: Joe Walker --- lib/gcli/cli.js | 16 ++++++++++++++-- lib/gcli/converters/converters.js | 11 ++++++++++- lib/gcli/system.js | 8 +------- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js index 532ba001..161b18ce 100644 --- a/lib/gcli/cli.js +++ b/lib/gcli/cli.js @@ -2164,11 +2164,23 @@ Output.prototype.convert = function(type, conversionContext) { }; Output.prototype.toJson = function() { + // Exceptions don't stringify, so we try a bit harder + var data = this.data; + if (this.error && JSON.stringify(this.data) === '{}') { + data = { + columnNumber: data.columnNumber, + fileName: data.fileName, + lineNumber: data.lineNumber, + message: data.message, + stack: data.stack + }; + } + return { typed: this.typed, type: this.type, - data: this.data, - error: this.error + data: data, + isError: this.error }; }; diff --git a/lib/gcli/converters/converters.js b/lib/gcli/converters/converters.js index 545755a4..84cab292 100644 --- a/lib/gcli/converters/converters.js +++ b/lib/gcli/converters/converters.js @@ -99,7 +99,7 @@ var errorDomConverter = { exec: function(ex, conversionContext) { var node = util.createElement(conversionContext.document, 'p'); node.className = 'gcli-error'; - node.textContent = ex; + node.textContent = errorStringConverter.exec(ex, conversionContext); return node; } }; @@ -112,6 +112,15 @@ var errorStringConverter = { from: 'error', to: 'string', exec: function(ex, conversionContext) { + if (typeof ex === 'string') { + return ex; + } + if (ex instanceof Error) { + return '' + ex; + } + if (typeof ex.message === 'string') { + return ex.message; + } return '' + ex; } }; diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 9d22ffbb..89c79e01 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -308,15 +308,9 @@ function addLocalFunctions(specs, front) { if (!commandSpec.isParent) { commandSpec.exec = function(args, context) { var typed = (context.prefix ? context.prefix + ' ' : '') + context.typed; - return front.execute(typed).then(function(reply) { var typedData = context.typedData(reply.type, reply.data); - if (!reply.error) { - return typedData; - } - else { - throw typedData; - } + return reply.isError ? Promise.reject(typedData) : typedData; }); }; } From d62af41aa9c75fd769732120775b9b4105131d47 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 13:52:08 +0000 Subject: [PATCH 19/63] runat-1128988: Update the function to convert GCLI -> Mochitests 2 Changes: - s/*function/function*/ - Use a
rather than a

(some tests assume this) Signed-off-by: Joe Walker --- lib/gcli/commands/server/firefox.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gcli/commands/server/firefox.js b/lib/gcli/commands/server/firefox.js index 7a802ded..23eef825 100644 --- a/lib/gcli/commands/server/firefox.js +++ b/lib/gcli/commands/server/firefox.js @@ -133,10 +133,10 @@ function createCommonJsToJsTestFilter() { '\n' + 'var exports = {};\n' + '\n' + - 'var TEST_URI = "data:text/html;charset=utf-8,

gcli-' + name + '

";\n' + + 'var TEST_URI = "data:text/html;charset=utf-8,
gcli-' + name + '
";\n' + '\n' + 'function test() {\n' + - ' return Task.spawn(*function() {\n' + + ' return Task.spawn(function*() {\n' + ' let options = yield helpers.openTab(TEST_URI);\n' + ' yield helpers.openToolbar(options);\n' + ' options.requisition.system.addItems(mockCommands.items);\n' + From 271e262a1a6d283014a05ec3c06be99368da4906 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 13:52:25 +0000 Subject: [PATCH 20/63] runat-1128988: Sync helpers.js with Firefox Someone had added an assert.ok, and we protect ourselves against null DOM nodes in checking test output Signed-off-by: Joe Walker --- lib/gcli/test/helpers.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/gcli/test/helpers.js b/lib/gcli/test/helpers.js index 0b4d2f0a..264da202 100644 --- a/lib/gcli/test/helpers.js +++ b/lib/gcli/test/helpers.js @@ -578,7 +578,7 @@ helpers._exec = function(options, name, expected) { } else { convertPromise = output.convert('dom', context).then(function(node) { - return node.textContent.trim(); + return (node == null) ? '' : node.textContent.trim(); }); } @@ -791,6 +791,10 @@ helpers.audit = function(options, audits) { 'due to ' + audit.skipRemainingIf.name : ''; assert.log('Skipped ' + name + ' ' + skipReason); + + // Tests need at least one pass, fail or todo. Create a dummy pass + assert.ok(true, 'Each test requires at least one pass, fail or todo'); + return Promise.resolve(undefined); } } From 5984685dbec3bb0d1c824d187bcc6331787d31f9 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 13:58:51 +0000 Subject: [PATCH 21/63] runat-1128988: Fix misspelled attribute. Matters in an xhtml document Signed-off-by: Joe Walker --- lib/gcli/test/mockCommands.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index e965b10b..d336cd1f 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -74,7 +74,7 @@ mockCommands.items = [ '' + '' + '' + - '' + + '' + ' ${key}' + '=' + '${args[key]}' + From 0deda2a3b540c5e597c131782bba79f544b9ff3b Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 13:59:16 +0000 Subject: [PATCH 22/63] runat-1128988: Max line length = 80 chars. Wrap. Signed-off-by: Joe Walker --- lib/gcli/test/mockCommands.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index d336cd1f..955023b8 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -40,10 +40,10 @@ function createExec(name) { Object.keys(args).map(function(argName) { var value = args[argName]; var type = this.getParameterByName(argName).type; - var promise = Promise.resolve(type.stringify(value, context)).then(function(str) { + var promise = Promise.resolve(type.stringify(value, context)); + promises.push(promise.then(function(str) { return { name: argName, value: str }; - }.bind(this)); - promises.push(promise); + }.bind(this))); }.bind(this)); return Promise.all(promises).then(function(data) { From a44f58b3875570da31b742d030ec33eee6edd656 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 14:00:20 +0000 Subject: [PATCH 23/63] runat-1128988: Tests on 'node' type should access the NodeList properly Signed-off-by: Joe Walker --- lib/gcli/test/testNode.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index e56e8509..7424ebaa 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -251,7 +251,7 @@ exports.testNodes = function(options) { if (!options.isRemote) { assert.is(output.args.node.tagName, 'HTML', ':root tagName'); assert.is(output.args.nodes.length, 0, 'nodes length'); - assert.is(output.args.nodes2[0].tagName, 'DIV', 'div tagName'); + assert.is(output.args.nodes2.item(0).tagName, 'DIV', 'div tagName'); } assert.is(output.data.args.node, ':root', 'node data'); From d49e5a3bb227ab11782917427ba5848e06447eb3 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 14:02:28 +0000 Subject: [PATCH 24/63] runat-1128988: Refactor util.promiseEach The whole onFailure thing wasn't needed, and we can chain the promises onto each other rather than manually running a loop. Half the LOC. Signed-off-by: Joe Walker --- lib/gcli/util/util.js | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/lib/gcli/util/util.js b/lib/gcli/util/util.js index 4e205f4b..16fade57 100644 --- a/lib/gcli/util/util.js +++ b/lib/gcli/util/util.js @@ -255,30 +255,20 @@ exports.promiseEach = function(array, action, scope) { return Promise.resolve([]); } - return new Promise(function(resolve, reject) { - var replies = []; - - var callNext = function(index) { - var onSuccess = function(reply) { - replies[index] = reply; - - if (index + 1 >= array.length) { - resolve(replies); - } - else { - callNext(index + 1); - } - }; - - var onFailure = function(ex) { - reject(ex); - }; - - var reply = action.call(scope, array[index], index, array); - Promise.resolve(reply).then(onSuccess).catch(onFailure); - }; - - callNext(0); + var allReply = []; + var promise = Promise.resolve(); + + array.forEach(function(member, i) { + promise = promise.then(function() { + var reply = action.call(scope, member, i, array); + return Promise.resolve(reply).then(function(data) { + allReply[i] = data; + }); + }); + }); + + return promise.then(function() { + return allReply; }); }; From a96027c5d74080e3cf6ed3018d9d6456b0d0ad8a Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 12 Mar 2015 14:30:24 +0000 Subject: [PATCH 25/63] runat-1128988: 2 places to make types more robust delegate.stringify does something better than die if it gets undefined. We url.parse checks if environment.window exists without accessing it. Signed-off-by: Joe Walker --- lib/gcli/types/delegate.js | 3 +++ lib/gcli/types/url.js | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index 2352e738..29c425e0 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -96,6 +96,9 @@ exports.items = [ }, stringify: function(value, context) { + if (value == null) { + return ''; + } // remote types are client only, and we don't attempt to transfer value // objects to the client (we can't be sure the are jsonable) so it is a // bit strange to be asked to stringify a value object, however since diff --git a/lib/gcli/types/url.js b/lib/gcli/types/url.js index 8baf7431..8a782bb0 100644 --- a/lib/gcli/types/url.js +++ b/lib/gcli/types/url.js @@ -61,7 +61,7 @@ exports.items = [ }.bind(this)); // Try to create a URL with the current page as a base ref - if (context.environment.window) { + if ('window' in context.environment) { try { var base = context.environment.window.location.href; var localized = host.createUrl(arg.text, base); From 3bf3d5ee524e36691f22ff8ccfdc7298e9a0f66a Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 13 Mar 2015 12:54:38 +0000 Subject: [PATCH 26/63] runat-1128988: Remote fixes for note and delegate types Firstly the nodelist type tried to instansiate itself across the remote interface, which was never going to work. Now it uses the 'remote' type to proxy the type functions. There is a special 'blank' conversion for types. parse() is async but lots of the time we're effectively doing parse('') and making that case async spreads the async virus to lots of places that it doesn't need to be. So there's a getBlank() shortcut that is sync. In almost all cases blank means 'INCOMPLETE' with an undefined value (see [1] for the default implementation). But there are exceptions like nodelist, where blank means an empty NodeList (which could be VALID) So we add a blankIsValid property to the remote type which defaults to false, so you get the default getBlank() behavior. But for nodelists we set this to true, so we get the correct VALID / empty NodeList behavior. [1]: https://github.com/joewalker/gcli/blob/585f7b83953de5c0faeb9fb187b55ab6171c3d03/lib/gcli/types/types.js#L1007 Signed-off-by: Joe Walker --- lib/gcli/types/delegate.js | 12 ++++++++++++ lib/gcli/types/node.js | 9 +++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index 29c425e0..c58334db 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -19,6 +19,7 @@ var Promise = require('../util/promise').Promise; var Conversion = require('./types').Conversion; var Status = require('./types').Status; +var BlankArgument = require('./types').BlankArgument; /** * The types we expose for registration @@ -86,6 +87,7 @@ exports.items = [ item: 'type', name: 'remote', paramName: undefined, + blankIsValid: false, getSpec: function(commandName, paramName) { return { @@ -95,6 +97,16 @@ exports.items = [ }; }, + getBlank: function(context) { + var val = { stringified: '' }; + if (this.blankIsValid) { + return new Conversion(val, new BlankArgument(), Status.VALID); + } + else { + return new Conversion(val, new BlankArgument(), Status.INCOMPLETE, ''); + } + }, + stringify: function(value, context) { if (value == null) { return ''; diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index cfd7a0a3..823040fd 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -166,8 +166,13 @@ exports.items = [ } }, - getSpec: function() { - return { name: 'nodelist', allowEmpty: this.allowEmpty }; + getSpec: function(commandName, paramName) { + return { + name: 'remote', + commandName: commandName, + paramName: paramName, + blankIsValid: true + }; }, getBlank: function(context) { From 6c82474c6a9b84a5fd2df3bd9bee29a43b421daf Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 13 Mar 2015 12:56:55 +0000 Subject: [PATCH 27/63] runat-1128988: Stop caching the results of getBlank() nodelist types have a blank value that is an empty NodeList, and a NodeList is tied to a document which can change (by navigation) so we need to ask call getBlank() whenever we want the defaultValue. Signed-off-by: Joe Walker --- lib/gcli/commands/commands.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/gcli/commands/commands.js b/lib/gcli/commands/commands.js index 17643117..b1f0a190 100644 --- a/lib/gcli/commands/commands.js +++ b/lib/gcli/commands/commands.js @@ -277,9 +277,17 @@ function Parameter(types, paramSpec, command, groupName) { ': Missing defaultValue for optional parameter.'); } - this.defaultValue = (this.paramSpec.defaultValue !== undefined) ? - this.paramSpec.defaultValue : - this.type.getBlank().value; + if (this.paramSpec.defaultValue !== undefined) { + this.defaultValue = this.paramSpec.defaultValue; + } + else { + Object.defineProperty(this, 'defaultValue', { + get: function() { + return this.type.getBlank().value; + }, + enumerable: true + }); + } // Resolve the documentation this.manual = lookup(this.paramSpec.manual); From de3572662936f08a0633fadb341600b717d25b7c Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 13 Mar 2015 12:57:20 +0000 Subject: [PATCH 28/63] runat-1128988: Minor comment clarification Signed-off-by: Joe Walker --- lib/gcli/languages/command.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/gcli/languages/command.js b/lib/gcli/languages/command.js index 6cfa4689..043206cf 100644 --- a/lib/gcli/languages/command.js +++ b/lib/gcli/languages/command.js @@ -123,7 +123,6 @@ var commandLanguage = exports.commandLanguage = { this.commandDom = undefined; }, - // From the requisition.textChanged event textChanged: function() { if (this.terminal == null) { return; // This can happen post-destroy() From 4dc4581ad9892fa246f0d7119958dc5ff00fb99b Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:01:28 +0000 Subject: [PATCH 29/63] runat-1128988: Having functions called 'eval' seems dangerous I tried using GCLI from babel and it didn't like to have functions called 'eval'. It looks legal under ES5 'strict mode' to me, but I don't think eval is a good name all the same. Signed-off-by: Joe Walker --- lib/gcli/languages/javascript.js | 7 +++---- lib/gcli/util/host.js | 2 +- web/gcli/util/host.js | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/gcli/languages/javascript.js b/lib/gcli/languages/javascript.js index f4df6877..229cdd4f 100644 --- a/lib/gcli/languages/javascript.js +++ b/lib/gcli/languages/javascript.js @@ -42,8 +42,7 @@ exports.items = [ }, exec: function(input) { - return this.eval(input).then(function(response) { - // console.log('javascript.exec', response); + return this.evaluate(input).then(function(response) { var output = (response.exception != null) ? response.exception.class : response.output; @@ -80,8 +79,8 @@ exports.items = [ }.bind(this)); }, - eval: function(input) { - return host.script.eval(input); + evaluate: function(input) { + return host.script.evaluate(input); } } ]; diff --git a/lib/gcli/util/host.js b/lib/gcli/util/host.js index 0012a4fd..c9dd46b8 100644 --- a/lib/gcli/util/host.js +++ b/lib/gcli/util/host.js @@ -177,7 +177,7 @@ exports.script = { useTarget: function(tgt) { }, // Execute some JavaScript - eval: function(javascript) { + evaluate: function(javascript) { try { return Promise.resolve({ input: javascript, diff --git a/web/gcli/util/host.js b/web/gcli/util/host.js index d4d06483..16b3bfe1 100644 --- a/web/gcli/util/host.js +++ b/web/gcli/util/host.js @@ -164,7 +164,7 @@ exports.script = { useTarget: function(tgt) { }, // Execute some JavaScript - eval: function(javascript) { + evaluate: function(javascript) { try { return Promise.resolve({ input: javascript, From 34370e7992668f07cdcdbec4812b39bcb6ea4d9b Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:19:34 +0000 Subject: [PATCH 30/63] runat-1128988: Remove 'with' from the template engine Slightly sad to replace 3 simple lines of code with 7 complex lines of code that need 14 lines of commentary to explain them. Some module loaders force everything into strict mode so just leaving off 'use strict' wasn't an option any more. Signed-off-by: Joe Walker --- lib/gcli/commands/preflist.js | 2 ++ lib/gcli/util/domtemplate.js | 49 ++++++++++++++++++++++++++++------- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/lib/gcli/commands/preflist.js b/lib/gcli/commands/preflist.js index 9652b03e..67ac5591 100644 --- a/lib/gcli/commands/preflist.js +++ b/lib/gcli/commands/preflist.js @@ -152,6 +152,8 @@ function PrefList(prefsData, conversionContext) { this.search = prefsData.search; this.settings = prefsData.settings; this.conversionContext = conversionContext; + + this.onLoad = this.onLoad.bind(this); } /** diff --git a/lib/gcli/util/domtemplate.js b/lib/gcli/util/domtemplate.js index f7572989..10323df0 100644 --- a/lib/gcli/util/domtemplate.js +++ b/lib/gcli/util/domtemplate.js @@ -14,12 +14,7 @@ * limitations under the License. */ -/* jshint strict:false */ -// -// -// - -'do not use strict'; +'use strict'; // WARNING: do not 'use strict' without reading the notes in envEval(); // Also don't remove the 'do not use strict' marker. The orion build uses these @@ -564,10 +559,35 @@ function envEval(state, script, data, frame) { ' can not be resolved using a simple property path.'); return '${' + script + '}'; } - /* jshint -W085 */ - with (data) { - return eval(script); - } + + // What we're looking to do is basically: + // with(data) { return eval(script); } + // except in strict mode where 'with' is banned. + // So we create a function which has a parameter list the same as the + // keys in 'data' and with 'script' as its function body. + // We then call this function with the values in 'data' + var keys = allKeys(data); + var args = keys.join(', '); + var func = 'function(' + args + ') { return ' + script + '; }'; + + // In order to extract this function from the eval, we wrap it in an IIFE + func = '(function() { return ' + func + ' })();'; + func = eval(func); + + var values = keys.map(function(key) { return data[key]; }); + return func.apply(null, values); + + // TODO: The 'with' method is different from the code above in the value + // of 'this' when calling functions. For example: + // envEval(state, 'foo()', { foo: function() { return this; } }, ...); + // The global for 'foo' when using 'with' is the data object. However the + // code above, the global is null. (Using 'func.apply(data, values)' + // changes 'this' in the 'foo()' frame, but not in the inside the body + // of 'foo', so that wouldn't help) + + // TODO: Come ES6 we can probably do something like this: + // let func = new Function(...keys, 'return ' + script); + // return func(...keys.map(key => data[key])); } } catch (ex) { @@ -579,6 +599,15 @@ function envEval(state, script, data, frame) { } } +/** + * Object.keys() that respects the prototype chain + */ +function allKeys(data) { + var keys = []; + for (var key in data) { keys.push(key); } + return keys; +} + /** * A generic way of reporting errors, for easy overloading in different * environments. From f216e8ad3c3d7d6371f4653ec5d6513b8e77a428 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:11:27 +0000 Subject: [PATCH 31/63] runat-1128988: Remove onEnter/onLeave/onChange notification The idea was that we could highlight nodes when command line cursor was 'inside' the relevant argument. But it's not working for several reasons: * It needs remoting (and this could be a perf issue) * We've not hooked it up to the new multiple node highlighter * The requirement to have context.environment.window hooked up is making things worse So I'm disabling it here. Signed-off-by: Joe Walker --- lib/gcli/types/node.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index 823040fd..63aa3555 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -56,7 +56,9 @@ exports.getWindowHolder = function() { * NodeListType to allow terminal to tell us which nodes should be highlighted */ function onEnter(assignment) { - assignment.highlighter = new Highlighter(windowHolder.window.document); + // TODO: GCLI doesn't support passing a context to notifications of cursor + // position, so onEnter/onLeave/onChange are disabled below until we fix this + assignment.highlighter = new Highlighter(context.environment.window.document); assignment.highlighter.nodelist = assignment.conversion.matches; } @@ -141,9 +143,9 @@ exports.items = [ return Promise.resolve(reply); }, - onEnter: onEnter, - onLeave: onLeave, - onChange: onChange + // onEnter: onEnter, + // onLeave: onLeave, + // onChange: onChange }, { // The 'nodelist' type is a CSS expression that refers to a node list @@ -217,8 +219,8 @@ exports.items = [ return Promise.resolve(reply); }, - onEnter: onEnter, - onLeave: onLeave, - onChange: onChange + // onEnter: onEnter, + // onLeave: onLeave, + // onChange: onChange } ]; From 5239d21ba882ed2f6af95248b3807b6094b132fb Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:19:07 +0000 Subject: [PATCH 32/63] runat-1128988: Replace windowHolder with environment windowHolder was flawed in that there was only one per JS loader, but we needed one for each command line. The theory of 'environment' was that it wasn't owned by GCLI. It was provided by the implementor of GCLI for the commands. This breaks that by requiring that 'window' have a special value (if we're going to use the node/nodelist/resource/javascript types) Given the complexities of alternative solutions, this seems like an ok trade-off. This commit is implementation only. Next we're going to fix tests. Signed-off-by: Joe Walker --- lib/gcli/commands/mocks.js | 4 +-- lib/gcli/test/mockDocument.js | 39 ++++++++++++++-------------- lib/gcli/test/testJs.js | 35 +++++++++---------------- lib/gcli/types/javascript.js | 30 ++-------------------- lib/gcli/types/node.js | 38 +++++---------------------- lib/gcli/types/resource.js | 48 +++++++++-------------------------- 6 files changed, 55 insertions(+), 139 deletions(-) diff --git a/lib/gcli/commands/mocks.js b/lib/gcli/commands/mocks.js index e14c124a..0dc614ab 100644 --- a/lib/gcli/commands/mocks.js +++ b/lib/gcli/commands/mocks.js @@ -47,13 +47,13 @@ exports.items = [ on: function(requisition) { mockCommands.setup(requisition); mockSettings.setup(requisition.system); - mockDocument.setup(); + mockDocument.setup(requisition); }, off: function(requisition) { mockCommands.shutdown(requisition); mockSettings.shutdown(requisition.system); - mockDocument.shutdown(); + mockDocument.shutdown(requisition); } } ]; diff --git a/lib/gcli/test/mockDocument.js b/lib/gcli/test/mockDocument.js index 886cee94..a137063a 100644 --- a/lib/gcli/test/mockDocument.js +++ b/lib/gcli/test/mockDocument.js @@ -19,30 +19,31 @@ var nodetype = require('../types/node'); var jsdom = require('jsdom').jsdom; -var origWindowHolder; +var usingMockDocument = false; /** * Registration and de-registration. */ -exports.setup = function() { - origWindowHolder = nodetype.getWindowHolder(); +exports.setup = function(requisition) { + if (requisition.environment.window == null) { + var document = jsdom('' + + '' + + '' + + ' ' + + ' ' + + '' + + '' + + '
' + + '' + + ''); + requisition.environment.window = document.defaultView; - var document = jsdom('' + - '' + - '' + - ' ' + - ' ' + - '' + - '' + - '
' + - '' + - ''); - - nodetype.setWindowHolder({ - window: document.defaultView, - }); + usingMockDocument = true; + } }; -exports.shutdown = function() { - nodetype.setWindowHolder(origWindowHolder); +exports.shutdown = function(requisition) { + if (usingMockDocument) { + requisition.environment.window = undefined; + } }; diff --git a/lib/gcli/test/testJs.js b/lib/gcli/test/testJs.js index 47e761b9..b6a110b1 100644 --- a/lib/gcli/test/testJs.js +++ b/lib/gcli/test/testJs.js @@ -18,34 +18,23 @@ var assert = require('../testharness/assert'); var helpers = require('./helpers'); -var javascript = require('../types/javascript'); - -// Store the original windowHolder -var tempWindowHolder; - -// Mock windowHolder to check that we're not trespassing on 'donteval' -var mockWindowHolder = { - window: { - document: {} - }, -}; -mockWindowHolder.window = mockWindowHolder; -Object.defineProperty(mockWindowHolder.window, 'donteval', { - get: function() { - assert.ok(false, 'donteval should not be used'); - return { cant: '', touch: '', 'this': '' }; - }, - enumerable: true, - configurable: true -}); exports.setup = function(options) { if (!jsTestAllowed(options)) { return; } - tempWindowHolder = javascript.getWindowHolder(); - javascript.setWindowHolder(mockWindowHolder); + // Check that we're not trespassing on 'donteval' + var win = options.requisition.environment.window; + Object.defineProperty(win, 'donteval', { + get: function() { + assert.ok(false, 'donteval should not be used'); + console.trace(); + return { cant: '', touch: '', 'this': '' }; + }, + enumerable: true, + configurable: true + }); }; exports.shutdown = function(options) { @@ -53,7 +42,7 @@ exports.shutdown = function(options) { return; } - javascript.setWindowHolder(tempWindowHolder); + delete options.requisition.environment.window.donteval; }; function jsTestAllowed(options) { diff --git a/lib/gcli/types/javascript.js b/lib/gcli/types/javascript.js index d3318385..1cbad2c4 100644 --- a/lib/gcli/types/javascript.js +++ b/lib/gcli/types/javascript.js @@ -23,33 +23,6 @@ var Conversion = require('./types').Conversion; var Type = require('./types').Type; var Status = require('./types').Status; -/** - * windowHolder is a way to get at a window/document/etc. The page that we're - * looking at could change, so a call to windowHolder.window could change - * between calls from the event loop. - */ -var windowHolder; -if (typeof document !== 'undefined') { - windowHolder = { - window: document.defaultView, - }; -} - -/** - * Setter for the document that contains the nodes we're matching - */ -exports.setWindowHolder = function(wh) { - windowHolder = wh; -}; - -/** - * Getter for the document that contains the nodes we're matching - * Most for changing things back to how they were for unit testing - */ -exports.getWindowHolder = function() { - return windowHolder; -}; - /** * 'javascript' handles scripted input */ @@ -77,7 +50,8 @@ JavascriptType.MAX_COMPLETION_MATCHES = 10; JavascriptType.prototype.parse = function(arg, context) { var typed = arg.text; - var scope = (windowHolder == null) ? null : windowHolder.window; + var scope = (context.environment.window == null) ? + null : context.environment.window; // No input is undefined if (typed === '') { diff --git a/lib/gcli/types/node.js b/lib/gcli/types/node.js index 63aa3555..0ce80107 100644 --- a/lib/gcli/types/node.js +++ b/lib/gcli/types/node.js @@ -24,33 +24,6 @@ var Status = require('./types').Status; var Conversion = require('./types').Conversion; var BlankArgument = require('./types').BlankArgument; -/** - * windowHolder is a way to get at a window/document/etc. The page that we're - * looking at could change, so a call to windowHolder.window could change - * between calls from the event loop. - */ -var windowHolder; -if (typeof document !== 'undefined') { - windowHolder = { - window: document.defaultView, - }; -} - -/** - * Setter for the document that contains the nodes we're matching - */ -exports.setWindowHolder = function(wh) { - windowHolder = wh; -}; - -/** - * Getter for the document that contains the nodes we're matching - * Most for changing things back to how they were for unit testing - */ -exports.getWindowHolder = function() { - return windowHolder; -}; - /** * Helper functions to be attached to the prototypes of NodeType and * NodeListType to allow terminal to tell us which nodes should be highlighted @@ -116,7 +89,7 @@ exports.items = [ else { var nodes; try { - nodes = windowHolder.window.document.querySelectorAll(arg.text); + nodes = context.environment.window.document.querySelectorAll(arg.text); if (nodes.length === 0) { reply = new Conversion(undefined, arg, Status.INCOMPLETE, l10n.lookup('nodeParseNone')); @@ -178,8 +151,11 @@ exports.items = [ }, getBlank: function(context) { - var emptyNodeList = (windowHolder == null) ? [] : - util.createEmptyNodeList(windowHolder.window.document); + var emptyNodeList = []; + if (context != null && context.environment.window != null) { + var doc = context.environment.window.document; + emptyNodeList = util.createEmptyNodeList(doc); + } return new Conversion(emptyNodeList, new BlankArgument(), Status.VALID); }, @@ -197,7 +173,7 @@ exports.items = [ reply = new Conversion(undefined, arg, Status.INCOMPLETE); } else { - var nodes = windowHolder.window.document.querySelectorAll(arg.text); + var nodes = context.environment.window.document.querySelectorAll(arg.text); if (nodes.length === 0 && !this.allowEmpty) { reply = new Conversion(undefined, arg, Status.INCOMPLETE, diff --git a/lib/gcli/types/resource.js b/lib/gcli/types/resource.js index d539418a..d84fd953 100644 --- a/lib/gcli/types/resource.js +++ b/lib/gcli/types/resource.js @@ -22,33 +22,6 @@ exports.clearResourceCache = function() { ResourceCache.clear(); }; -/** - * windowHolder is a way to get at a window/document/etc. The page that we're - * looking at could change, so a call to windowHolder.window could change - * between calls from the event loop. - */ -var windowHolder; -if (typeof document !== 'undefined') { - windowHolder = { - window: document.defaultView, - }; -} - -/** - * Setter for the document that contains the nodes we're matching - */ -exports.setWindowHolder = function(wh) { - windowHolder = wh; -}; - -/** - * Getter for the document that contains the nodes we're matching - * Most for changing things back to how they were for unit testing - */ -exports.getWindowHolder = function() { - return windowHolder; -}; - /** * Resources are bits of CSS and JavaScript that the page either includes * directly or as a result of reading some remote resource. @@ -98,13 +71,13 @@ CssResource.prototype.loadContents = function() { }.bind(this)); }; -CssResource._getAllStyles = function() { +CssResource._getAllStyles = function(context) { var resources = []; - if (windowHolder == null) { + if (context.environment.window == null) { return resources; } - var doc = windowHolder.window.document; + var doc = context.environment.window.document; Array.prototype.forEach.call(doc.styleSheets, function(domSheet) { CssResource._getStyle(domSheet, resources); }); @@ -178,12 +151,13 @@ ScriptResource.prototype.loadContents = function() { }.bind(this)); }; -ScriptResource._getAllScripts = function() { - if (windowHolder == null) { +ScriptResource._getAllScripts = function(context) { + if (context.environment.window == null) { return []; } - var scriptNodes = windowHolder.window.document.querySelectorAll('script'); + var doc = context.environment.window.document; + var scriptNodes = doc.querySelectorAll('script'); var resources = Array.prototype.map.call(scriptNodes, function(scriptNode) { var resource = ResourceCache.get(scriptNode); if (!resource) { @@ -279,13 +253,15 @@ exports.items = [ } }, - lookup: function() { + lookup: function(context) { var resources = []; if (this.include !== Resource.TYPE_SCRIPT) { - Array.prototype.push.apply(resources, CssResource._getAllStyles()); + Array.prototype.push.apply(resources, + CssResource._getAllStyles(context)); } if (this.include !== Resource.TYPE_CSS) { - Array.prototype.push.apply(resources, ScriptResource._getAllScripts()); + Array.prototype.push.apply(resources, + ScriptResource._getAllScripts(context)); } return new Promise(function(resolve, reject) { From 8a31381bd1878072afd59cbfd3293223375308ec Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:26:26 +0000 Subject: [PATCH 33/63] runat-1128988: Fix tests that we broke recently Pass 'context' to resource tests. Narrow down the Node breakage in the JS tests. More remote values are not available and shouldn't be tested for Signed-off-by: Joe Walker --- lib/gcli/test/testCli2.js | 2 +- lib/gcli/test/testJs.js | 11 +++-------- lib/gcli/test/testResource.js | 31 +++++++++++++++++-------------- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/lib/gcli/test/testCli2.js b/lib/gcli/test/testCli2.js index bed7b4fe..431c8073 100644 --- a/lib/gcli/test/testCli2.js +++ b/lib/gcli/test/testCli2.js @@ -411,7 +411,7 @@ exports.testElement = function(options) { unassigned: [ ], args: { command: { name: 'tse' }, - node: { value: undefined, arg: '', status: 'INCOMPLETE' }, + node: { arg: '', status: 'INCOMPLETE' }, nodes: { arg: '', status: 'VALID', message: '' }, nodes2: { arg: '', status: 'VALID', message: '' }, } diff --git a/lib/gcli/test/testJs.js b/lib/gcli/test/testJs.js index b6a110b1..23f772ac 100644 --- a/lib/gcli/test/testJs.js +++ b/lib/gcli/test/testJs.js @@ -46,8 +46,7 @@ exports.shutdown = function(options) { }; function jsTestAllowed(options) { - return options.isRemote || // We're directly accessing the javascript type - options.isNode || + return options.isRemote || // Altering the environment (which isn't remoted) options.requisition.system.commands.get('{') == null; } @@ -288,7 +287,8 @@ exports.testDocument = function(options) { command: { name: '{' }, javascript: { value: 'document.title', - arg: '{ document.title ', + // arg: '{ document.title ', + // Node/JSDom gets this wrong and omits the trailing space. Why? status: 'VALID', message: '' } @@ -321,11 +321,6 @@ exports.testDocument = function(options) { }; exports.testDonteval = function(options) { - if (jsTestAllowed(options)) { - // nodom causes an eval here, maybe that's node/v8? - assert.ok('donteval' in mockWindowHolder.window, 'donteval exists'); - } - return helpers.audit(options, [ { skipRemainingIf: true, // Commented out until we fix non-enumerable props diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index e3776dcb..84b250a8 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -29,12 +29,13 @@ exports.testAllPredictions1 = function(options) { return; } + var context = options.requisition.conversionContext; var resource = options.requisition.system.types.createType('resource'); - return resource.getLookup().then(function(opts) { + return resource.getLookup(context).then(function(opts) { assert.ok(opts.length > 1, 'have all resources'); return util.promiseEach(opts, function(prediction) { - return checkPrediction(resource, prediction); + return checkPrediction(resource, prediction, context); }); }); }; @@ -45,13 +46,14 @@ exports.testScriptPredictions = function(options) { return; } + var context = options.requisition.conversionContext; var types = options.requisition.system.types; var resource = types.createType({ name: 'resource', include: 'text/javascript' }); - return resource.getLookup().then(function(opts) { + return resource.getLookup(context).then(function(opts) { assert.ok(opts.length > 1, 'have js resources'); return util.promiseEach(opts, function(prediction) { - return checkPrediction(resource, prediction); + return checkPrediction(resource, prediction, context); }); }); }; @@ -62,26 +64,28 @@ exports.testStylePredictions = function(options) { return; } + var context = options.requisition.conversionContext; var types = options.requisition.system.types; var resource = types.createType({ name: 'resource', include: 'text/css' }); - return resource.getLookup().then(function(opts) { + return resource.getLookup(context).then(function(opts) { assert.ok(opts.length >= 1, 'have css resources'); return util.promiseEach(opts, function(prediction) { - return checkPrediction(resource, prediction); + return checkPrediction(resource, prediction, context); }); }); }; exports.testAllPredictions2 = function(options) { + var context = options.requisition.conversionContext; var types = options.requisition.system.types; var scriptRes = types.createType({ name: 'resource', include: 'text/javascript' }); - return scriptRes.getLookup().then(function(scriptOptions) { + return scriptRes.getLookup(context).then(function(scriptOptions) { var styleRes = types.createType({ name: 'resource', include: 'text/css' }); - return styleRes.getLookup().then(function(styleOptions) { + return styleRes.getLookup(context).then(function(styleOptions) { var allRes = types.createType({ name: 'resource' }); - return allRes.getLookup().then(function(allOptions) { + return allRes.getLookup(context).then(function(allOptions) { assert.is(scriptOptions.length + styleOptions.length, allOptions.length, 'split'); @@ -91,22 +95,21 @@ exports.testAllPredictions2 = function(options) { }; exports.testAllPredictions3 = function(options) { + var context = options.requisition.conversionContext; var types = options.requisition.system.types; var res1 = types.createType({ name: 'resource' }); - return res1.getLookup().then(function(options1) { + return res1.getLookup(context).then(function(options1) { var res2 = types.createType('resource'); - return res2.getLookup().then(function(options2) { + return res2.getLookup(context).then(function(options2) { assert.is(options1.length, options2.length, 'type spec'); }); }); }; -function checkPrediction(res, prediction) { +function checkPrediction(res, prediction, context) { var name = prediction.name; var value = prediction.value; - // resources don't need context so cheat and pass in null - var context = null; return res.parseString(name, context).then(function(conversion) { assert.is(conversion.getStatus(), Status.VALID, 'status VALID for ' + name); assert.is(conversion.value, value, 'value for ' + name); From 2071763b92cbb0950c7fa57a584084fbc44c0ef5 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:27:20 +0000 Subject: [PATCH 34/63] runat-1128988: 2 minor fixes Used value in delegate type, and format fix in cli.js Signed-off-by: Joe Walker --- lib/gcli/cli.js | 2 +- lib/gcli/types/delegate.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js index 161b18ce..33332cfe 100644 --- a/lib/gcli/cli.js +++ b/lib/gcli/cli.js @@ -659,7 +659,7 @@ Requisition.prototype.getParameterNames = function() { * this is still an error status. */ Object.defineProperty(Requisition.prototype, 'status', { - get : function() { + get: function() { var status = Status.VALID; if (this._unassigned.length !== 0) { var isAllIncomplete = true; diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index c58334db..4171c856 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -125,7 +125,6 @@ exports.items = [ parse: function(arg, context) { return this.front.parseType(context.typed, this.paramName).then(function(json) { var status = Status.fromString(json.status); - var val = { stringified: arg.text }; return new Conversion(undefined, arg, status, json.message, json.predictions); }.bind(this)); }, From 291909e3908856b306cf3478c3ce0b3d1d7954b8 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:35:08 +0000 Subject: [PATCH 35/63] runat-1128988: Pass on blankIsValid to delegate children I'm not sure we'll ever actually need to double-remote a type, but just in case we should pass on the data that's needed. Signed-off-by: Joe Walker --- lib/gcli/types/delegate.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index 4171c856..c962a0d6 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -93,7 +93,8 @@ exports.items = [ return { name: 'remote', commandName: commandName, - paramName: paramName + paramName: paramName, + blankIsValid: this.blankIsValid }; }, From 5d83bd3624e4525e1e817f7a6f165d6bca1769a5 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:41:44 +0000 Subject: [PATCH 36/63] runat-1128988: Use undefined for the value when !blankIsValid When blank isn't valid it's important that we have a value of undefined so that the isDataRequired calculation works, which seems broken. I'm not going to dig deeply into fixing it now though. Signed-off-by: Joe Walker --- lib/gcli/commands/commands.js | 3 +++ lib/gcli/types/delegate.js | 7 ++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/gcli/commands/commands.js b/lib/gcli/commands/commands.js index b1f0a190..67793b2d 100644 --- a/lib/gcli/commands/commands.js +++ b/lib/gcli/commands/commands.js @@ -295,6 +295,9 @@ function Parameter(types, paramSpec, command, groupName) { // Is the user required to enter data for this parameter? (i.e. has // defaultValue been set to something other than undefined) + // TODO: When the defaultValue comes from type.getBlank().value (see above) + // then perhaps we should set using something like + // isDataRequired = (type.getBlank().status !== VALID) this.isDataRequired = (this.defaultValue === undefined); // Are we allowed to assign data to this parameter using positional diff --git a/lib/gcli/types/delegate.js b/lib/gcli/types/delegate.js index c962a0d6..940bf555 100644 --- a/lib/gcli/types/delegate.js +++ b/lib/gcli/types/delegate.js @@ -99,12 +99,13 @@ exports.items = [ }, getBlank: function(context) { - var val = { stringified: '' }; if (this.blankIsValid) { - return new Conversion(val, new BlankArgument(), Status.VALID); + return new Conversion({ stringified: '' }, + new BlankArgument(), Status.VALID); } else { - return new Conversion(val, new BlankArgument(), Status.INCOMPLETE, ''); + return new Conversion(undefined, new BlankArgument(), + Status.INCOMPLETE, ''); } }, From 75877deb8167c0fa3e0731851755f808a0c1c371 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Tue, 17 Mar 2015 13:42:25 +0000 Subject: [PATCH 37/63] runat-1128988: Be defensive about domSheet.ownerNode Signed-off-by: Joe Walker --- lib/gcli/types/resource.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gcli/types/resource.js b/lib/gcli/types/resource.js index d84fd953..67a5b60d 100644 --- a/lib/gcli/types/resource.js +++ b/lib/gcli/types/resource.js @@ -53,7 +53,7 @@ Resource.TYPE_CSS = 'text/css'; function CssResource(domSheet) { this.name = domSheet.href; if (!this.name) { - this.name = domSheet.ownerNode.id ? + this.name = domSheet.ownerNode && domSheet.ownerNode.id ? 'css#' + domSheet.ownerNode.id : 'inline-css'; } From 6c0a31c5a99a23e2ea867e54e1e61825fa973530 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 11:22:48 +0000 Subject: [PATCH 38/63] runat-1128988: Add event.once I'm really surprised that this wasn't there already. Signed-off-by: Joe Walker --- lib/gcli/util/util.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/gcli/util/util.js b/lib/gcli/util/util.js index 16fade57..fb9ae1da 100644 --- a/lib/gcli/util/util.js +++ b/lib/gcli/util/util.js @@ -160,6 +160,20 @@ exports.createEvent = function(name) { handlers = []; }; + /** + * Fire an event just once using a promise. + */ + event.once = function() { + return new Promise(function(resolve, reject) { + var handler = function(arg) { + event.remove(handler); + resolve(arg); + }; + + event.add(handler); + }); + }, + /** * Temporarily prevent this event from firing. * @see resumeFire(ev) From 6c11f94fc458bfdf9b6009cc971335fe00680642 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 12:05:40 +0000 Subject: [PATCH 39/63] runat-1128988: Don't stop loading modules if one fails We made this change a while back for modules that were loaded without delay. This does the same for modules with delayedLoad:true Signed-off-by: Joe Walker --- lib/gcli/system.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 89c79e01..3d409a81 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -187,7 +187,7 @@ exports.createSystem = function(options) { var promises = Object.keys(loadableModules).map(function(name) { delete modules[name]; - return loadModule(name); + return loadModule(name).catch(console.error); }); Object.keys(modules).forEach(unloadModule); From 87cb17626d9f02aac66ac419e5e26a1853219154 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 12:09:25 +0000 Subject: [PATCH 40/63] runat-1128988: Simplify loading mockCommands in Firefox We're going to load mockCommands into the server process which means it needs to be loadable via require as well as loadScript. We also want to minimize the processing done as we sync the file between trees. Signed-off-by: Joe Walker --- lib/gcli/test/mockCommands.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index 955023b8..9079fc2f 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -17,7 +17,16 @@ 'use strict'; var Promise = require('../util/promise').Promise; -var mockCommands = exports; + +var mockCommands; +if (typeof exports !== 'undefined') { + // If we're being loaded via require(); + mockCommands = exports; +} +else { + // If we're being loaded via loadScript in mochitest + mockCommands = {}; +} // We use an alias for exports here because this module is used in Firefox // mochitests where we don't have define/require From 879c05f0a07ea60240370fd4af4b3bfd962be872 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 12:13:07 +0000 Subject: [PATCH 41/63] runat-1128988: Protocol.js events are lower-case-with-dashes And not camelCase. Also we're not sending the specs with the event because then we'd need to get customProps right, and because it prevents the receiver deciding if it wants to transfer a significant amount of data. Signed-off-by: Joe Walker --- lib/gcli/connectors/remoted.js | 3 +-- lib/gcli/system.js | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/gcli/connectors/remoted.js b/lib/gcli/connectors/remoted.js index 5c8286fd..66f9f3a9 100644 --- a/lib/gcli/connectors/remoted.js +++ b/lib/gcli/connectors/remoted.js @@ -42,8 +42,7 @@ Remoter.prototype.addListener = function(action) { var listener = { action: action, caller: function() { - var commands = this.requisition.system.commands; - action('commandsChanged', commands.getCommandSpecs()); + action('commands-changed'); }.bind(this) }; this._listeners.push(listener); diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 3d409a81..5f2ba546 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -255,7 +255,7 @@ exports.createSystem = function(options) { * commands imported into the local system */ exports.connectFront = function(system, front, customProps) { - front.on('commandsChanged', function(specs) { + front.on('commands-changed', function() { syncItems(system, front, customProps).catch(util.errorHandler); }); @@ -284,7 +284,7 @@ function syncItems(system, front, customProps) { }; /** - * Take the data from the 'specs' command (or the 'commandsChanged' event) and + * Take the data from the 'specs' command (or the 'commands-changed' event) and * add function to proxy the execution back over the front */ function addLocalFunctions(specs, front) { From 1a76df20248cf676798a7a0c7950afc8b10fdd9a Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 12:14:47 +0000 Subject: [PATCH 42/63] runat-1128988: Add event batching to commands-changed We don't want to light up the remote interface 10x for every module, so this adds holdFire/resumeFire to addItemsByModule and friends. Signed-off-by: Joe Walker --- lib/gcli/system.js | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 5f2ba546..429ecd68 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -149,7 +149,14 @@ exports.createSystem = function(options) { }, addItemsByModule: function(names, options) { + var promises = []; + options = options || {}; + if (!options.delayedLoad) { + // We could be about to add many commands, just report the change once + this.commands.onCommandsChange.holdFire(); + } + if (typeof names === 'string') { names = [ names ]; } @@ -165,20 +172,34 @@ exports.createSystem = function(options) { pendingChanges = true; } else { - loadModule(name).catch(console.error); + promises.push(loadModule(name).catch(console.error)); } }); + + if (options.delayedLoad) { + return Promise.resolve(); + } + else { + return Promise.all(promises).then(function() { + this.commands.onCommandsChange.resumeFire(); + }.bind(this)); + } }, removeItemsByModule: function(name) { + this.commands.onCommandsChange.holdFire(); + delete loadableModules[name]; unloadModule(name); + + this.commands.onCommandsChange.resumeFire(); }, load: function() { if (!pendingChanges) { return Promise.resolve(); } + this.commands.onCommandsChange.holdFire(); // clone loadedModules, so we can remove what is left at the end var modules = Object.keys(loadedModules).map(function(name) { @@ -193,7 +214,9 @@ exports.createSystem = function(options) { Object.keys(modules).forEach(unloadModule); pendingChanges = false; - return Promise.all(promises); + return Promise.all(promises).then(function() { + this.commands.onCommandsChange.resumeFire(); + }.bind(this)); }, toString: function() { From def985f5c84258eed8ff7ced06a2b6c4cca9c666 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 12:20:05 +0000 Subject: [PATCH 43/63] runat-1128988: Simplify header injected into tests We're (broadly) doing 2 things here. Partly we're causing all mock commands to be loaded into the server process. We're still running the test commands against the client process. This exercises the more of the infrastructure with the test suite. The changes to do that double the size of the test runner that we inject into all the unit test files. Cut and paste is bad, so we abstract this into a new function in helpers.js This change fixes how we alter the test files as we copy them from the GCLI tree to mozilla-central. It's probably best to understand this looking at what happens to central when we run this command. Signed-off-by: Joe Walker --- lib/gcli/commands/server/firefox.js | 51 +++++++++-------------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/lib/gcli/commands/server/firefox.js b/lib/gcli/commands/server/firefox.js index 23eef825..aa9fbf07 100644 --- a/lib/gcli/commands/server/firefox.js +++ b/lib/gcli/commands/server/firefox.js @@ -94,10 +94,7 @@ function buildFirefox(destDir) { ] }, filter: createCommonJsToJsTestFilter(), - filenameFilter: function(filename) { - // the t prefix prevents 'test' from matching the directory name - return filename.replace(/t\/test/, 't\/browser_gcli_').toLowerCase(); - }, + filenameFilter: gcliToCentralFilenameFilter, dest: (destDir ? destDir + testDir : 'built/ff') }); copy({ @@ -116,6 +113,15 @@ function buildFirefox(destDir) { return log; } +/** + * GCLI test files are called testFoo.js where mozilla-central uses + * browser_gcli_foo.js. This converts. + */ +function gcliToCentralFilenameFilter(filename) { + // the t prefix prevents 'test' from matching the directory name + return filename.replace(/t\/test/, 't\/browser_gcli_').toLowerCase(); +} + /** * Convert the CommonJS module style that GCLI uses to the plain JS style used * by Firefox. Note that this is custom for our tests, but it could be a @@ -123,33 +129,18 @@ function buildFirefox(destDir) { */ function createCommonJsToJsTestFilter() { var filter = function commonJsToJsTestFilter(data, location) { - var name = location.path.substring(1); + var name = gcliToCentralFilenameFilter(location.path.substring(1)); var header = '$1' + - '\n' + - '// \n' + '\n' + '// THIS FILE IS GENERATED FROM SOURCE IN THE GCLI PROJECT\n' + - '// DO NOT EDIT IT DIRECTLY\n' + + '// TALK TO SOMEONE IN DEVELOPER TOOLS BEFORE EDITING IT\n' + '\n' + - 'var exports = {};\n' + - '\n' + - 'var TEST_URI = "data:text/html;charset=utf-8,
gcli-' + name + '
";\n' + + 'const exports = {};\n' + '\n' + 'function test() {\n' + - ' return Task.spawn(function*() {\n' + - ' let options = yield helpers.openTab(TEST_URI);\n' + - ' yield helpers.openToolbar(options);\n' + - ' options.requisition.system.addItems(mockCommands.items);\n' + - '\n' + - ' yield helpers.runTests(options, exports);\n' + - '\n' + - ' options.requisition.system.removeItems(mockCommands.items);\n' + - ' yield helpers.closeToolbar(options);\n' + - ' yield helpers.closeTab(options);\n' + - ' }).then(finish, helpers.handleError);\n' + + ' helpers.runTestModule(exports, "' + name + '");\n' + '}\n' + - '\n' + - '// '; + '\n'; return data.toString() // Inject the header above just after 'use strict' .replace(/('use strict';)/, header) @@ -167,23 +158,13 @@ function createCommonJsToJsTestFilter() { function commonJsToJsMockFilter(data) { var header = '$1' + - '\n' + - '// \n' + '\n' + '// THIS FILE IS GENERATED FROM SOURCE IN THE GCLI PROJECT\n' + - '// DO NOT EDIT IT DIRECTLY\n' + - '\n' + - '// \n'; + '// DO NOT EDIT IT DIRECTLY\n'; return data.toString() // Inject the header above just after 'use strict' .replace(/('use strict';)/, header) - // In mochitests everything is global - .replace(/var mockCommands = exports;/, 'var mockCommands = {};') - // Comment out test helpers that we define separately - .replace(/(var [A-z]* = require\(['"][A-z_\.\/]*\/assert['"]\);)/g, '// $1') - .replace(/(var [A-z]* = require\(['"][A-z_\.\/]*\/helpers['"]\);)/g, '// $1') - .replace(/(var [A-z]* = require\(['"][A-z_\.\/]*\/mockCommands['"]\);)/g, '// $1') // Make the require statements absolute rather than relative. // We're ignoring paths that start ../.. or ./ but this works for now .replace(/\nvar ([A-z]*) = require\(['"]..\/([A-z_\/]*)['"]\)/g, '\nvar $1 = require(\'gcli/$2\')'); From 2e5318757b5fc048d86b8373ad5b84674274a94c Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 13:05:20 +0000 Subject: [PATCH 44/63] runat-1128988: Simplify header injected into tests (Fixed) Minor changes aimed at fixing the output of the previous commit. Again this probably makes more sense looking at the results, for which, see "The results of running 'firefox ../gecko-dev'" (joewalker/gecko-dev@3915d17) in joewalker/gecko-dev#10 Signed-off-by: Joe Walker --- lib/gcli/commands/server/firefox.js | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/lib/gcli/commands/server/firefox.js b/lib/gcli/commands/server/firefox.js index aa9fbf07..33c8da33 100644 --- a/lib/gcli/commands/server/firefox.js +++ b/lib/gcli/commands/server/firefox.js @@ -94,7 +94,10 @@ function buildFirefox(destDir) { ] }, filter: createCommonJsToJsTestFilter(), - filenameFilter: gcliToCentralFilenameFilter, + filenameFilter: function(filename) { + // the t prefix prevents 'test' from matching the directory name + return filename.replace(/t\/test/, 't\/browser_gcli_').toLowerCase(); + }, dest: (destDir ? destDir + testDir : 'built/ff') }); copy({ @@ -113,15 +116,6 @@ function buildFirefox(destDir) { return log; } -/** - * GCLI test files are called testFoo.js where mozilla-central uses - * browser_gcli_foo.js. This converts. - */ -function gcliToCentralFilenameFilter(filename) { - // the t prefix prevents 'test' from matching the directory name - return filename.replace(/t\/test/, 't\/browser_gcli_').toLowerCase(); -} - /** * Convert the CommonJS module style that GCLI uses to the plain JS style used * by Firefox. Note that this is custom for our tests, but it could be a @@ -129,18 +123,17 @@ function gcliToCentralFilenameFilter(filename) { */ function createCommonJsToJsTestFilter() { var filter = function commonJsToJsTestFilter(data, location) { - var name = gcliToCentralFilenameFilter(location.path.substring(1)); + var name = location.path.replace(/\/test/, 'browser_gcli_').toLowerCase() var header = '$1' + - '\n' + + '\n\n' + '// THIS FILE IS GENERATED FROM SOURCE IN THE GCLI PROJECT\n' + - '// TALK TO SOMEONE IN DEVELOPER TOOLS BEFORE EDITING IT\n' + + '// PLEASE TALK TO SOMEONE IN DEVELOPER TOOLS BEFORE EDITING IT\n' + '\n' + 'const exports = {};\n' + '\n' + 'function test() {\n' + ' helpers.runTestModule(exports, "' + name + '");\n' + - '}\n' + - '\n'; + '}'; return data.toString() // Inject the header above just after 'use strict' .replace(/('use strict';)/, header) @@ -158,9 +151,9 @@ function createCommonJsToJsTestFilter() { function commonJsToJsMockFilter(data) { var header = '$1' + - '\n' + + '\n\n' + '// THIS FILE IS GENERATED FROM SOURCE IN THE GCLI PROJECT\n' + - '// DO NOT EDIT IT DIRECTLY\n'; + '// PLEASE TALK TO SOMEONE IN DEVELOPER TOOLS BEFORE EDITING IT'; return data.toString() // Inject the header above just after 'use strict' From 5eb2bff060032c360c654eca4d73d517a9a09038 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 20:36:01 +0000 Subject: [PATCH 45/63] runat-1128988: Log actual predictions when a test fails Signed-off-by: Joe Walker --- lib/gcli/test/helpers.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gcli/test/helpers.js b/lib/gcli/test/helpers.js index 264da202..67757300 100644 --- a/lib/gcli/test/helpers.js +++ b/lib/gcli/test/helpers.js @@ -422,6 +422,10 @@ helpers._check = function(options, name, checks) { var index = actualPredictions.indexOf(prediction); assert.ok(index !== -1, 'predictionsContains:' + prediction + suffix); + if (index === -1) { + log('Actual predictions (' + actualPredictions.length + '): ' + + actualPredictions.join(', ')); + } }); }; outstanding.push(helpers._actual.predictions(options).then(containsCheck)); From 28c80bf893f6bfb930499f5066134f3e92cffc2b Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 20:37:50 +0000 Subject: [PATCH 46/63] runat-1128988: Use Promise.resolve for syncronous resolve. Signed-off-by: Joe Walker --- lib/gcli/types/resource.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/gcli/types/resource.js b/lib/gcli/types/resource.js index 67a5b60d..ea6c9c09 100644 --- a/lib/gcli/types/resource.js +++ b/lib/gcli/types/resource.js @@ -264,11 +264,9 @@ exports.items = [ ScriptResource._getAllScripts(context)); } - return new Promise(function(resolve, reject) { - resolve(resources.map(function(resource) { - return { name: resource.name, value: resource }; - })); - }.bind(this)); + return Promise.resolve(resources.map(function(resource) { + return { name: resource.name, value: resource }; + })); } } ]; From 04f795b8fd7428d57ad744a27acf3627a05d87a1 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 20:45:51 +0000 Subject: [PATCH 47/63] runat-1128988: Merge getSelectionData into getSeletionLookup Selection types have 2 ways to set the available options. 'data' allows you to provide an array of strings for types when the underlying type is a string. 'lookup' allows you to provide an array of objects with name and value properties for times when the underlying type isn't a string. Previously we had 2 remote functions one for data and one for lookup. The selection type has a function (getLookup) which does the work of resolving promises, resolving cases where data or lookup is a function and converting from data to lookup. There wasn't much point in having 2 remote functions when we could just use getLookup to resolve both. The changes to remoted.js will need to be mirrored in gcli.js in Firefox. Signed-off-by: Joe Walker --- lib/gcli/connectors/remoted.js | 75 ++++++++++------------------------ lib/gcli/types/selection.js | 7 +--- 2 files changed, 22 insertions(+), 60 deletions(-) diff --git a/lib/gcli/connectors/remoted.js b/lib/gcli/connectors/remoted.js index 66f9f3a9..1f3da578 100644 --- a/lib/gcli/connectors/remoted.js +++ b/lib/gcli/connectors/remoted.js @@ -176,30 +176,31 @@ Remoter.prototype.exposed = { * Call type.lookup() on a selection type to get the allowed values */ getSelectionLookup: method(function(commandName, paramName) { - var type = getType(this.requisition, commandName, paramName); + var command = this.requisition.system.commands.get(commandName); + if (command == null) { + throw new Error('No command called \'' + commandName + '\''); + } - var context = this.requisition.executionContext; - return type.lookup(context).map(function(info) { - // lookup returns an array of objects with name/value properties and - // the values might not be JSONable, so remove them - return { name: info.name }; + var type; + command.params.forEach(function(param) { + if (param.name === paramName) { + type = param.type; + } }); - }, { - request: { - commandName: Arg(0, "string"), // The command containing the parameter in question - paramName: Arg(1, "string"), // The name of the parameter - }, - response: RetVal("json") - }), - /** - * Call type.data() on a selection type to get the allowed values - */ - getSelectionData: method(function(commandName, paramName) { - var type = getType(this.requisition, commandName, paramName); + if (type == null) { + throw new Error('No parameter called \'' + paramName + '\' in \'' + + commandName + '\''); + } - var context = this.requisition.executionContext; - return type.data(context); + var reply = type.getLookup(this.requisition.executionContext); + return Promise.resolve(reply).then(function(lookup) { + // lookup returns an array of objects with name/value properties and + // the values might not be JSONable, so remove them + return lookup.map(function(info) { + return { name: info.name }; + }); + }); }, { request: { commandName: Arg(0, "string"), // The command containing the parameter in question @@ -259,32 +260,6 @@ Remoter.prototype.exposed = { }) }; -/** - * Helper for #getSelectionLookup and #getSelectionData that finds a type - * instance given a commandName and paramName - */ -function getType(requisition, commandName, paramName) { - var command = requisition.system.commands.get(commandName); - if (command == null) { - throw new Error('No command called \'' + commandName + '\''); - } - - var type; - command.params.forEach(function(param) { - if (param.name === paramName) { - type = param.type; - } - }); - - if (type == null) { - throw new Error('No parameter called \'' + paramName + '\' in \'' + - commandName + '\''); - } - - return type; -} - - /** * Asynchronous construction. Use GcliFront(); @@ -370,14 +345,6 @@ GcliFront.prototype.getSelectionLookup = function(commandName, paramName) { return this.connection.call('getSelectionLookup', data); }; -GcliFront.prototype.getSelectionData = function(commandName, paramName) { - var data = { - commandName: commandName, - paramName: paramName - }; - return this.connection.call('getSelectionData', data); -}; - GcliFront.prototype.system = function(cmd, args, cwd, env) { var data = { cmd: cmd, diff --git a/lib/gcli/types/selection.js b/lib/gcli/types/selection.js index 2ed41ce0..7995efb6 100644 --- a/lib/gcli/types/selection.js +++ b/lib/gcli/types/selection.js @@ -78,8 +78,7 @@ SelectionType.prototype.getSpec = function(commandName, paramName) { if (typeof this.lookup === 'function' || typeof this.data === 'function') { spec.commandName = commandName; spec.paramName = paramName; - spec.remoteLookup = (typeof this.lookup === 'function'); - spec.remoteData = (typeof this.data === 'function'); + spec.remoteLookup = true; } return spec; }; @@ -129,10 +128,6 @@ SelectionType.prototype.getLookup = function(context) { reply = this.front.getSelectionLookup(this.commandName, this.paramName); reply = resolve(reply, context); } - else if (this.remoteData) { - reply = this.front.getSelectionData(this.commandName, this.paramName); - reply = resolve(reply, context).then(this._dataToLookup); - } else if (typeof this.lookup === 'function') { reply = resolve(this.lookup.bind(this), context); } From 1053f7d2b035eeeb22a8f2c1f3cb0a982f7e8f47 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 20:47:26 +0000 Subject: [PATCH 48/63] runat-1128988: Remote the javascript type correctly. Can't transfer the global remotely. Signed-off-by: Joe Walker --- lib/gcli/types/javascript.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/gcli/types/javascript.js b/lib/gcli/types/javascript.js index 1cbad2c4..649da58e 100644 --- a/lib/gcli/types/javascript.js +++ b/lib/gcli/types/javascript.js @@ -31,8 +31,11 @@ function JavascriptType(typeSpec) { JavascriptType.prototype = Object.create(Type.prototype); -JavascriptType.prototype.getSpec = function() { - return 'javascript'; +JavascriptType.prototype.getSpec = function(commandName, paramName) { + return { + name: 'remote', + paramName: paramName + }; }; JavascriptType.prototype.stringify = function(value, context) { From 60a37bae51731a8f76b27b1100384bf55c168fac Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 21:03:15 +0000 Subject: [PATCH 49/63] runat-1128988: setTimeout is undefined in the content process. Signed-off-by: Joe Walker --- lib/gcli/test/mockCommands.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index 9079fc2f..13fe1cb8 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -548,7 +548,7 @@ mockCommands.items = [ exec: function(args, context) { if (args.method === 'reject') { return new Promise(function(resolve, reject) { - setTimeout(function() { + context.environment.window.setTimeout(function() { reject('rejected promise'); }, 10); }); @@ -556,7 +556,7 @@ mockCommands.items = [ if (args.method === 'rejecttyped') { return new Promise(function(resolve, reject) { - setTimeout(function() { + context.environment.window.setTimeout(function() { reject(context.typedData('number', 54)); }, 10); }); @@ -564,7 +564,7 @@ mockCommands.items = [ if (args.method === 'throwinpromise') { return new Promise(function(resolve, reject) { - setTimeout(function() { + context.environment.window.setTimeout(function() { resolve('should be lost'); }, 10); }).then(function() { @@ -695,7 +695,7 @@ mockCommands.items = [ name: 'selection', data: function(context) { return new Promise(function(resolve, reject) { - setTimeout(function() { + context.environment.window.setTimeout(function() { resolve([ 'Shalom', 'Namasté', 'Hallo', 'Dydd-da', 'Chào', 'Hej', 'Saluton', 'Sawubona' From a037767c43e06bad09b556a34a1a2c91e5df05a2 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 21:04:29 +0000 Subject: [PATCH 50/63] runat-1128988: Add mock command to help testing remote resources Signed-off-by: Joe Walker --- lib/gcli/test/mockCommands.js | 11 +++++++++++ lib/gcli/test/testResource.js | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/lib/gcli/test/mockCommands.js b/lib/gcli/test/mockCommands.js index 13fe1cb8..4148b075 100644 --- a/lib/gcli/test/mockCommands.js +++ b/lib/gcli/test/mockCommands.js @@ -778,5 +778,16 @@ mockCommands.items = [ exec: function(args, context) { return args; } + }, + { + item: 'command', + name: 'tsres', + params: [ + { + name: 'resource', + type: 'resource' + } + ], + exec: createExec('tsres'), } ]; diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index 84b250a8..631e6d29 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -16,6 +16,7 @@ 'use strict'; +var helpers = require('./helpers'); var assert = require('../testharness/assert'); var Promise = require('../util/promise').Promise; @@ -23,6 +24,17 @@ var util = require('../util/util'); var resource = require('../types/resource'); var Status = require('../types/types').Status; +exports.testCommand = function(options) { + return helpers.audit(options, [ + { + setup: 'tsres ', + check: { + predictionsContains: [ 'inline-css' ], + } + } + ]); +}; + exports.testAllPredictions1 = function(options) { if (options.isFirefox || options.isNode) { assert.log('Skipping checks due to firefox document.stylsheets support.'); From b34b96f60a44b3c0254829bc51413eda1c055cc7 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 21:05:37 +0000 Subject: [PATCH 51/63] runat-1128988: Remove values from tests that now run remotely Signed-off-by: Joe Walker --- lib/gcli/test/testAsync.js | 3 --- lib/gcli/test/testExec.js | 1 - 2 files changed, 4 deletions(-) diff --git a/lib/gcli/test/testAsync.js b/lib/gcli/test/testAsync.js index 2511113f..2f85fe52 100644 --- a/lib/gcli/test/testAsync.js +++ b/lib/gcli/test/testAsync.js @@ -50,7 +50,6 @@ exports.testBasic = function(options) { args: { command: { name: 'tsslow' }, hello: { - value: undefined, arg: '', status: 'INCOMPLETE' }, @@ -71,7 +70,6 @@ exports.testBasic = function(options) { args: { command: { name: 'tsslow' }, hello: { - value: undefined, arg: ' S', status: 'INCOMPLETE' }, @@ -92,7 +90,6 @@ exports.testBasic = function(options) { args: { command: { name: 'tsslow' }, hello: { - value: 'Shalom', arg: ' Shalom ', status: 'VALID', message: '' diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index 9fa06036..9874dc5c 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -358,7 +358,6 @@ exports.testExecScript = function(options) { args: { command: { name: 'tsj' }, javascript: { - value: '1 + 1', arg: ' { 1 + 1 }', status: 'VALID', message: '' From fa4ec3f8a0022fb6ce7840eb45363b815a88fdeb Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 21:09:00 +0000 Subject: [PATCH 52/63] runat-1128988: Update test skips for remote testing Lots of JS tests affected by how it's remoted, and the skips in testJs were on crack, but that's unused in Firefox so we're not too worried about that. Resource tests are now run remotely too, and we can't run tests locally on firefox. The new command test helps with that. Signed-off-by: Joe Walker --- lib/gcli/test/testJs.js | 13 +++++++------ lib/gcli/test/testKeyboard1.js | 13 ++++--------- lib/gcli/test/testResource.js | 22 ++++++++++++++++------ 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/lib/gcli/test/testJs.js b/lib/gcli/test/testJs.js index 23f772ac..21961ca6 100644 --- a/lib/gcli/test/testJs.js +++ b/lib/gcli/test/testJs.js @@ -20,7 +20,7 @@ var assert = require('../testharness/assert'); var helpers = require('./helpers'); exports.setup = function(options) { - if (!jsTestAllowed(options)) { + if (jsTestDisallowed(options)) { return; } @@ -38,22 +38,23 @@ exports.setup = function(options) { }; exports.shutdown = function(options) { - if (!jsTestAllowed(options)) { + if (jsTestDisallowed(options)) { return; } delete options.requisition.environment.window.donteval; }; -function jsTestAllowed(options) { +function jsTestDisallowed(options) { return options.isRemote || // Altering the environment (which isn't remoted) + options.isNode || options.requisition.system.commands.get('{') == null; } exports.testBasic = function(options) { return helpers.audit(options, [ { - skipRemainingIf: jsTestAllowed, + skipRemainingIf: jsTestDisallowed, setup: '{', check: { input: '{', @@ -208,7 +209,7 @@ exports.testBasic = function(options) { exports.testDocument = function(options) { return helpers.audit(options, [ { - skipRemainingIf: jsTestAllowed, + skipRemainingIf: jsTestDisallowed, setup: '{ docu', check: { input: '{ docu', @@ -444,7 +445,7 @@ exports.testDonteval = function(options) { exports.testExec = function(options) { return helpers.audit(options, [ { - skipRemainingIf: jsTestAllowed, + skipRemainingIf: jsTestDisallowed, setup: '{ 1+1', check: { input: '{ 1+1', diff --git a/lib/gcli/test/testKeyboard1.js b/lib/gcli/test/testKeyboard1.js index e6580c97..1f2cbeef 100644 --- a/lib/gcli/test/testKeyboard1.js +++ b/lib/gcli/test/testKeyboard1.js @@ -39,16 +39,12 @@ exports.testSimple = function(options) { exports.testScript = function(options) { return helpers.audit(options, [ { - skipIf: function commandJsMissing() { - return options.requisition.system.commands.get('{') == null; - }, + skipRemainingIf: options.isRemote || + options.requisition.system.commands.get('{') == null, setup: '{ wind', check: { input: '{ window' } }, { - skipIf: function commandJsMissing() { - return options.requisition.system.commands.get('{') == null; - }, setup: '{ window.docum', check: { input: '{ window.document' } } @@ -58,9 +54,8 @@ exports.testScript = function(options) { exports.testJsdom = function(options) { return helpers.audit(options, [ { - skipIf: function jsDomOrCommandJsMissing() { - return options.requisition.system.commands.get('{') == null; - }, + skipIf: options.isRemote || + options.requisition.system.commands.get('{') == null, setup: '{ window.document.titl', check: { input: '{ window.document.title ' } } diff --git a/lib/gcli/test/testResource.js b/lib/gcli/test/testResource.js index 631e6d29..aab8a434 100644 --- a/lib/gcli/test/testResource.js +++ b/lib/gcli/test/testResource.js @@ -36,8 +36,8 @@ exports.testCommand = function(options) { }; exports.testAllPredictions1 = function(options) { - if (options.isFirefox || options.isNode) { - assert.log('Skipping checks due to firefox document.stylsheets support.'); + if (options.isRemote) { + assert.log('Can\'t directly test remote types locally.'); return; } @@ -53,8 +53,8 @@ exports.testAllPredictions1 = function(options) { }; exports.testScriptPredictions = function(options) { - if (options.isFirefox || options.isNode) { - assert.log('Skipping checks due to firefox document.stylsheets support.'); + if (options.isRemote || options.isNode) { + assert.log('Can\'t directly test remote types locally.'); return; } @@ -71,8 +71,8 @@ exports.testScriptPredictions = function(options) { }; exports.testStylePredictions = function(options) { - if (options.isFirefox || options.isNode) { - assert.log('Skipping checks due to firefox document.stylsheets support.'); + if (options.isRemote) { + assert.log('Can\'t directly test remote types locally.'); return; } @@ -89,6 +89,11 @@ exports.testStylePredictions = function(options) { }; exports.testAllPredictions2 = function(options) { + if (options.isRemote) { + assert.log('Can\'t directly test remote types locally.'); + return; + } + var context = options.requisition.conversionContext; var types = options.requisition.system.types; @@ -107,6 +112,11 @@ exports.testAllPredictions2 = function(options) { }; exports.testAllPredictions3 = function(options) { + if (options.isRemote) { + assert.log('Can\'t directly test remote types locally.'); + return; + } + var context = options.requisition.conversionContext; var types = options.requisition.system.types; var res1 = types.createType({ name: 'resource' }); From b38877385c254c9d4e5ec8815ff4605e2ac098e9 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Thu, 19 Mar 2015 21:10:11 +0000 Subject: [PATCH 53/63] runat-1128988: Clone the typeSpec rather than try to clean up Clean up worked while things were less async. Then it was an accident waiting to happen. Signed-off-by: Joe Walker --- lib/gcli/test/testTypes.js | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/gcli/test/testTypes.js b/lib/gcli/test/testTypes.js index 2fef0fc6..9b392331 100644 --- a/lib/gcli/test/testTypes.js +++ b/lib/gcli/test/testTypes.js @@ -20,9 +20,11 @@ var assert = require('../testharness/assert'); var util = require('../util/util'); var Promise = require('../util/promise').Promise; -function forEachType(options, typeSpec, callback) { +function forEachType(options, templateTypeSpec, callback) { var types = options.requisition.system.types; return util.promiseEach(types.getTypeNames(), function(name) { + var typeSpec = {}; + util.copyProperties(templateTypeSpec, typeSpec); typeSpec.name = name; typeSpec.requisition = options.requisition; @@ -44,20 +46,15 @@ function forEachType(options, typeSpec, callback) { else if (name === 'union') { typeSpec.alternatives = [{ name: 'string' }]; } + else if (options.isRemote) { + if (name === 'node' || name === 'nodelist') { + return; + } + } var type = types.createType(typeSpec); var reply = callback(type); - return Promise.resolve(reply).then(function(value) { - // Clean up - delete typeSpec.name; - delete typeSpec.requisition; - delete typeSpec.data; - delete typeSpec.delegateType; - delete typeSpec.subtype; - delete typeSpec.alternatives; - - return value; - }); + return Promise.resolve(reply); }); } From 40c8105b2c6214bcfb3cb742a3baf8616d225461 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 14:26:40 +0000 Subject: [PATCH 54/63] runat-1128988: Add system.destroy() Just go through the loaded modules, calling unload on each. Signed-off-by: Joe Walker --- lib/gcli/system.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 429ecd68..f1d20889 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -219,6 +219,16 @@ exports.createSystem = function(options) { }.bind(this)); }, + destroy: function() { + this.commands.onCommandsChange.holdFire(); + + Object.keys(loadedModules).forEach(function(name) { + unloadModule(name); + }); + + this.commands.onCommandsChange.resumeFire(); + }, + toString: function() { return 'System [' + 'commands:' + components.command.getAll().length + ', ' + From 7bf1de9df580dc3a6f87b9678f1a38eaf0a857df Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 14:34:32 +0000 Subject: [PATCH 55/63] runat-1128988: Extract removeItemsFromFront to a separate function Signed-off-by: Joe Walker --- lib/gcli/system.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index f1d20889..d15f4cfe 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -301,13 +301,7 @@ exports.connectFront = function(system, front, customProps) { */ function syncItems(system, front, customProps) { return front.specs(customProps).then(function(specs) { - // Go through all the commands removing any that are associated with the - // given front. The method of association is the hack in addLocalFunctions. - system.commands.getAll().forEach(function(command) { - if (command.front === front) { - system.commands.remove(command); - } - }); + removeItemsFromFront(system, front); var remoteItems = addLocalFunctions(specs, front); system.addItems(remoteItems); @@ -325,7 +319,7 @@ function addLocalFunctions(specs, front) { // all the remote types specs.forEach(function(commandSpec) { // HACK: Tack the front to the command so we know how to remove it - // in syncItems() above + // in removeItemsFromFront() below commandSpec.front = front; // Tell the type instances for a command how to contact their counterparts @@ -353,3 +347,15 @@ function addLocalFunctions(specs, front) { return specs; } + +/** + * Go through all the commands removing any that are associated with the + * given front. The method of association is the hack in addLocalFunctions. + */ +function removeItemsFromFront(system, front) { + system.commands.getAll().forEach(function(command) { + if (command.front === front) { + system.commands.remove(command); + } + }); +} From e1f2f75452cee173dc8cb0b284dd5240ec629f82 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 14:40:29 +0000 Subject: [PATCH 56/63] runat-1128988: Introduce disconnectFront(system, front) Signed-off-by: Joe Walker --- lib/gcli/system.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index d15f4cfe..0bba6ed1 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -288,9 +288,10 @@ exports.createSystem = function(options) { * commands imported into the local system */ exports.connectFront = function(system, front, customProps) { - front.on('commands-changed', function() { + system._handleCommandsChanged = function() { syncItems(system, front, customProps).catch(util.errorHandler); - }); + }; + front.on('commands-changed', system._handleCommandsChanged); return syncItems(system, front, customProps); }; @@ -359,3 +360,12 @@ function removeItemsFromFront(system, front) { } }); } + +/** + * Undo the effect of #connectFront + */ +exports.disconnectFront = function(system, front) { + front.off('commands-changed', system._handleCommandsChanged); + system._handleCommandsChanged = undefined; + removeItemsFromFront(system, front); +}; From de3d7dbf31b1bc3d8f10b74dff35a8713a4d4890 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 16:21:37 +0000 Subject: [PATCH 57/63] runat-1128988: Review fix: Simplify testNode https://github.com/joewalker/gecko-dev/commit/53af47923e17b8d9b8844036c792fcde9d3c7a0d#commitcomment-10346841 Signed-off-by: Joe Walker --- lib/gcli/test/testNode.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/gcli/test/testNode.js b/lib/gcli/test/testNode.js index 7424ebaa..59e49d53 100644 --- a/lib/gcli/test/testNode.js +++ b/lib/gcli/test/testNode.js @@ -162,15 +162,13 @@ exports.testNodeDom = function(options) { nodes: { status: 'VALID' }, nodes2: { status: 'VALID' } } - } - }, - { - skipIf: options.isRemote, // arg values are unavailable remotely - setup: 'tse :root ', + }, exec: { }, post: function(output) { - assert.is(output.args.node.tagName, 'HTML', ':root tagName'); + if (!options.isRemote) { + assert.is(output.args.node.tagName, 'HTML', ':root tagName'); + } } }, { From 1860ace761e64b4a09d8ad40a8140c58e983690b Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 16:26:29 +0000 Subject: [PATCH 58/63] runat-1128988: Sync with Templater.jsm Remove invalid header comment. Enable working from frozen data objects. Use Function.apply rather than eval() Signed-off-by: Joe Walker --- lib/gcli/util/domtemplate.js | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/lib/gcli/util/domtemplate.js b/lib/gcli/util/domtemplate.js index 10323df0..94d7ea72 100644 --- a/lib/gcli/util/domtemplate.js +++ b/lib/gcli/util/domtemplate.js @@ -16,10 +16,6 @@ 'use strict'; -// WARNING: do not 'use strict' without reading the notes in envEval(); -// Also don't remove the 'do not use strict' marker. The orion build uses these -// markers to know where to insert AMD headers. - /** * For full documentation, see: * https://github.com/mozilla/domtemplate/blob/master/README.md @@ -350,24 +346,28 @@ function processForEachMember(state, member, templNode, siblingNode, data, param try { var cState = cloneState(state); handleAsync(member, siblingNode, function(reply, node) { - data[paramName] = reply; + // Clone data because we can't be sure that we can safely mutate it + var newData = Object.create(null); + Object.keys(data).forEach(function(key) { + newData[key] = data[key]; + }); + newData[paramName] = reply; if (node.parentNode != null) { var clone; if (templNode.nodeName.toLowerCase() === 'loop') { for (var i = 0; i < templNode.childNodes.length; i++) { clone = templNode.childNodes[i].cloneNode(true); node.parentNode.insertBefore(clone, node); - processNode(cState, clone, data); + processNode(cState, clone, newData); } } else { clone = templNode.cloneNode(true); clone.removeAttribute('foreach'); node.parentNode.insertBefore(clone, node); - processNode(cState, clone, data); + processNode(cState, clone, newData); } } - delete data[paramName]; }); } finally { @@ -536,10 +536,6 @@ function property(state, path, data, newValue) { /** * Like eval, but that creates a context of the variables in env in * which the script is evaluated. - * WARNING: This script uses 'with' which is generally regarded to be evil. - * The alternative is to create a Function at runtime that takes X parameters - * according to the X keys in the env object, and then call that function using - * the values in the env object. This is likely to be slow, but workable. * @param script The string to be evaluated. * @param data The environment in which to eval the script. * @param frame Optional debugging string in case of failure. @@ -567,12 +563,7 @@ function envEval(state, script, data, frame) { // keys in 'data' and with 'script' as its function body. // We then call this function with the values in 'data' var keys = allKeys(data); - var args = keys.join(', '); - var func = 'function(' + args + ') { return ' + script + '; }'; - - // In order to extract this function from the eval, we wrap it in an IIFE - func = '(function() { return ' + func + ' })();'; - func = eval(func); + var func = Function.apply(null, keys.concat("return " + script)); var values = keys.map(function(key) { return data[key]; }); return func.apply(null, values); @@ -584,10 +575,6 @@ function envEval(state, script, data, frame) { // code above, the global is null. (Using 'func.apply(data, values)' // changes 'this' in the 'foo()' frame, but not in the inside the body // of 'foo', so that wouldn't help) - - // TODO: Come ES6 we can probably do something like this: - // let func = new Function(...keys, 'return ' + script); - // return func(...keys.map(key => data[key])); } } catch (ex) { From 304318e32ac6d344ebbfdcdd302ee9dbbe8982cb Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 16:35:53 +0000 Subject: [PATCH 59/63] runat-1128988: Enable nested command execution Allow a command to execute another command. This means: * re-enable the deprecated updateExec function from executionContext * sidestep the problems of simultaneous executions by spinning up a new Requisition in _contextUpdateExec * add tests Signed-off-by: Joe Walker --- lib/gcli/cli.js | 13 +++++++++---- lib/gcli/test/testExec.js | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js index 33332cfe..0e7bab2d 100644 --- a/lib/gcli/cli.js +++ b/lib/gcli/cli.js @@ -575,11 +575,12 @@ Object.defineProperty(Requisition.prototype, 'executionContext', { enumerable: true }); + this._executionContext.updateExec = this._contextUpdateExec.bind(this); + if (legacy) { this._executionContext.createView = view.createView; this._executionContext.exec = this.exec.bind(this); this._executionContext.update = this._contextUpdate.bind(this); - this._executionContext.updateExec = this._contextUpdateExec.bind(this); Object.defineProperty(this._executionContext, 'document', { get: function() { return requisition.document; }, @@ -2085,10 +2086,14 @@ Requisition.prototype.exec = function(options) { * unexpected change to the current command. */ Requisition.prototype._contextUpdateExec = function(typed, options) { - return this.updateExec(typed, options).then(function(reply) { - this.onExternalUpdate({ typed: typed }); + var reqOpts = { + document: this.document, + environment: this.environment + }; + var child = new Requisition(this.system, reqOpts); + return child.updateExec(typed, options).then(function(reply) { return reply; - }.bind(this)); + }.bind(child)); }; /** diff --git a/lib/gcli/test/testExec.js b/lib/gcli/test/testExec.js index 9874dc5c..ab65bfdb 100644 --- a/lib/gcli/test/testExec.js +++ b/lib/gcli/test/testExec.js @@ -605,5 +605,43 @@ exports.testExecDefaults = function(options) { } } ]); +}; + +exports.testNested = function(options) { + var commands = options.requisition.system.commands; + commands.add({ + name: 'nestorama', + exec: function(args, context) { + return context.updateExec('tsb').then(function(tsbOutput) { + return context.updateExec('tsu 6').then(function(tsuOutput) { + return JSON.stringify({ + tsb: tsbOutput.data, + tsu: tsuOutput.data + }); + }); + }); + } + }); + return helpers.audit(options, [ + { + setup: 'nestorama', + exec: { + output: + '{' + + '"tsb":{' + + '"name":"tsb",' + + '"args":{"toggle":"false"}' + + '},' + + '"tsu":{' + + '"name":"tsu",' + + '"args":{"num":"6"}' + + '}' + + '}' + }, + post: function() { + commands.remove('nestorama'); + } + } + ]); }; From d46ff21da754ffe0ad91a601bfee29c1d973bb09 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Wed, 25 Mar 2015 16:36:52 +0000 Subject: [PATCH 60/63] runat-1128988: Disallow using once with a callback Signed-off-by: Joe Walker --- lib/gcli/util/util.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gcli/util/util.js b/lib/gcli/util/util.js index fb9ae1da..19003b17 100644 --- a/lib/gcli/util/util.js +++ b/lib/gcli/util/util.js @@ -164,6 +164,10 @@ exports.createEvent = function(name) { * Fire an event just once using a promise. */ event.once = function() { + if (arguments.length !== 0) { + throw new Error('event.once uses promise return values'); + } + return new Promise(function(resolve, reject) { var handler = function(arg) { event.remove(handler); From 44fdb0009be4e3b39a236f96eb5e369c43a67969 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Mon, 13 Apr 2015 08:28:17 +0100 Subject: [PATCH 61/63] runat-1128988: Re-order functions https://github.com/joewalker/gecko-dev/pull/11#commitcomment-10632175 Signed-off-by: Joe Walker --- lib/gcli/system.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/gcli/system.js b/lib/gcli/system.js index 0bba6ed1..59d861a2 100644 --- a/lib/gcli/system.js +++ b/lib/gcli/system.js @@ -296,6 +296,15 @@ exports.connectFront = function(system, front, customProps) { return syncItems(system, front, customProps); }; +/** + * Undo the effect of #connectFront + */ +exports.disconnectFront = function(system, front) { + front.off('commands-changed', system._handleCommandsChanged); + system._handleCommandsChanged = undefined; + removeItemsFromFront(system, front); +}; + /** * Remove the items in this system that came from a previous sync action, and * re-add them. See connectFront() for explanation of properties @@ -360,12 +369,3 @@ function removeItemsFromFront(system, front) { } }); } - -/** - * Undo the effect of #connectFront - */ -exports.disconnectFront = function(system, front) { - front.off('commands-changed', system._handleCommandsChanged); - system._handleCommandsChanged = undefined; - removeItemsFromFront(system, front); -}; From ee9b89fa9d5ba86ee86e855b45c76fb752fcf37d Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Mon, 13 Apr 2015 08:29:51 +0100 Subject: [PATCH 62/63] runat-1128988: Format JSON output better This gives us formatted output rather than the default compressed output. Signed-off-by: Joe Walker --- lib/gcli/converters/basic.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gcli/converters/basic.js b/lib/gcli/converters/basic.js index 7ce0848f..3cb448e9 100644 --- a/lib/gcli/converters/basic.js +++ b/lib/gcli/converters/basic.js @@ -88,7 +88,7 @@ exports.items = [ from: 'json', to: 'string', exec: function(json, conversionContext) { - return JSON.stringify(json); + return JSON.stringify(json, null, ' '); } } ]; From 9330099d740ca865093a457549a00cbce21087fe Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Mon, 13 Apr 2015 10:47:34 +0100 Subject: [PATCH 63/63] runat-1128988: Implement Task.spawn for the web This code doesn't reach mozilla-central. No need for detailed review. It's essentially a *much* shorter version of Task.spawn() from https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm Signed-off-by: Joe Walker --- lib/gcli/util/host.js | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/gcli/util/host.js b/lib/gcli/util/host.js index c9dd46b8..92d9dbb1 100644 --- a/lib/gcli/util/host.js +++ b/lib/gcli/util/host.js @@ -119,7 +119,29 @@ exports.spawn = function(context, spawnSpec) { * @return a promise of whatever task() returns */ exports.exec = function(task) { - return Promise.resolve(task()); + var iterator = task(); + + return new Promise(function(resolve, reject) { + // If task wasn't a generator function, resolve with whatever + if (iterator == null || + iterator.constructor.constructor.name !== 'GeneratorFunction') { + resolve(iterator); + return; + } + + var callNext = function(lastValue) { + var iteration = iterator.next(lastValue); + Promise.resolve(iteration.value).then(function(value) { + var action = (iteration.done ? resolve : callNext); + action(value); + }).catch(function(error) { + reject(error); + iterator['throw'](error); + }); + }; + + callNext(undefined); + }); }; /**