Implement extracting action chain from a request

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: maja_zf, Assigned: maja_zf)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

This is my first rough shot at implementing (part of) the spec, writing tests as I go. Before I go any further, I'd like to hear what you think. Thanks!
Attachment #8791840 - Flags: feedback?(james)
Attachment #8791840 - Flags: feedback?(ato)
As promised, a n-i for David, too. Any comments?
Flags: needinfo?(dburns)
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

This reads pleasingly like the spec :) Seems like a good start to me but I'm not familiar with the existing code.
Attachment #8791840 - Flags: feedback?(james) → feedback+
This looks like a brilliant start
Flags: needinfo?(dburns)
Comment hidden (mozreview-request)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

This is a great start!

Besides the nits, I have concerns about the data model and the unmarshaling of the JSON input data.  After studying the patch carefully, it seems (mostly) technically correct, but I think we can make improvements so that it looks less like a verbatim implementation of the spec and more like code that will interface well with the rest of Marionette.

I do think this is the right amount of work to land in a single patch.  However, I suggest making this patch _replace_ the existing _testing/marionette/action.js_ file (see issue).  When doing that, I would prefer the renaming of _action.js_ to _legacyAction.js_ to happen in a separate commit.

As I was reading the spec and this patch side by side, I also discovered a bunch of issues with the spec.  So this is going to be a great thing for rooting out bugs there too.

::: testing/marionette/jar.mn:9
(Diff revision 2)
>  
>  marionette.jar:
>  % content marionette %content/
>    content/server.js (server.js)
>    content/driver.js (driver.js)
> +  content/webdriverAction.js (webdriverAction.js)

I would reverse the relationship and name this actions.js and instead rename the existing to legacyaction.js or something.

Marionette intends to be a standards-conforming implementation of W3C WebDriver, and the existing actions API implementation is non-conforming.

::: testing/marionette/test_actions.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please rename file to _test_action.js_ to match the source file.

::: testing/marionette/test_actions.js:6
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {utils: Cu} = Components;
> +

`"use strict";`

::: testing/marionette/unit.ini:14
(Diff revision 2)
>  
>  [test_element.js]
>  [test_error.js]
>  [test_message.js]
>  [test_navigate.js]
> +[test_actions.js]

Sort lexicographically.

::: testing/marionette/webdriverAction.js:15
(Diff revision 2)
> +
> +Cu.import("chrome://marionette/content/element.js");
> +Cu.import("chrome://marionette/content/error.js");
> +
> +
> +this.EXPORTED_SYMBOLS = ["webdriveraction", "inputStateMap"];

Have each file only export one symbol.

::: testing/marionette/webdriverAction.js:25
(Diff revision 2)
> +// TODO associate with session
> +// populated by "dispatch tick actions"?
> +this.inputStateMap = new Map();
> +
> +
> +const ACTION_TYPES = new Set([

Export:

```js
action.Types = new Set(["none", "key", "pointer"]);
```

::: testing/marionette/webdriverAction.js:31
(Diff revision 2)
> +  "none",
> +  "key",
> +  "pointer",
> +]);
> +
> +this.webdriveraction = {}

Missing `;`

::: testing/marionette/webdriverAction.js:70
(Diff revision 2)
> +  this.primary = primary;
> +  this.x = 0;
> +  this.y = 0;
> +}
> +
> +webdriveraction.PointerInputState.prototype.toString = function(){

Throughout this patch, put a space before `{`.

::: testing/marionette/webdriverAction.js:72
(Diff revision 2)
> +  this.y = 0;
> +}
> +
> +webdriveraction.PointerInputState.prototype.toString = function(){
> +  return '[object PointerInputState]';
> +}

Since these statements are _assignments_, you also need to end them with `;`.

::: testing/marionette/webdriverAction.js:88
(Diff revision 2)
> +  this.type = type;
> +  this.subtype = subtype;
> +};
> +
> +// actions: list of action sequences
> +webdriveraction.extractActionChain = function extractActionChain(actions) {

Throughout, it’s unnecessary to give the function a literate name like this:

```js
action.extractActionChain = function THIS_PART_IS_SUPERFLUOUS(actions) { … }
```

I would probably also consider creating an _ActionChain_ class that provides a static _fromJson_ method.

::: testing/marionette/webdriverAction.js:90
(Diff revision 2)
> +};
> +
> +// actions: list of action sequences
> +webdriveraction.extractActionChain = function extractActionChain(actions) {
> +  // TODO check current browsing context?
> +  if (typeof actions === 'undefined' || !Array.isArray(actions)){

This type check can be removed because `Array.isArray` deals with the `undefined` value.

::: testing/marionette/webdriverAction.js:91
(Diff revision 2)
> +
> +// actions: list of action sequences
> +webdriveraction.extractActionChain = function extractActionChain(actions) {
> +  // TODO check current browsing context?
> +  if (typeof actions === 'undefined' || !Array.isArray(actions)){
> +    throw new InvalidArgumentError(`'actions' ${actions} is not an Array`);

Throughout this patch, use Unix-style error messages where the input is mentioned after a colon at the end of the message:

```js
throw new InvalidArgumentError("Expected 'actions' to be an array, got: " + actions);
```

Or some message similar to that.

::: testing/marionette/webdriverAction.js:96
(Diff revision 2)
> +    throw new InvalidArgumentError(`'actions' ${actions} is not an Array`);
> +  }
> +  let actionsByTick = [];
> +  for (let actionSequence of actions){
> +    let inputSourceActions = webdriveraction.processInputSourceActionSequence(actionSequence);
> +    for (var i = 0; i < inputSourceActions.length; i++){

`let`

::: testing/marionette/webdriverAction.js:98
(Diff revision 2)
> +  let actionsByTick = [];
> +  for (let actionSequence of actions){
> +    let inputSourceActions = webdriveraction.processInputSourceActionSequence(actionSequence);
> +    for (var i = 0; i < inputSourceActions.length; i++){
> +      if (actionsByTick.length < i + 1){
> +        // new tick

Move outside of if-condition as it describes what happens in the line above.

::: testing/marionette/webdriverAction.js:108
(Diff revision 2)
> +  }
> +  return actionsByTick;
> +};
> +
> +// action_sequence has a list of actionItems for one input source
> +webdriveraction.processInputSourceActionSequence = function processInputSourceActionSequence(

This is a very literal implementation of the specification text.  We can think of the processing steps for actions as JSON unmarshaling procedures into JS objects that represents them.

The data model for actions can be shared between the null-, key-, pointer-, and pause actions.  In a sane programming language we could achieve this using type embedding to share an interface between all of them, but allowing each to have their own characteristics.

We can model this by using an inheritance model, where _PointerUpAction_ extends _PointerAction_ extends _Action_.  But the more I think about it, the less I like the complexity.  Since we can represent structs directly as literate objects in JS, why don’t we use them?

As we can tell from the [‘dispatch tick actions’ algorithm](http://w3c.github.io/webdriver/webdriver-spec.html#dfn-dispatch-tick-actions) we end up mapping down to one dispatch action per subtype, so we can make an optimisation to leave out the input class type when we read in the data.  There’s no requirement that the implementation follows the specification’s algorithms in a verbatively.

One can imagine simplifying this to:

```js
action = {
  Pause: "pause",
  KeyDown: "keyDown",
  KeyUp: "keyUp",
  PointerDown: "pointerDown",
  PointerUp: "pointerUp",
  PointerMove: "pointerMove",
  PointerCancel: "pointerCancel",
};

const ACTIONS = {
  none: new Set([Pause]),
  key: new Set([Pause, KeyDown, KeyUp]),
  pointer: new Set([
    Pause,
    PointerDown,
    PointerUp,
    PointerMove,
    PointerCancel,
  ]),
]);

// No explicit constructor for this
// because it's too easy to mess up
action.Action = class {  // could be called action.Item or action.Object or whatever…
  toString() {
    return '[action ${this.type}]';
  }

  static from(json) {
    let subtypes = ACTIONS[json.type];
    if (!subtypes) {
      throw new InvalidArgumentError("Unknown type: " + json.type);
    }

    if (!subtypes.has(json.subtype)) {
      throw new InvalidArgumentError("Unknown subtype: " + json.subtype);
    }

    let item = new action.Item();
    item.type = subtype;

    switch (item.type) {
      case action.KeyUp:
      case action.KeyDown:
        assertDefined(json.key);
        // need to write this utility
        if (unicode.countGraphemes(json.key) != 1) {
          throw new InvalidArgumentError();
        }
        item.key = json.key;
        break;

      case action.PointerDown:
      case action.PointerUp:
        assertDefined(json.button);
        assertPositiveInteger(json.button);
        item.button = json.button;
        break;
      
      case action.PointerMove:
        assertDefined(json.duration);
        assertPositiveInteger(json.duration);
        item.duration = json.duration;
      
        assertDefined(json.element);
        if (!element.isWebElementReference(json.element)) {
          throw new InvalidArgumentError();
        }
        item.element = json.element;

        assertDefined(x);
        assertPositiveInteger(x);
        item.x = json.x;
      
        assertDefined(y),
        assertPositiveInteger(y);
        item.y = json.y;

        break;

      case action.PointerCancel:
        // TODO
        break;

      case action.Pause:
        assertDefined(json.duration);
        assertPositiveInteger(json.duration);
        item.duration = json.duration;
        break;
    }

    return item;
  }
};

function assertDefined(obj) { … }
function assertPositiveInteger(obj) { … }
```

::: testing/marionette/webdriverAction.js:112
(Diff revision 2)
> +    throw new InvalidArgumentError(`'actionSequence.type' must be one of
> +                                   ${Array.from(ACTION_TYPES)}`);

Just say that the action sequence type was invalid.

::: testing/marionette/webdriverAction.js:116
(Diff revision 2)
> +  if (!ACTION_TYPES.has(type)){
> +    throw new InvalidArgumentError(`'actionSequence.type' must be one of
> +                                   ${Array.from(ACTION_TYPES)}`);
> +  }
> +  let id = actionSequence.id;
> +  if (typeof id === 'undefined'){

Throughout, triple `===` for `typeof` checks are superfluous because the return type is guaranteed to be a string.

::: testing/marionette/webdriverAction.js:118
(Diff revision 2)
> +  }
> +  else if (typeof id !== 'string'){

Bring `else if` up behind the `}` on :118.

::: testing/marionette/webdriverAction.js:127
(Diff revision 2)
> +  let actionItems = actionSequence.actions;
> +  if (!Array.isArray(actionItems)){
> +    throw new InvalidArgumentError(`'actionSequence.actions' ${actionSequence.actions} is not an Array`);
> +  }
> +
> +  let parameters;

I think we should rename this to _pointerParams_ because it is specific to actions on the virtual pointer device.  We don’t send paramters to the other devices.

::: testing/marionette/webdriverAction.js:132
(Diff revision 2)
> +  let parameters;
> +  if (type === "pointer"){
> +    parameters = webdriveraction.processPointerParameters(actionSequence.parameters);
> +  }
> +
> +  if (inputStateMap.has(id) && !(inputStateMap.get(id) instanceof ACTION_INPUT_STATE[type])){

`instanceof` is notorious because it doesn’t compare objects across browser- or process boundaries.

I think it would be better to implement an equality method on the input state object so that we can achieve the following:

```js
let type = InputState.fromJson(sequence.type);
if (inputState.has(id) && !inputState.get(id).is(type)) {
  throw new InvalidArgumentError(…);
}
```

A possible implementation:

```js
class InputState {
  constructor() {
    this.type = this.constructor.name;
  }

  is(other) {
    return this.type == other.type;
  }

  toString() {
    return `[object ${this.type}InputState]`;
  }

  static fromJson(json) {
    if (typeof json != "string" || !(json in action.InputState)) {
      throw InvalidArgumentError();
    }
    return new action.InputState[json]();
  }
}

action.InputState.Key = class extends InputState {
  constructor() {
    this.pressed = new Set();
    this.alt = false;
    …
  }
};
```

::: testing/marionette/webdriverAction.js:134
(Diff revision 2)
> +    parameters = webdriveraction.processPointerParameters(actionSequence.parameters);
> +  }
> +
> +  if (inputStateMap.has(id) && !(inputStateMap.get(id) instanceof ACTION_INPUT_STATE[type])){
> +    throw new InvalidArgumentError(
> +      `${id} is already mapped to ${inputStateMap.get(id)}`);

Good use of template strings, but another style nit is that the second line of multiline statements should have four spaces for indentations to set them apart form a new block.

::: testing/marionette/webdriverAction.js:139
(Diff revision 2)
> +    if (Object.prototype.toString.call(actionItem) !== "[object Object]"){
> +      throw new InvalidArgumentError(`${actionItem} is not an object`);
> +    }

Drop this.  In JS everything is an object, and we do the correct JSON type checks in geckodriver.

::: testing/marionette/webdriverAction.js:144
(Diff revision 2)
> +    if (Object.prototype.toString.call(actionItem) !== "[object Object]"){
> +      throw new InvalidArgumentError(`${actionItem} is not an object`);
> +    }
> +    let action = {};
> +    switch(type){
> +      case("none"):

Drop paranthesis around cases.

::: testing/marionette/webdriverAction.js:170
(Diff revision 2)
> +  let pointerType = parametersData.pointerType;
> +  if (typeof pointerType !== 'undefined'){
> +    let types = ["mouse", "pen", "touch"];
> +    if(!types.includes(pointerType)){
> +      throw new InvalidArgumentError(`pointerType must be one of ${types}`);
> +    }
> +    parameters.pointerType = pointerType;
> +  }

I think we should replace this with a formalised type:

```js
action.PointerType = {
  Mouse: "mouse",
  Pen: "pen",
  Touch: "touch",
};
action.PointerType.get = function(str) {
  // many more clever ways to write lookup tables, but one approach:
  switch (str) {
    case "mouse":
      return PointerType.Mouse;
    case "pen":
      return PointerType.Pen;
    case "touch":
      return PointerType.Touch;
    default:
      throw new InvalidArgumentError();
  }
};

…

params.pointerType = PointerType.get(json.pointerType);
```

::: testing/marionette/webdriverAction.js:178
(Diff revision 2)
> +  let primary = parametersData.primary;
> +  if (typeof primary !== 'undefined'){
> +    if (typeof primary !== 'boolean'){
> +      throw new InvalidArgumentError(`primary ${primary} must be a boolean`);
> +    }
> +    parameters.primary = primary;
> +  }

Introduce a helper:

```js
function assertBoolean(obj) {
  if (!typeof obj == "boolean") {
    throw new InvalidArgumentError();
  }
}
```

::: testing/marionette/webdriverAction.js:281
(Diff revision 2)
> +    action.y = webdriveraction.validatePositiveNum(actionItem.y, 'y');
> +  }
> +};
> +
> +// helpers
> +webdriveraction.validatePositiveNum = function validatePositiveNum(value, name){

For functions that which primary purpose is to throw internally should highlight this in the name.

Rename this _assertPositiveInteger_.

Also I don’t think we should export it.

Also make the `name` argument optional, like this:

```js
function assertPositiveInteger(value, name = undefined) {
```

::: testing/marionette/webdriverAction.js:282
(Diff revision 2)
> +  }
> +};
> +
> +// helpers
> +webdriveraction.validatePositiveNum = function validatePositiveNum(value, name){
> +  let v = parseInt(value);

This allows `value` to be any type, so we shouldn’t use `parseInt`.  As I understand it the purpose here is to check that an input value is a positive integer.

::: testing/marionette/webdriverAction.js:283
(Diff revision 2)
> +};
> +
> +// helpers
> +webdriveraction.validatePositiveNum = function validatePositiveNum(value, name){
> +  let v = parseInt(value);
> +  if (isNaN(v) || v < 0){

`isNaN` is not an integer check.  Use `Number.isInteger`.

Also `v < 0` checks that it is negative; not positive.

::: testing/marionette/webdriverAction.js:286
(Diff revision 2)
> +webdriveraction.validatePositiveNum = function validatePositiveNum(value, name){
> +  let v = parseInt(value);
> +  if (isNaN(v) || v < 0){
> +    throw new InvalidArgumentError(`Value of '${name}' should be integer >= 0`);
> +  }
> +  return v;

Remove return type.

Comment 8

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80316
Attachment #8791840 - Flags: review?(ato)

Comment 9

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> Please rename file to _test_action.js_ to match the source file.

I forgot that you also need to add it to _testing/marionette/unit.ini_.
Assignee

Comment 10

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> I forgot that you also need to add it to _testing/marionette/unit.ini_.

test_action(s).js was already there. If your comment meant to refer to a different ini path, let me know.
Assignee

Comment 11

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> Throughout this patch, put a space before `{`.

Is there a linter I should be using?
Assignee

Comment 12

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> Throughout, it’s unnecessary to give the function a literate name like this:
> 
> ```js
> action.extractActionChain = function THIS_PART_IS_SUPERFLUOUS(actions) { … }
> ```
> 
> I would probably also consider creating an _ActionChain_ class that provides a static _fromJson_ method.

> Throughout, it’s unnecessary to give the function a literate name

Right. I added those in so I can use `fn.name` in the logging in test_action.js while allowing the test code to be less repetitive. Please reopen if this is a problem.

> `isNaN` is not an integer check.  Use `Number.isInteger`.
> 
> Also `v < 0` checks that it is negative; not positive.

Ok, thanks. FYI, I followed this pattern (parseInt, isNaN) based on driver.js

Comment 13

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> test_action(s).js was already there. If your comment meant to refer to a different ini path, let me know.

Yes, I see it now.

> Is there a linter I should be using?

There is no linter or formatter for this code, I’m sad to say.

Comment 14

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> Ok, thanks. FYI, I followed this pattern (parseInt, isNaN) based on driver.js

It’s safe to assume that their uses are bugs.  The specification talks mainly about integers and 32-bit unsigned integers.

I recently fixed one such mistake in GeckoDriver#setTimeouts: https://reviewboard.mozilla.org/r/78740/diff/4#index_header
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review80234

> This is a very literal implementation of the specification text.  We can think of the processing steps for actions as JSON unmarshaling procedures into JS objects that represents them.
> 
> The data model for actions can be shared between the null-, key-, pointer-, and pause actions.  In a sane programming language we could achieve this using type embedding to share an interface between all of them, but allowing each to have their own characteristics.
> 
> We can model this by using an inheritance model, where _PointerUpAction_ extends _PointerAction_ extends _Action_.  But the more I think about it, the less I like the complexity.  Since we can represent structs directly as literate objects in JS, why don’t we use them?
> 
> As we can tell from the [‘dispatch tick actions’ algorithm](http://w3c.github.io/webdriver/webdriver-spec.html#dfn-dispatch-tick-actions) we end up mapping down to one dispatch action per subtype, so we can make an optimisation to leave out the input class type when we read in the data.  There’s no requirement that the implementation follows the specification’s algorithms in a verbatively.
> 
> One can imagine simplifying this to:
> 
> ```js
> action = {
>   Pause: "pause",
>   KeyDown: "keyDown",
>   KeyUp: "keyUp",
>   PointerDown: "pointerDown",
>   PointerUp: "pointerUp",
>   PointerMove: "pointerMove",
>   PointerCancel: "pointerCancel",
> };
> 
> const ACTIONS = {
>   none: new Set([Pause]),
>   key: new Set([Pause, KeyDown, KeyUp]),
>   pointer: new Set([
>     Pause,
>     PointerDown,
>     PointerUp,
>     PointerMove,
>     PointerCancel,
>   ]),
> ]);
> 
> // No explicit constructor for this
> // because it's too easy to mess up
> action.Action = class {  // could be called action.Item or action.Object or whatever…
>   toString() {
>     return '[action ${this.type}]';
>   }
> 
>   static from(json) {
>     let subtypes = ACTIONS[json.type];
>     if (!subtypes) {
>       throw new InvalidArgumentError("Unknown type: " + json.type);
>     }
> 
>     if (!subtypes.has(json.subtype)) {
>       throw new InvalidArgumentError("Unknown subtype: " + json.subtype);
>     }
> 
>     let item = new action.Item();
>     item.type = subtype;
> 
>     switch (item.type) {
>       case action.KeyUp:
>       case action.KeyDown:
>         assertDefined(json.key);
>         // need to write this utility
>         if (unicode.countGraphemes(json.key) != 1) {
>           throw new InvalidArgumentError();
>         }
>         item.key = json.key;
>         break;
> 
>       case action.PointerDown:
>       case action.PointerUp:
>         assertDefined(json.button);
>         assertPositiveInteger(json.button);
>         item.button = json.button;
>         break;
>       
>       case action.PointerMove:
>         assertDefined(json.duration);
>         assertPositiveInteger(json.duration);
>         item.duration = json.duration;
>       
>         assertDefined(json.element);
>         if (!element.isWebElementReference(json.element)) {
>           throw new InvalidArgumentError();
>         }
>         item.element = json.element;
> 
>         assertDefined(x);
>         assertPositiveInteger(x);
>         item.x = json.x;
>       
>         assertDefined(y),
>         assertPositiveInteger(y);
>         item.y = json.y;
> 
>         break;
> 
>       case action.PointerCancel:
>         // TODO
>         break;
> 
>       case action.Pause:
>         assertDefined(json.duration);
>         assertPositiveInteger(json.duration);
>         item.duration = json.duration;
>         break;
>     }
> 
>     return item;
>   }
> };
> 
> function assertDefined(obj) { … }
> function assertPositiveInteger(obj) { … }
> ```

I've implemented some approximation of this idea. The action processing also relies on type/pointerParams from actionSequence, and a possibly-fresh id, so the end result feels a but clumsy. Thank goodness for tests.
Attachment #8798703 - Flags: review?(james)
Attachment #8798703 - Flags: review?(ato)
Attachment #8798704 - Flags: review?(james)
Attachment #8798704 - Flags: review?(ato)
Attachment #8798705 - Flags: review?(james)
Attachment #8798705 - Flags: review?(ato)
Attachment #8798706 - Flags: review?(james)
Attachment #8798706 - Flags: review?(ato)

Comment 21

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79106/#review84160

This is great progress!  Very impressed with the tests.  I should point out right away that most of the issues I’ve raised are nitpicks.

I feel greater consideration should be taken for what APIs is exposed to consumers of the `action` module.  I find it helps thinking about how listener.js will be using it.  The two basic needs, seen from this point of view, is to deserialise some input data (‘processing’ in specalese) and to dispatch a sequence of actions by ticks.

The second consideration is testability, i.e. we don’t want to make it unneccessarily cumbersome to write good tests.  So the published API need to be verbose enough so that testing does not feel like a black box environment where entire action chains needs to be constructed to test a single parameter.

The patch already does a good job of providing an unmarshaling function for single actions in `Action.fromJson`, and I think it’s worthwhile to carry this idea through to the other main concepts such as chains, sequences, ticks, and pointer parameters.

All a public consumer might want to do is:

```js
GeckoDriver.prototype.actions = function(cmd, resp) {
  …
  let byTick = action.Chain.fromJson(cmd.parameters);
  action.dispatch(byTick);
};
```

::: testing/marionette/action.js:47
(Diff revision 3)
> -      event.sendKeyUp(pack[1], keyModifiers, this.container.frame);
> -      this.actions(chain, touchId, i, keyModifiers, cb);
> -      break;
> +  Mouse: "mouse",
> +  Pen: "pen",
> +  Touch: "touch",
> +};
>  
> -    case "click":
> +action.PointerType.get = function(str){

Space before `{`.

::: testing/marionette/action.js:48
(Diff revision 3)
> -      el = this.seenEls.get(pack[1], this.container);
> -      let button = pack[2];
> +  let name = capitalize(str);
> +  if (!(capitalize(str) in this)) {

Second call to `capitalize` should be unnecessary here.

::: testing/marionette/action.js:51
(Diff revision 3)
> -      }
> +  }
> -      this.actions(chain, touchId, i, keyModifiers, cb);
> +  else {

Pull `else` up one line.

::: testing/marionette/action.js:77
(Diff revision 3)
> -      this.generateEvents("move", c.x, c.y, touchId, null, keyModifiers);
> +  }
> -      this.actions(chain, touchId, i, keyModifiers, cb);
> -      break;
>  
> -    case "moveByOffset":
> -      this.generateEvents(
> +  is(other) {
> +    return this.type == other.type;

We should probably use `===` here.

::: testing/marionette/action.js:109
(Diff revision 3)
>    }
>  };
>  
> -/**
> - * Given an element and a pair of coordinates, returns an array of the
> - * form [clientX, clientY, pageX, pageY, screenX, screenY].
> +action.InputState.Null = class extends InputState {
> +  constructor() {
> +    super();

I guess since we have to call `super` anyway, we could just pass `"Null`" on so you wouldn’t have to do so much magic in `InputState`.

I don’t feel strongly about this.

::: testing/marionette/action.js:110
(Diff revision 3)
>  };
>  
> -/**
> - * Given an element and a pair of coordinates, returns an array of the
> - * form [clientX, clientY, pageX, pageY, screenX, screenY].
> - */
> +action.InputState.Null = class extends InputState {
> +  constructor() {
> +    super();
> +    this.type = 'none';

Double quotes.

::: testing/marionette/action.js:128
(Diff revision 3)
> -      break;
> +};
> +
> +action.Action = class {
> +  constructor(id, type, subtype) {
> +    // represents action object for actionByTick
> +    if ([id,type,subtype].includes(undefined)){

Spaces after commas and space before `{`.

::: testing/marionette/action.js:140
(Diff revision 3)
> +  static fromJson(actionSequence, actionItem, id) {
> +    let type = actionSequence.type;

`id` is included in `actionSequence.id`, so it’s unnecessary to pass it in as a separate argument.

::: testing/marionette/action.js:153
(Diff revision 3)
> -      break;
> +      action.processPointerAction(id,
> +          action.processPointerParameters(actionSequence.parameters), item);

I would put the `processPointerAction` function inside this if-condition and make the `processPointerParameters` function a separete type:

```js
if (type === "pointer") {
  let params = PointerParameters.fromJson(actionSequence.parameters);
  if (action.inputStateMap.get(id).subtype !== item.subtype) {
    throw new InvalidArgumentError();
  }
  item.pointerType = params.pointerType;
  item.primary = params.primary;
}
```

::: testing/marionette/action.js:160
(Diff revision 3)
> +    }
>  
> -    case "cancel":
> -      this.isTap = false;
> -      if (this.mouseEventsOnly) {
> -        let [x, y] = this.lastCoordinates;
> +    switch (item.subtype) {
> +      case action.KeyUp:
> +      case action.KeyDown:
> +        let key = actionItem.value

Missing `;`.

::: testing/marionette/action.js:163
(Diff revision 3)
> -      this.isTap = false;
> -      if (this.mouseEventsOnly) {
> -        let [x, y] = this.lastCoordinates;
> -        this.emitMouseEvent(doc, "mouseup", x, y, null, null, keyModifiers);
> -      } else {
> -        this.touchProvider.emitTouchEvent("touchcancel", this.touchIds[touchId]);
> +      case action.KeyUp:
> +      case action.KeyDown:
> +        let key = actionItem.value
> +        // todo countGraphemes?
> +        // what about key codes like arrow, versus unicode chars?
> +        if (typeof key != 'string' || (typeof key == 'string' && key.length != 1)) {

The `!= 1` check here is obviously wrong because JS characters are counted as code units and not code points, and the specification also mandates we use grapheme clusters (for some added benefit for dispatching composed characters the way they were inputted).

But I think it’s entirely fine to leave that for a later patch.  This is not just good, it’s good enough!

::: testing/marionette/action.js:164
(Diff revision 3)
> -        delete this.touchIds[touchId];
> +          throw new InvalidArgumentError("Expected 'key' to be a single-character String, " +
> +                                         "got: " + key);

Rename `String` to `string`.  It is common to write primitive types with lower-casing in JS.  This applies only to `undefined`, `boolean` , `string`, and `number`.

::: testing/marionette/action.js:172
(Diff revision 3)
> -      this.lastCoordinates = null;
> +        item.value = key;
> -      break;
> +        break;
>  
> -    case "move":
> -      this.isTap = false;
> -      if (this.mouseEventsOnly) {
> +      case action.PointerDown:
> +      case action.PointerUp:
> +        assertPositiveInteger(actionItem.button, 'button');

Double quotes.

::: testing/marionette/action.js:177
(Diff revision 3)
> -      if (this.mouseEventsOnly) {
> -        this.emitMouseEvent(doc, "mousemove", x, y, null, null, keyModifiers);
> -      } else {
> -        let touch = this.touchProvider.createATouch(
> -            this.touchIds[touchId].target, x, y, touchId);
> -        this.touchIds[touchId] = touch;
> +        assertPositiveInteger(actionItem.button, 'button');
> +        item.button = actionItem.button;
> +        break;
> +
> +      case action.PointerMove:
> +        assertPositiveInteger(actionItem.duration);

For consistency with the other assertions, we should add a second argument `"duration"` so that the name of the checked key is included in the error message.

::: testing/marionette/action.js:177
(Diff revision 3)
> -        this.touchIds[touchId] = touch;
> -        this.touchProvider.emitTouchEvent("touchmove", touch);
> +        assertPositiveInteger(actionItem.duration);
> +        item.duration = actionItem.duration;

I just filed https://github.com/w3c/webdriver/issues/422 about this.

::: testing/marionette/action.js:179
(Diff revision 3)
> -      } else {
> -        let touch = this.touchProvider.createATouch(
> -            this.touchIds[touchId].target, x, y, touchId);
> -        this.touchIds[touchId] = touch;
> -        this.touchProvider.emitTouchEvent("touchmove", touch);
> +        break;
> +
> +      case action.PointerMove:
> +        assertPositiveInteger(actionItem.duration);
> +        item.duration = actionItem.duration;
> +        let webElement = actionItem.element;

Assignment to `webElement` is both unnecessary and misleading as it’s just a reference; not an actual web element.

::: testing/marionette/action.js:180
(Diff revision 3)
> -        let touch = this.touchProvider.createATouch(
> -            this.touchIds[touchId].target, x, y, touchId);
> -        this.touchIds[touchId] = touch;
> -        this.touchProvider.emitTouchEvent("touchmove", touch);
> +
> +      case action.PointerMove:
> +        assertPositiveInteger(actionItem.duration);
> +        item.duration = actionItem.duration;
> +        let webElement = actionItem.element;
> +        if (typeof webElement != "undefined" && !element.isWebElementReference(webElement)) {

This is a bit tricky.  At some point we need to turn the web element reference into an element by looking it up in the element store.  I’m not sure where to do that because it requires passing a reference to the element store from listener.js…

::: testing/marionette/action.js:182
(Diff revision 3)
> +              "Expected 'actionItem.element' to be an Object that " +
> +              `represents a web element, got: ${webElement}`);

I think just saying “… to be a web element reference, got: …” is fine.

::: testing/marionette/action.js:189
(Diff revision 3)
> +        }
> +        item.element = actionItem.element;
> +
> +        item.x = actionItem.x;
> +        if (typeof item.x != "undefined") {
> +          assertPositiveInteger(item.x, 'x');

Double quote.

::: testing/marionette/action.js:193
(Diff revision 3)
> +        if (typeof item.x != "undefined") {
> +          assertPositiveInteger(item.x, 'x');
> -      }
> +        }
> +        item.y = actionItem.y;
> +        if (typeof item.y != "undefined") {
> +          assertPositiveInteger(item.y, 'y');

Double quote.

::: testing/marionette/action.js:198
(Diff revision 3)
> +          assertPositiveInteger(item.y, 'y');
> +        }
> +        break;
> +
> +      case action.PointerCancel:
> +        // TODO

Throw `UnsupportedOperationError`.

::: testing/marionette/action.js:212
(Diff revision 3)
> +    return item;
> +  }
> +};
> +
> +// actions: list of action sequences
> +action.extractActionChain = function extractActionChain(actions) {

I think we should generalise the types here too, like we did with `Action.fromJson`.

A somewhat sane consumer API could look like:

```js
GeckoDriver.prototype.actions = function(cmd, resp) {
  …
  let chain = action.Chain.fromJson(cmd.parameters);
  action.dispatch(chain);
};
```

::: testing/marionette/action.js:213
(Diff revision 3)
> +  }
> +};
> +
> +// actions: list of action sequences
> +action.extractActionChain = function extractActionChain(actions) {
> +  // TODO check current browsing context?

We will check the current browsing context in `GeckoDriver#action`.

::: testing/marionette/action.js:219
(Diff revision 3)
> +  if (!Array.isArray(actions)) {
> +    throw new InvalidArgumentError(`Expected 'actions' to be an Array, got: ${actions}`);
> +  }
> +  let actionsByTick = [];
> +  for (let actionSequence of actions) {
> +    let inputSourceActions = action.processInputSourceActionSequence(actionSequence);

Similarily:

```js
…
let seq = Sequence.fromJson(sequenceData);
for (let i = 0; i < seq.length; ++i) {
  // new tick
  if (byTick.length < (i + 1)) {
    byTick.push([]);
  }
  byTick[i].push(seq[i]);
}
…
```

::: testing/marionette/action.js:222
(Diff revision 3)
> +  let actionsByTick = [];
> +  for (let actionSequence of actions) {
> +    let inputSourceActions = action.processInputSourceActionSequence(actionSequence);
> +    for (let i = 0; i < inputSourceActions.length; i++) {
> +      // new tick
> +      if (actionsByTick.length < i + 1) {

This isn’t necessary because of the operator ordering in JS, but please put a `(i + 1)` grouping around so it is crystal clear.

::: testing/marionette/action.js:231
(Diff revision 3)
> +    }
> +  }
> +  return actionsByTick;
> +};
>  
> -    default:
> +// action_sequence has a list of actionItems for one input source

Please turn this an the other relevant comments into API documentation with type annotations.  You will find examples of these in some of the other modules under testing/marionette.

This is extremely helpful when reading the code and you don’t have the specification text at hand, so it is possible to reason about the input and the output.

::: testing/marionette/action.js:232
(Diff revision 3)
> +  }
> +  return actionsByTick;
> +};
>  
> -    default:
> -      throw new WebDriverError("Unknown event type: " + type);
> +// action_sequence has a list of actionItems for one input source
> +action.processInputSourceActionSequence = function processInputSourceActionSequence(

Turn this into `action.Sequence`.

::: testing/marionette/action.js:237
(Diff revision 3)
> +  if (typeof id == 'undefined') {
> +    id = element.generateUUID();
> +  } else if (typeof id != 'string') {
> +    throw new InvalidArgumentError(`Expected 'id' to be a string, got: ${id}`);
> +  }

Double quotes.

::: testing/marionette/action.js:243
(Diff revision 3)
> +  if (!Array.isArray(actionItems)) {
> +    throw new InvalidArgumentError(
> +        `Expected 'actionSequence.actions' to be an Array, got: ${actionSequence.actions}`);
> +  }

We seem to check arrays in a few different places.  I’ll let you be the judge of whether we should write a helper.

::: testing/marionette/action.js:248
(Diff revision 3)
> +  if (action.inputStateMap.has(id) && !action.inputStateMap.get(id).is(inputSourceState)) {
> +    throw new InvalidArgumentError(
> +        `Expected ${id} to be mapped to ${inputSourceState}, ` +
> +        `got: ${action.inputStateMap.get(id)}`);
> +  }

I suggest overriding the `get` method on `action.inputStateMap` so that it acts like a `collections.defaultdict` in Python:

```js
action.inputStateMap = class extends Map {
  get(key) {
    let found = super.get(key);
    if (typeof found != "undefined") {
      return found;
    }
    return {subtype: null};
  }
};
```

This would make the test more succinct:

```js
if (!inputStateMap.get(id).is(inputSourceState)) {
  throw new InvalidArgumentError();
}
```

::: testing/marionette/action.js:253
(Diff revision 3)
> +  let actions = [];
> +  for (let actionItem of actionItems) {
> +    actions.push(action.Action.fromJson(actionSequence, actionItem, id));
>    }

Many JS programmers would argue you could be more adventerous here and use mapping:

```js
let actions = actionItems.map(item => Action.fromJson(actionSequence, actionItem, id));
```

But I’m pefectly fine with the code as it is.

::: testing/marionette/action.js:261
(Diff revision 3)
>    }
> -  this.checkForInterrupted();
> +  return actions;
>  };
>  
> -action.Chain.prototype.mouseTap = function(doc, x, y, button, count, mod) {
> -  this.emitMouseEvent(doc, "mousemove", x, y, button, count, mod);
> +action.processPointerParameters = function processPointerParameters(parametersData) {
> +  let pointerParams = {

Shorten to just `params` or something.

::: testing/marionette/action.js:265
(Diff revision 3)
> +  if (typeof parametersData == 'undefined') {
> +    return pointerParams;
> +  }

Delete.

::: testing/marionette/action.js:268
(Diff revision 3)
> -  this.emitMouseEvent(doc, "mouseup", x, y, button, count, mod);
> +    primary: true,
> -};
> +  };
> +  if (typeof parametersData == 'undefined') {
> +    return pointerParams;
> +  }
> +  if (typeof parametersData.pointerType != 'undefined') {

Double quotes.

::: testing/marionette/action.js:269
(Diff revision 3)
> -};
> +  };
> +  if (typeof parametersData == 'undefined') {
> +    return pointerParams;
> +  }
> +  if (typeof parametersData.pointerType != 'undefined') {
> +    pointerParams.pointerType = action.PointerType.get(parametersData.pointerType);

We could expand `PointerType.get` to take a second optional argument to be the default if the first isn’t found:

```js
let params = {
  pointerType: PointerType.get(data.pointerType, PointerType.Mouse),
  primary: true,
};
```

This will probably make the code slightly more succinct.

::: testing/marionette/action.js:272
(Diff revision 3)
> +  }
> +  if (typeof parametersData.pointerType != 'undefined') {
> +    pointerParams.pointerType = action.PointerType.get(parametersData.pointerType);
> +  }
> +  let primary = parametersData.primary;
> +  if (typeof primary != 'undefined') {

Double quotes.

::: testing/marionette/action.js:273
(Diff revision 3)
> +  if (typeof parametersData.pointerType != 'undefined') {
> +    pointerParams.pointerType = action.PointerType.get(parametersData.pointerType);
> +  }
> +  let primary = parametersData.primary;
> +  if (typeof primary != 'undefined') {
> +    assertBoolean(primary, 'primary');

Double quotes.

::: testing/marionette/action.js:281
(Diff revision 3)
> +  return pointerParams;
> +};
> +
> +action.processPointerAction = function processPointerAction(id, pointerParams, act) {
> +  let subtype = act.subtype;
> +  if (action.inputStateMap.has(id) && action.inputStateMap.get(id).subtype !== subtype) {

Should be `subtype.is(subtype)`.

::: testing/marionette/action.js:293
(Diff revision 3)
> +};
> +
> +// helpers
> +function assertPositiveInteger(value, name = undefined) {
> +  if (!Number.isInteger(value) || value < 0) {
> +    throw new InvalidArgumentError(`Expected '${name}' to be an integer >= 0, got: ${value}`);

`name` can be undefined so you should make sure the string makes sense if it is.

“Expected integer >= 0, got: …" sounds good without.

::: testing/marionette/action.js:299
(Diff revision 3)
> +  }
> +}
> +
> +function assertBoolean(value, name = undefined) {
> +  if (typeof(value) != "boolean") {
> +    throw new InvalidArgumentError(`Expected '${name}' to be a boolean, got: ${value}`);

`name` can be undefined.  As above, take this into account when constructing the error message.

::: testing/marionette/action.js:304
(Diff revision 3)
> +    throw new InvalidArgumentError(`Expected '${name}' to be a boolean, got: ${value}`);
> +  }
> +}
> +
> +function capitalize(str) {
> +  if (typeof str != 'string'){

Space before `{` and double quotes.

::: testing/marionette/element.js:642
(Diff revision 3)
>      [element.Key]: uuid,
>      [element.LegacyKey]: uuid,
>    };
>  };
>  
> +element.isWebElementReference = function(el){

Space before `{`.

`el` is misleading as what is passed in is an `Object.<string, string>`.  Perhaps rename it to `ref` or `obj`?

::: testing/marionette/test_action.js:61
(Diff revision 3)
> +  };
> +  for (let d of [-1, 'a']) {
> +    actionItem.button = d;
> +    checkErrors(/to be an integer >= 0/,
> +                action.Action.fromJson,
> +                [actionSequence, actionItem, actionSequence.id],

As I pointed out in `Action.fromJson` we don’t have to pass `actionSequence` twice here.

::: testing/marionette/test_action.js:527
(Diff revision 3)
> +function getTypeString(obj) {
> +  return Object.prototype.toString.call(obj);
> +};
> +
> +function checkErrors(regex, func, args, message) {
> +  if (typeof message === 'undefined') {

Use `==` and double quotes.

Comment 22

3 years ago
mozreview-review
Comment on attachment 8798703 [details]
Rename to action and legacyaction

https://reviewboard.mozilla.org/r/84120/#review84890
Attachment #8798703 - Flags: review?(ato) → review-

Comment 23

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review84888
Attachment #8791840 - Flags: review?(ato) → review-

Comment 24

3 years ago
mozreview-review
Comment on attachment 8798704 [details]
Address review comments (nits, style)

https://reviewboard.mozilla.org/r/84122/#review84892
Attachment #8798704 - Flags: review?(ato) → review-

Comment 25

3 years ago
mozreview-review
Comment on attachment 8798705 [details]
Formalize PointerType and ACTIONS; rework InputState

https://reviewboard.mozilla.org/r/84124/#review84894
Attachment #8798705 - Flags: review?(ato) → review-

Comment 26

3 years ago
mozreview-review
Comment on attachment 8798706 [details]
Move most action subtype processing into one function

https://reviewboard.mozilla.org/r/84126/#review84896
Attachment #8798706 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)
Attachment #8798703 - Attachment is obsolete: true
Attachment #8798703 - Flags: review?(james)
Attachment #8798704 - Attachment is obsolete: true
Attachment #8798704 - Flags: review?(james)
Attachment #8798705 - Attachment is obsolete: true
Attachment #8798705 - Flags: review?(james)
Attachment #8798706 - Attachment is obsolete: true
Attachment #8798706 - Flags: review?(james)
Note that a bunch of comments in MozReview did not get published on this bug -- you'll need look at the list of issues to see full discussion.

Comment 29

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

::: testing/marionette/action.js:19
(Diff revision 4)
>  
>  this.EXPORTED_SYMBOLS = ["action"];
>  
>  const logger = Log.repository.getLogger("Marionette");
>  
> -this.action = {};
> +this.action = {

Provide a brief description of the action module as API documentation.

::: testing/marionette/action.js:29
(Diff revision 4)
> +const ACTIONS = {
> +  none: new Set([action.Pause]),
> +  key: new Set([action.Pause, action.KeyDown, action.KeyUp]),
> +  pointer: new Set([
> +    action.Pause,
> +    action.PointerDown,
> +    action.PointerUp,
> +    action.PointerMove,
> +    action.PointerCancel,
> +  ]),
> +};

Move this above `this.action`.

::: testing/marionette/action.js:41
(Diff revision 4)
> +    action.PointerMove,
> +    action.PointerCancel,
> +  ]),
> +};
> +
> +action.PointerType = {

API documentation.  Fine to reuse what is described in the spec.

::: testing/marionette/action.js:47
(Diff revision 4)
>  /**
> - * Functionality for (single finger) action chains.
> + * @param {string} str
> + *     Name of pointer type.
> + *
> + * @return {string}
> + *     A pointer type for processing pointer parameters.
> + *
> + * @throws InvalidArgumentError
> + *     If @code{str} is not a valid pointer type.
>   */

Provide a oneliner describing that this is a lookup function for `PointerType`.

::: testing/marionette/action.js:51
(Diff revision 4)
>  
>  /**
> - * Functionality for (single finger) action chains.
> + * @param {string} str
> + *     Name of pointer type.
> + *
> + * @return {string}

`PointerType`

::: testing/marionette/action.js:55
(Diff revision 4)
> + *
> + * @return {string}
> + *     A pointer type for processing pointer parameters.
> + *
> + * @throws InvalidArgumentError
> + *     If @code{str} is not a valid pointer type.

`@code{str}` → `|str|`

::: testing/marionette/action.js:61
(Diff revision 4)
>    } else {
> -    this.checkForInterrupted = () => {};
> +    return this[name];
>    }

Matter of personal preference, but drop the `else` clause here.  I tend to expect the first if-condition to succeed when I see `else`.  That is not the case here.

::: testing/marionette/action.js:68
(Diff revision 4)
> -  this.inputSource = null;
>  };
>  
> -action.Chain.prototype.dispatchActions = function(
> -    args,
> -    touchId,
> +
> +/**
> + * Input state associated with current session. This is a map between input id and

s/id/ID/

::: testing/marionette/action.js:83
(Diff revision 4)
> -  if (touchId == null) {
> -    touchId = this.nextTouchId++;
> -  }
> -
> -  if (!container.frame.document.createTouch) {
> -    this.mouseEventsOnly = true;
> +  is(other) {
> +    if (typeof other == "undefined"){
> +      return false;
> +    }
> +    return this.type === other.type;
> +  }

API documentation.

::: testing/marionette/action.js:84
(Diff revision 4)
> -  this.container = container;
> -  let commandArray = element.fromJson(
> -      args, seenEls, container.frame, container.shadowRoot);
> -
> -  if (touchId == null) {
> -    touchId = this.nextTouchId++;
> +  constructor() {
> +    this.type = this.constructor.name.toLowerCase();
> +  }
> +
> +  is(other) {
> +    if (typeof other == "undefined"){

Space before `{`.

::: testing/marionette/action.js:94
(Diff revision 4)
> -    altKey: false,
> -    metaKey: false,
> -  };
> -
> -  return new Promise(resolve => {
> -    this.actions(commandArray, touchId, 0, keyModifiers, resolve);
> -  }).catch(this.resetValues);
> -};
> -
> -/**
> - * This function emit mouse event.
> +  /**
> +   * @param {?} actionSequence
> +   *     object representing an action sequence.
> +   *
> +   * @return {action.InputState}
> +   *     An @code{action.InputState} object for the type of the @code{actionSequence}.
> +   *
> +   * @throws InvalidArgumentError
> +   *     If @code{actionSequence.type} is not valid.
> +   */
> +  static fromJson(actionSequence) {

Provide a one-liner description.

::: testing/marionette/action.js:96
(Diff revision 4)
> -  let keyModifiers = {
> -    shiftKey: false,
> -    ctrlKey: false,
> -    altKey: false,
> -    metaKey: false,
> -  };
> +    return `[object ${this.constructor.name}InputState]`;
> +  }
> +
> +  /**
> +   * @param {?} actionSequence
> +   *     object representing an action sequence.

s/object/Object/

::: testing/marionette/action.js:99
(Diff revision 4)
> -    altKey: false,
> -    metaKey: false,
> -  };
> -
> -  return new Promise(resolve => {
> -    this.actions(commandArray, touchId, 0, keyModifiers, resolve);
> +  /**
> +   * @param {?} actionSequence
> +   *     object representing an action sequence.
> +   *
> +   * @return {action.InputState}
> +   *     An @code{action.InputState} object for the type of the @code{actionSequence}.

`@code{actionSequence}` → `|actionSequence|`

::: testing/marionette/action.js:114
(Diff revision 4)
> -    } else {
> -      mods = 0;
> -    }
> +  }
> +}
> +
> +action.InputState = {};

Documentation, please.

::: testing/marionette/action.js:118
(Diff revision 4)
> +
> +action.InputState = {};
>  
> -    domUtils.sendMouseEvent(
> -        type,
> -        elClientX,
> +/**
> + * Input state associated with a keyboard-type device.
> + * @type {action.InputState}

Drop.

::: testing/marionette/action.js:133
(Diff revision 4)
>    }
>  };
>  
>  /**
> - * Reset any persisted values after a command completes.
> + * Input state not associated with a specific physical device.
> + * @type {action.InputState}

Drop.

::: testing/marionette/action.js:144
(Diff revision 4)
> -  this.mouseEventsOnly = false;
> +  }
>  };
>  
>  /**
> - * Emit events for each action in the provided chain.
> + * Input state associated with a pointer-type input device.
> + * @type {action.InputState}

Drop.

::: testing/marionette/action.js:146
(Diff revision 4)
> - * To emit touch events for each finger, one might send a [["press", id],
> - * ["wait", 5], ["release"]] chain.
> + * @param  {string} subtype
> + *     Kind of pointing device.

Document kinds.

::: testing/marionette/action.js:170
(Diff revision 4)
> - * @param {number} i
> - *     Keeps track of the current action of the chain.
> + * @param  {string} subtype
> + *     Action subtype.

Document subtypes.

::: testing/marionette/action.js:245
(Diff revision 4)
> -    case "moveByOffset":
> -      this.generateEvents(
> -          "move",
> -          this.lastCoordinates[0] + pack[1],
> -          this.lastCoordinates[1] + pack[2],
> -          touchId,
> +      case action.PointerMove:
> +        assertPositiveInteger(actionItem.duration, "duration");
> +        // TODO? https://github.com/w3c/webdriver/issues/422
> +        item.duration = actionItem.duration;
> +        if (typeof actionItem.element != "undefined" &&
> +                !element.isWebElementReference(actionItem.element)) {

Too much indentation.  Should be four spaces from `if` on line above.

::: testing/marionette/action.js:280
(Diff revision 4)
> +  }
>  };
>  
>  /**
> - * Given an element and a pair of coordinates, returns an array of the
> - * form [clientX, clientY, pageX, pageY, screenX, screenY].
> + * Represents a series of ticks, specifying which actions to perform at each tick.
> + * @type {action.Chain}

Drop

::: testing/marionette/action.js:292
(Diff revision 4)
> -/**
> +  /**
> - * @param {number} x
> - *     X coordinate of the location to generate the event that is relative
> - *     to the viewport.
> - * @param {number} y
> - *     Y coordinate of the location to generate the event that is relative
> +   * @param  {Array<?>} actions
> +   *     Array of objects that each represent an action sequence.
> +   *
> +   * @return {action.Chain}
> +   *     Transpose of @code{actions} such that actions to be performed in a single tick

`@code{actions}` → `|actions|`

::: testing/marionette/action.js:296
(Diff revision 4)
> - * @param {number} y
> - *     Y coordinate of the location to generate the event that is relative
> - *     to the viewport.
> - */
> -action.Chain.prototype.generateEvents = function(
> -    type, x, y, touchId, target, keyModifiers) {
> +   * @return {action.Chain}
> +   *     Transpose of @code{actions} such that actions to be performed in a single tick
> +   *     are grouped together.
> +   *
> +   * @throws InvalidArgumentError
> +   *     If @code{actions} is not an Array.

`@code{actions}` → `|actions|`

::: testing/marionette/action.js:320
(Diff revision 4)
> -        let touch = this.touchIds[touchId];
> -        let [x, y] = this.lastCoordinates;
>  
> -        touch = this.touchProvider.createATouch(touch.target, x, y, touchId);
> -        this.touchProvider.emitTouchEvent("touchend", touch);
> +/**
> + * Represents one input source action sequence; this is essentially an @code{Array<action.Action>}.
> + * @type {action.Sequence}

Drop.

::: testing/marionette/action.js:322
(Diff revision 4)
>  
> -        touch = this.touchProvider.createATouch(touch.target, x, y, touchId);
> -        this.touchProvider.emitTouchEvent("touchend", touch);
> +/**
> + * Represents one input source action sequence; this is essentially an @code{Array<action.Action>}.
> + * @type {action.Sequence}
> + */
> +action.Sequence = class extends Array{

Space before `{`.

::: testing/marionette/action.js:328
(Diff revision 4)
> +  toString() {
> +    return `[sequence ${super.toString()}]`;
> +  }
>  
> -        if (this.isTap) {
> -          this.mouseTap(
> +  /**
> +   * @param  {?} actionSequence

One space after `@param`.

::: testing/marionette/action.js:354
(Diff revision 4)
> +    if (!Array.isArray(actionItems)) {
> +      throw new InvalidArgumentError(
> +          `Expected 'actionSequence.actions' to be an Array, got: ${actionSequence.actions}`);
> +    }
> +
> +    if (action.inputStateMap.has(id) && !action.inputStateMap.get(id).is(inputSourceState)) {

I believe I mentioned in an earlier comment that I think we should extend the `Map` type of `inputStateMap` so that `.get(id)` always returns something, like a `collections.defaultdict` in Python does.

This would reduce this line:

```js
if (action.inputStateMap.get(id).is(inputSourceState)) {
```

::: testing/marionette/action.js:369
(Diff revision 4)
> -      }
> +  }
> +};
>  
> -      this.isTap = false;
> -      this.lastCoordinates = null;
> -      break;
> +/**
> + * Represents parameters in an action for a pointer input source.
> + * @type {action.PointerParameters}

Drop.

::: testing/marionette/action.js:371
(Diff revision 4)
>  
> -      this.isTap = false;
> -      this.lastCoordinates = null;
> -      break;
> +/**
> + * Represents parameters in an action for a pointer input source.
> + * @type {action.PointerParameters}
> + *
> + * @param  {string=} pointerType

One space after `@param`.

::: testing/marionette/action.js:372
(Diff revision 4)
> + *     Type of pointing device.
> + *     If the parameter is undefined, "mouse" is used.

Put on same line.

::: testing/marionette/action.js:374
(Diff revision 4)
> -      break;
> + * @type {action.PointerParameters}
> + *
> + * @param  {string=} pointerType
> + *     Type of pointing device.
> + *     If the parameter is undefined, "mouse" is used.
> + * @param  {boolean=} primary

One space after `@param`.

::: testing/marionette/action.js:376
(Diff revision 4)
> + * @param  {string=} pointerType
> + *     Type of pointing device.
> + *     If the parameter is undefined, "mouse" is used.
> + * @param  {boolean=} primary
> + *     Whether the input source is the primary pointing device.
> + *     If the parameter is underfined, @code{true} is used.

`@code{true}` → true

::: testing/marionette/action.js:386
(Diff revision 4)
> +    assertBoolean(primary, "primary");
> +    this.primary = primary;
> +  };
>  
> -    case "cancel":
> -      this.isTap = false;
> +  toString() {
> +    return `[pointerParameters ${this.pointerType}, primary:${this.primary}]`;

Probably replace `:` with `=`.

::: testing/marionette/action.js:390
(Diff revision 4)
> -      }
> +  }
> -      this.lastCoordinates = null;
> -      break;
>  
> -    case "move":
> -      this.isTap = false;
> +  /**
> +   * @param  {?} parametersData

One space after `@param`.

::: testing/marionette/action.js:405
(Diff revision 4)
> -      let [clientX, clientY, pageX, pageY, screenX, screenY] =
> -          this.getCoordinateInfo(target, x, y);
> +/**
> + * Adds @code{pointerType} and @code{primary} attributes to Action @{act}.
> + *
> + * @param  {string} id
> + *     Input source id.
> + * @param  {action.PointerParams} pointerParams
> + *     Input source pointer parameters.
> + * @param  {action.Action} act
> + *     Action to be updated.
> + *
> + * @throws InvalidArgumentError
> + *     If @code{id} is already mapped to an @code{action.InputState} that is
> + *     not compatible with @code{act.subtype}.
> + */
> +action.processPointerAction = function processPointerAction(id, pointerParams, act) {

I would unpublish this.

::: testing/marionette/action.js:405
(Diff revision 4)
> -      let [clientX, clientY, pageX, pageY, screenX, screenY] =
> -          this.getCoordinateInfo(target, x, y);
> +/**
> + * Adds @code{pointerType} and @code{primary} attributes to Action @{act}.
> + *
> + * @param  {string} id
> + *     Input source id.
> + * @param  {action.PointerParams} pointerParams
> + *     Input source pointer parameters.
> + * @param  {action.Action} act
> + *     Action to be updated.
> + *
> + * @throws InvalidArgumentError
> + *     If @code{id} is already mapped to an @code{action.InputState} that is
> + *     not compatible with @code{act.subtype}.
> + */
> +action.processPointerAction = function processPointerAction(id, pointerParams, act) {

I would unpublish this from the module, since we’re not intending consumers to call it directly.

::: testing/marionette/action.js:433
(Diff revision 4)
> +};
>  
> -      event.initMouseEvent(
> -          "contextmenu",
> -          true,
> -          true,
> +// helpers
> +function assertPositiveInteger(value, name = undefined) {
> +  if (!Number.isInteger(value) || value < 0) {
> +    throw new InvalidArgumentError(`Expected ${name} integer >= 0, got: ${value}`);

Correct so the message works when `name` is undefined.

::: testing/marionette/action.js:439
(Diff revision 4)
> -      target.dispatchEvent(event);
> -      break;
>  
> -    default:
> -      throw new WebDriverError("Unknown event type: " + type);
> +function assertBoolean(value, name = undefined) {
> +  if (typeof(value) != "boolean") {
> +    throw new InvalidArgumentError(`Expected ${name} boolean, got: ${value}`);

Correct the error message so it works when `name` is undefined.

Saying that "Expected undefined boolean, got: whatevz" sounds very wrong.

::: testing/marionette/driver.js:20
(Diff revision 4)
> -Cu.import("chrome://marionette/content/action.js");
> +Cu.import("chrome://marionette/content/legacyaction.js");
>  Cu.import("chrome://marionette/content/atom.js");
>  Cu.import("chrome://marionette/content/browser.js");
>  Cu.import("chrome://marionette/content/element.js");
>  Cu.import("chrome://marionette/content/error.js");
>  Cu.import("chrome://marionette/content/evaluate.js");
>  Cu.import("chrome://marionette/content/event.js");
>  Cu.import("chrome://marionette/content/interaction.js");
>  Cu.import("chrome://marionette/content/logging.js");
>  Cu.import("chrome://marionette/content/modal.js");
>  Cu.import("chrome://marionette/content/proxy.js");
>  Cu.import("chrome://marionette/content/simpletest.js");

Order lexicographically.

::: testing/marionette/driver.js:20
(Diff revision 4)
> -Cu.import("chrome://marionette/content/action.js");
> +Cu.import("chrome://marionette/content/legacyaction.js");
>  Cu.import("chrome://marionette/content/atom.js");
>  Cu.import("chrome://marionette/content/browser.js");
>  Cu.import("chrome://marionette/content/element.js");
>  Cu.import("chrome://marionette/content/error.js");
>  Cu.import("chrome://marionette/content/evaluate.js");
>  Cu.import("chrome://marionette/content/event.js");
>  Cu.import("chrome://marionette/content/interaction.js");
>  Cu.import("chrome://marionette/content/logging.js");
>  Cu.import("chrome://marionette/content/modal.js");
>  Cu.import("chrome://marionette/content/proxy.js");
>  Cu.import("chrome://marionette/content/simpletest.js");

Sort lexicographically.

::: testing/marionette/element.js:642
(Diff revision 4)
>      [element.Key]: uuid,
>      [element.LegacyKey]: uuid,
>    };
>  };
>  
> +element.isWebElementReference = function(ref) {

API documentation + test

::: testing/marionette/test_action.js:9
(Diff revision 4)
> +Cu.import("chrome://marionette/content/error.js");
> +Cu.import("chrome://marionette/content/element.js");
> +Cu.import("chrome://marionette/content/action.js");

Order lexicographically.

::: testing/marionette/test_action.js:15
(Diff revision 4)
> +Cu.import("chrome://marionette/content/element.js");
> +Cu.import("chrome://marionette/content/action.js");
> +
> +add_test(function test_createAction() {
> +  Assert.throws(() => new action.Action(), InvalidArgumentError,
> +                "Missing Action constructor args");

Too much indentation.

::: testing/marionette/test_action.js:16
(Diff revision 4)
> +Cu.import("chrome://marionette/content/action.js");
> +
> +add_test(function test_createAction() {
> +  Assert.throws(() => new action.Action(), InvalidArgumentError,
> +                "Missing Action constructor args");
> +  Assert.throws(() => new action.Action(1,2), InvalidArgumentError,

Requires a test that passing three arguments does not throw, but assigns the `id`, `type`, and `subtype` properties accordingly.

::: testing/marionette/test_action.js:17
(Diff revision 4)
> +
> +add_test(function test_createAction() {
> +  Assert.throws(() => new action.Action(), InvalidArgumentError,
> +                "Missing Action constructor args");
> +  Assert.throws(() => new action.Action(1,2), InvalidArgumentError,
> +                "Missing Action constructor args");

Indent

::: testing/marionette/test_action.js:24
(Diff revision 4)
> +});
> +
> +add_test(function test_defaultPointerParameters() {
> +  let defaultParameters = {pointerType: action.PointerType.Mouse, primary: true};
> +  deepEqual(action.PointerParameters.fromJson(), defaultParameters);
> +  deepEqual(action.PointerParameters.fromJson({blah: "nonsense"}), defaultParameters);

I would change this to test that `primary` is not a boolean.  Additional JSON keys is generally not worth testing.

::: testing/marionette/test_action.js:32
(Diff revision 4)
> +});
> +
> +add_test(function test_processPointerParameters() {
> +  let check = function(regex, message, args) {
> +    checkErrors(regex, action.PointerParameters.fromJson, args, message);
> +  }

Use ES6 arrow function, or add missing `;`.

::: testing/marionette/test_action.js:36
(Diff revision 4)
> +    checkErrors(regex, action.PointerParameters.fromJson, args, message);
> +  }
> +  let parametersData = {pointerType: "foo"};
> +  let message = `parametersData: [pointerType: ${parametersData.pointerType}, ` +
> +      `primary: ${parametersData.primary}]`;
> +  check(/Unknown pointerType/, message, [parametersData]);

It looks odd to pass in a single array here when `check` wraps `checkErrors`.  Why not convert it to an array inside the wrapper?

::: testing/marionette/test_action.js:45
(Diff revision 4)
> +
> +  parametersData.primary = false;
> +  deepEqual(action.PointerParameters.fromJson(parametersData),
> +            {pointerType: action.PointerType.Pen, primary: false});
> +
> +

Drop one of the empty lines.

::: testing/marionette/test_action.js:50
(Diff revision 4)
> +  let actionItem = {
> +    type: "pointerDown",
> +  };
> +  let actionSequence = {
> +    type: "pointer",
> +    id: "some_id",
> +  };

For brevity you could pull these into just two lines.

::: testing/marionette/test_action.js:73
(Diff revision 4)
> +  let actionSequence = {
> +    id: "some_id",
> +  }

One line?

::: testing/marionette/test_action.js:77
(Diff revision 4)
> +  let actionItem = {};
> +  let actionSequence = {
> +    id: "some_id",
> +  }
> +  let check = function(type, subtype, message = undefined) {
> +    let m = message || `duration: ${actionItem.duration}, subtype: ${subtype}`;

You can reuse the `message` variable.

::: testing/marionette/test_action.js:81
(Diff revision 4)
> +  let check = function(type, subtype, message = undefined) {
> +    let m = message || `duration: ${actionItem.duration}, subtype: ${subtype}`;
> +    actionItem.type = subtype;
> +    actionSequence.type = type;
> +    checkErrors(/integer >= 0/,
> +                action.Action.fromJson, [actionSequence, actionItem], m);

Indent

::: testing/marionette/test_action.js:92
(Diff revision 4)
> +      let message = `${name}: ${actionItem[name]}`;
> +      check("pointer", "pointerMove", message);

Pass `message` in as a literate, no need to assign it.

::: testing/marionette/test_action.js:100
(Diff revision 4)
> +  let actionSequence = {
> +    type: "pointer",
> +    id: "some_id",
> +  }
> +  let actionItem = {
> +    duration: 5,
> +    type: "pointerMove",
> +  };

Two lines?

::: testing/marionette/test_action.js:108
(Diff revision 4)
> +  }
> +  let actionItem = {
> +    duration: 5,
> +    type: "pointerMove",
> +  };
> +  for (let d of [-1, "a", {a:"blah"}]) {

Space after `a`.

::: testing/marionette/test_action.js:109
(Diff revision 4)
> +  let actionItem = {
> +    duration: 5,
> +    type: "pointerMove",
> +  };
> +  for (let d of [-1, "a", {a:"blah"}]) {
> +    actionItem.element = d;

Missing a test for `element.isWebElementReference`.

::: testing/marionette/test_action.js:111
(Diff revision 4)
> +                action.Action.fromJson,
> +                [actionSequence, actionItem],
> +                `actionItem.element: (${getTypeString(d)})`);

Indent

::: testing/marionette/test_action.js:115
(Diff revision 4)
> +    checkErrors(/Expected 'actionItem.element' to be a web element reference/,
> +                action.Action.fromJson,
> +                [actionSequence, actionItem],
> +                `actionItem.element: (${getTypeString(d)})`);
> +  }
> +

Missing a test for whether `item.element` is set successfully.

::: testing/marionette/test_action.js:120
(Diff revision 4)
> +  let actionSequence = {
> +    id: "some_id",
> +    type: "pointer",
> +  };

One line?

::: testing/marionette/test_action.js:145
(Diff revision 4)
> +      duration: 5,
> +      type: "pointerMove",
> +      x: 0,
> +      y: undefined,
> +      element: undefined,
> +

Empty line

::: testing/marionette/test_action.js:155
(Diff revision 4)
> +  for (let a of actionItems) {
> +    let act = action.Action.fromJson(actionSequence, a);

Maybe `expected` and `actual` would be good variable names?

::: testing/marionette/test_action.js:197
(Diff revision 4)
> +    }
> +    else {

Pull `else` up to the line with `}`.

::: testing/marionette/test_action.js:211
(Diff revision 4)
> +  let actionItem = {
> +    type: "pause",
> +    duration: 5,
> +  };
> +  let actionSequence = {
> +    id: "some_id",
> +  };

Two lines?

::: testing/marionette/test_action.js:232
(Diff revision 4)
> +  let actionItem = {
> +    type: "dancing",
> +  };
> +  let actionSequence = {
> +    id: "some_id",
> +  };

Two lines?

::: testing/marionette/test_action.js:250
(Diff revision 4)
> +  let actionSequence = {
> +    type: "key",
> +    id: "some_id",
> +  };
> +  let actionItem = {
> +    type: "keyDown",
> +  };

Two lines?

::: testing/marionette/test_action.js:262
(Diff revision 4)
> +
> +  for (let d of [-1, "bad", undefined, [], ["a"], {length: 1}, null]) {
> +    actionItem.value = d;
> +    let message = `actionItem.value: (${getTypeString(d)})`;
> +    Assert.throws(() => action.Action.fromJson(actionSequence, actionItem),
> +                  InvalidArgumentError, message);

Indent

::: testing/marionette/test_action.js:264
(Diff revision 4)
> +                  /Expected 'key' to be a single-character string/,
> +                  message);

Indent

::: testing/marionette/test_action.js:278
(Diff revision 4)
> +  equal(act.id, actionSequence.id);
> +  equal(act.value, actionItem.value);
> +
> +  run_next_test();
> +});
> +

Drop empty line

::: testing/marionette/test_action.js:281
(Diff revision 4)
> +  let actionSequence = {
> +    type: "swim",
> +    id: "some id",
> +  };

One line?

::: testing/marionette/test_action.js:285
(Diff revision 4)
> +  let check = function(message, regex) {
> +    checkErrors(regex, action.Sequence.fromJson,
> +                [actionSequence], message);
> +  };

Arrow function?

::: testing/marionette/test_action.js:289
(Diff revision 4)
> +  check(`actionSequence.type: ${actionSequence.type}`,
> +        /Unknown action type/);

Indent or put on same line?

::: testing/marionette/test_action.js:295
(Diff revision 4)
> +        /Unknown action type/);
> +
> +  actionSequence.type = "none";
> +  actionSequence.id = -1;
> +  check(`actionSequence.id: ${getTypeString(actionSequence.id)}`,
> +        /Expected 'id' to be a string/);

Indent

::: testing/marionette/test_action.js:300
(Diff revision 4)
> +        /Expected 'id' to be a string/);
> +
> +  actionSequence.id = "some_id";
> +  actionSequence.actions = -1;
> +  check(`actionSequence.actions: ${getTypeString(actionSequence.actions)}`,
> +        /Expected 'actionSequence.actions' to be an Array/);

Indent

::: testing/marionette/test_action.js:306
(Diff revision 4)
> +  let actionItem = {
> +    type: "pause",
> +    duration: 5,
> +  };
> +  let actionSequence = {
> +    type: "none",
> +    id: "some id",
> +    actions: [actionItem],
> +  };

Two lines?

::: testing/marionette/test_action.js:337
(Diff revision 4)
> +  let expectedAction = new action.Action(actionSequence.id,
> +                                          actionSequence.type,
> +                                          actionItem.type);

A matter of taste, but I normally break all arguments to the next line in circumstances like these:

```js
let expectedAction = new action.Action(
    actionSequence.id, actionSequence.type, actionSequence.type);
```

Alternatively you could shorten the variable names `actionSequence` and `expectedAction`:

```js
let expected = new action.Action(seq.id, seq.type, seq.type);
```

::: testing/marionette/test_action.js:350
(Diff revision 4)
> +  let actionItem = {
> +      type: "keyUp",
> +      value: "a",
> +  };
> +  let actionSequence = {
> +    type: "key",
> +    id: "9",
> +    actions: [actionItem],
> +  };

Two lines?

::: testing/marionette/test_action.js:371
(Diff revision 4)
> +  let actionItems = [
> +    {
> +      type: "pause",
> +      duration: 5,
> +    },
> +  ];
> +  let actionSequence = {
> +    type: "key",
> +    actions: actionItems,
> +  };

Two lines?

::: testing/marionette/test_action.js:383
(Diff revision 4)
> +  equal(actions[0].id.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i)[0],
> +        actions[0].id);

This test seems wrong to me.  A test that `actions[0].id` matches the regexp seems sufficient.  

As far as I understand, this test first matches, extracts the find and then compares it with the input.

Is there no easier way to do this?

::: testing/marionette/test_action.js:390
(Diff revision 4)
> +  let actionItem = {
> +    type: "pause",
> +    duration: 5,
> +  };
> +  let actionSequence = {
> +    type: "key",
> +    id: id,
> +    actions: [actionItem],
> +  };

Two lines?

::: testing/marionette/test_action.js:402
(Diff revision 4)
> +              action.Sequence.fromJson,
> +              [actionSequence],
> +              `${actionSequence.type} using ${wrongInputState}`);

Indent

::: testing/marionette/test_action.js:427
(Diff revision 4)
> +              action.processPointerAction,
> +              [id, parameters, a],
> +              `$subtype ${actionItem.type} with ${wrongInputState.subtype} in inputState`);

Indent

::: testing/marionette/test_action.js:442
(Diff revision 4)
> +    Assert.throws(() => action.Chain.fromJson(actions),
> +                  InvalidArgumentError, message);
> +    Assert.throws(() => action.Chain.fromJson(actions),
> +                  /Expected 'actions' to be an Array/,
> +                  message);

Indent

::: testing/marionette/test_action.js:451
(Diff revision 4)
> +                  message);
> +  }
> +  run_next_test();
> +});
> +
> +add_test(function test_extractActionChain_empty() {

s/_empty/Empty/

::: testing/marionette/test_action.js:457
(Diff revision 4)
> +  let actionItem = {
> +    type: "pause",
> +    duration: 5,
> +  };
> +  let actionSequence = {
> +    type: "none",
> +    id: "some id",
> +    actions: [actionItem],
> +  };

Two lines?

::: testing/marionette/test_action.js:521
(Diff revision 4)
> +                                          "key",
> +                                          keyActionItems[2].type);

Indent
Attachment #8791840 - Flags: review?(ato) → review-
Assignee

Comment 30

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

> I believe I mentioned in an earlier comment that I think we should extend the `Map` type of `inputStateMap` so that `.get(id)` always returns something, like a `collections.defaultdict` in Python does.
> 
> This would reduce this line:
> 
> ```js
> if (action.inputStateMap.get(id).is(inputSourceState)) {
> ```

Yes, I responded to that comment and others, but MozReview does not make my replies easy to find. You can see them all in the Review Summary view. 

Adding defaultdict-like behaviour won't shorten this part of the code. Original reply: `3 days, 20 hours ago: We still have to check for inputStateMap.has(id): if id isn't there, we don't want to throw an error in the first place. (That is, we only want to throw an error if id is present and mapped to the wrong thing.)`

Comment 31

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

> Yes, I responded to that comment and others, but MozReview does not make my replies easy to find. You can see them all in the Review Summary view. 
> 
> Adding defaultdict-like behaviour won't shorten this part of the code. Original reply: `3 days, 20 hours ago: We still have to check for inputStateMap.has(id): if id isn't there, we don't want to throw an error in the first place. (That is, we only want to throw an error if id is present and mapped to the wrong thing.)`

Sorry, I missed these.  But that makes sense.
Assignee

Comment 32

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

> Move this above `this.action`.

Leaving as is because this refers to `action.Pause`, etc.

> `PointerType`

Isn't string more accurate? e.g. PointerType.Mouse is a string, not a subtype of PointerType.
Assignee

Comment 33

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

> I would unpublish this from the module, since we’re not intending consumers to call it directly.

As I mentioned in the previous round, `processPointerAction` splits out some behaviour in a way that is useful for unit testing, so I'd like to keep it. Is there a preferred way to indicate in the docs that this is not intended for consumers?
Comment hidden (mozreview-request)

Comment 35

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review87764

This looks shippable.  Please address the remaining nitpicks before pushing.

::: testing/marionette/action.js:74
(Diff revision 5)
> -  this.container = container;
> -  let commandArray = element.fromJson(
> -      args, seenEls, container.frame, container.shadowRoot);
>  
> -  if (touchId == null) {
> -    touchId = this.nextTouchId++;
> +/**
> + * Input state associated with current session. This is a map between input id and

s/id/ID/

::: testing/marionette/action.js:76
(Diff revision 5)
> -      args, seenEls, container.frame, container.shadowRoot);
>  
> -  if (touchId == null) {
> -    touchId = this.nextTouchId++;
> -  }
> +/**
> + * Input state associated with current session. This is a map between input id and
> + * the device state for that input source, with one entry for each active input source.
> + *

Drop empty line.

::: testing/marionette/action.js:91
(Diff revision 5)
>    }
>  
> -  let keyModifiers = {
> -    shiftKey: false,
> -    ctrlKey: false,
> -    altKey: false,
> +  /**
> +   * Check equality of this InputState object with another.
> +   *
> +   * @param  {?}  other

One space before `other`.

::: testing/marionette/action.js:191
(Diff revision 5)
> +    for (let attr of [id, type, subtype]){
> +      if (typeof attr != "string"){

Spaces before `{`.

::: testing/marionette/action.js:208
(Diff revision 5)
> -  i++;
>  
> -  if (["press", "wait", "keyDown", "keyUp", "click"].indexOf(command) == -1) {
> -    // if mouseEventsOnly, then touchIds isn't used
> -    if (!(touchId in this.touchIds) && !this.mouseEventsOnly) {
> -      this.resetValues();
> +  /**
> +   * @param {?} actionSequence
> +   *     Object representing sequence of actions from one input source.
> +   * @param {?}  actionItem

One space before `actionItem`.

::: testing/marionette/action.js:335
(Diff revision 5)
> -        let [x, y] = this.lastCoordinates;
>  
> -        touch = this.touchProvider.createATouch(touch.target, x, y, touchId);
> -        this.touchProvider.emitTouchEvent("touchend", touch);
> +/**
> + * Represents one input source action sequence; this is essentially an |Array<action.Action>|.
> + */
> +action.Sequence = class extends Array{

Space before `{`.

::: testing/marionette/element.js:642
(Diff revision 5)
> +/**
> + * Checks if |ref| has either |element.Key| or |element.LegacyKey| as properties.
> + *
> + * @param {?} ref
> + *     Object that represents a web element reference.
> + * @return {boolean}
> + *     True if |ref| has either expected property.
> + */
> +element.isWebElementReference = function(ref) {
> +  let properties = Object.getOwnPropertyNames(ref);
> +  return properties.includes(element.Key) || properties.includes(element.LegacyKey);
> +};

Missing a test for this function.

::: testing/marionette/test_action.js:55
(Diff revision 5)
> +                action.Action.fromJson,
> +                [actionSequence, actionItem],
> +                `button: ${actionItem.button}`);

Four space indent.
Attachment #8791840 - Flags: review?(ato) → review+
Assignee

Comment 36

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review86996

> Isn't string more accurate? e.g. PointerType.Mouse is a string, not a subtype of PointerType.

No reply in last review, so I'm assuming I can drop this issue.

> As I mentioned in the previous round, `processPointerAction` splits out some behaviour in a way that is useful for unit testing, so I'd like to keep it. Is there a preferred way to indicate in the docs that this is not intended for consumers?

No reply in last review, so dropping the issue.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review88430

::: testing/marionette/action.js:23
(Diff revision 8)
> -
>  /**
> - * Functionality for (single finger) action chains.
> + * Implements WebDriver Actions API: a low-level interfac for providing
> + * virtualised device input to the web browser.
>   */
> -action.Chain = function(checkForInterrupted) {
> +this.action = {

Not for this patch, but it seems like with ES 2016 and `Symbol` you can make a safer approximation to an enum e.g. https://gist.github.com/xmlking/e86e4f15ec32b12c4689

::: testing/marionette/action.js:148
(Diff revision 8)
> - * Reset any persisted values after a command completes.
> + * Input state not associated with a specific physical device.
>   */
> -action.Chain.prototype.resetValues = function() {
> -  this.container = null;
> -  this.seenEls = null;
> -  this.touchProvider = null;
> +action.InputState.Null = class extends InputState {
> +  constructor() {
> +    super();
> +    this.type = "none";

Seems slightly odd that this has a type property but not any of the others?

::: testing/marionette/action.js:255
(Diff revision 8)
> -      this.actions(chain, touchId, i, keyModifiers, cb);
> -      break;
> +        break;
>  
> -    case "moveByOffset":
> -      this.generateEvents(
> -          "move",
> +      case action.PointerMove:
> +        assertPositiveInteger(actionItem.duration, "duration");
> +        // TODO? https://github.com/w3c/webdriver/issues/422

This bug is now fixed.

::: testing/marionette/action.js:263
(Diff revision 8)
> -          touchId,
> -          null,
> -          keyModifiers);
> -      this.actions(chain, touchId, i, keyModifiers, cb);
> -      break;
> +            !element.isWebElementReference(actionItem.element)) {
> +          throw new InvalidArgumentError(
> +              "Expected 'actionItem.element' to be a web element reference, " +
> +              `got: ${actionItem.element}`);
> +        }
> +        // TODO turn the web element reference into an element by looking

That shouldn't happen until the action is actually run though, since earlier actions could invalidate the reference.

::: testing/marionette/action.js:323
(Diff revision 8)
> -            touch.clientY,
> -            null,
> -            null,
> -            keyModifiers);
> -      }
> +        }
> -      this.lastCoordinates = null;
> +        actionsByTick[i].push(inputSourceActions[i]);

What about the case where not all ipnut sources have the same number of actions?

Comment 41

3 years ago
mozreview-review
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review88440
Attachment #8791840 - Flags: review?(james) → review+
Assignee

Comment 42

3 years ago
mozreview-review-reply
Comment on attachment 8791840 [details]
Bug 1303234 - Implement extracting action chain from a request;

https://reviewboard.mozilla.org/r/79108/#review88430

> Seems slightly odd that this has a type property but not any of the others?

They all inherit a type property from InputState. In this case we override it.

> What about the case where not all ipnut sources have the same number of actions?

That works. See `test_extractActionChain_twoAndThreeTicks`.
Comment hidden (mozreview-request)

Comment 44

3 years ago
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5ea056e363e
Implement extracting action chain from a request; r=ato,jgraham

Comment 45

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5ea056e363e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Sheriffs: Requesting uplift of this to Aurora and Beta as a test-only change.
Whiteboard: [checkin-needed-aurora] [checkin-needed-beta]
Looks like this didn't get uplifted to Beta before it got merged to Release today. I'm going to call this wontfix for 50 at this point, but feel free to argue for it if you feel it's critical enough to warrant uplift there still.
Whiteboard: [checkin-needed-aurora] [checkin-needed-beta] → [checkin-needed-aurora]
(In reply to Ryan VanderMeulen [:RyanVM] from comment #47)
> Looks like this didn't get uplifted to Beta before it got merged to Release
> today. I'm going to call this wontfix for 50 at this point, but feel free to
> argue for it if you feel it's critical enough to warrant uplift there still.

Thanks Ryan.  It is not critical for 50.
You need to log in before you can comment on or make changes to this bug.