Andreas Tolfsen ❲:ato❳ Assignee
	Description • 10 years ago

The Marionette server handles requests separately with a global sense of state which makes it hard to introduce generalised behaviour to many commands.  This effectively slows down protocol implementation because each command request individually needs to do heavy lifting.

A possible solution to this is to make abstractions that call down to the protocol implementation to hide the details that are unimportant.  This would relieve – and to some extent separate – the WebDriver implementation from the infrastructural parts.

It would be advantageous to build a more expressive architecture through a series of refactorings to address these issues, as well as making Marionette more readable and approachable.  Reducing complexity is a long term goal, but we are currently lacking the correct design to make that an achievable goal.

Specifically we should aim to do the following:

- Request handling and command dispatching
- Error handling and ensuring uncaught errors are handled
- Introduce changes that make it easier to implement the wire protocol as defined in the specification
- Investigate and possibly suggest a less laborious chrome/content communication model

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 9 years ago

pushlog: https://hg.mozilla.org/try/pushloghtml?changeset=1d217e25e6e0
try desktop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d217e25e6e0
try mobile: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4209addece2

	Chris Manchester (limited bugmail, email directly)
	Comment 2 • 9 years ago

(In reply to Andreas Tolfsen (:ato) from comment #1)
> pushlog: https://hg.mozilla.org/try/pushloghtml?changeset=1d217e25e6e0
> try desktop:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d217e25e6e0
> try mobile:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4209addece2

https://hg.mozilla.org/try/rev/71912d44d6cc#l1.204

s/finalUIStartup/finalUiStartup/

	Jonathan Griffin (:jgriffin)
	Comment 3 • 9 years ago

The problem I currently see in the latest try push:

JavaScript error: chrome://marionette/content/cmdproc.js, line 1: Error: chrome://marionette/content/error.js - EXPORTED_SYMBOLS is not an array.

is almost certainly caused by needing to express EXPORTED_SYMBOLS as this.EXPORTED_SYMBOLS for B2G.  B2G reuses the global scope, so 'this.' is needed to force EXPORTED_SYMBOLS into the file scope being used.  See bug 798491 for more context.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 9 years ago

Brief update on the progress:  Fixed many issues.  Tests are now running on B2G, but I expect ~80 failures or so.

pushlog: https://hg.mozilla.org/try/pushloghtml?changeset=7c5049dbb1b4
try desktop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c5049dbb1b4

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 9 years ago

try desktop, with fix for marionette-frame-manager.js, shows expected results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=511665f0b699

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 9 years ago

try B2G emulators, including hidden jobs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e108a41440&exclusion_state=all

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 7 • 9 years ago

try desktop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b45d08941772&exclusion_state=all
try mobile: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba0e2a7aa926&exclusion_state=all

Some issues with ScriptTimeoutError not being thrown in executeScript in chrome context.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 8 • 9 years ago

try desktop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da65548a4f39
try mobile: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b22e90e493fd

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 9 • 9 years ago

try desktop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=914275b9c36c
try mobile: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7509d0739f77
pushlog: https://hg.mozilla.org/try/pushloghtml?changeset=d1cc07eaf969

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb30b176a5a4

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 11 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94277928022&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b2eaf940af&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 13 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca6f8d4c23b8&exclusion_profile=false

Note that Mn and Mnw on B2G ICS are disabled on Treeherder.  Mn is failing spectacularly on m-c and Mnw are showing approximately the same number of failures (202/8/2 vs. 202/6/2).

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 14 • 9 years ago

Rebased patch series, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e55f6a99edf5&exclusion_profile=false

	Chris Manchester (limited bugmail, email directly)
	Comment 15 • 9 years ago

This is a bit ugly (and rough), but ports bug 1096488 to apply on top of the patch series in a way that passes the tests of interests there with e10s on.

	Chris Manchester (limited bugmail, email directly)
	Comment 16 • 9 years ago

Thanks bzexport, but no thanks!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 17 • 9 years ago

try with :chmanchester's rebased remoteness change diff applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a7dadcfca3&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 18 • 9 years ago

try with modal dialogues fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45022498ddb&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 19 • 9 years ago

try with emulator callbacks fixed yet again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6b4ba4173b3&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 20 • 9 years ago

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5461 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 68d42c9da326dd70453488a34501bc0b6d94334a

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 21 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5461 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 68d42c9da326dd70453488a34501bc0b6d94334a

	Chris Manchester (limited bugmail, email directly)
	Comment 22 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4393

::: testing/marionette/marionette-server.js
(Diff revision 1)
> -                              command_id, this.context);
> +                              command_id, true /* ignore visibility check */);

Do we usually put comments next to arguments like this? I haven't seen it around.

	Chris Manchester (limited bugmail, email directly)
	Comment 23 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4395

::: testing/marionette/error.js
(Diff revision 1)
> +  if (obj === null || typeof obj != "object")

I don't think it's a rule written down anywhere, but I am strongly inclined to use braces in cases like these.

::: testing/marionette/error.js
(Diff revision 1)
> +    Cu.reportError(msg);

This is the only place in this file I see a dependency on chrome, is this required?

::: testing/marionette/error.js
(Diff revision 1)
> + * Checks if obj is an instance of the Error prototype in a safely manner.

nit: s/safely/safe/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 24 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4397

> Do we usually put comments next to arguments like this? I haven't seen it around.

I find that for functions with very long argument sequences it helps to say what an argument is for if it's not obvious from its name or type.

I'm not suggesting we make a rule of that.  Ideally the number of arguments to a function will be short and easy to remember.  When they aren't it's usually a warning sign (like this case) that there are too many arguments.

	Chris Manchester (limited bugmail, email directly)
	Comment 25 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4403

::: testing/marionette/command.js
(Diff revision 1)
> +    }.bind(this)).then(resolve, reject);

spawn returns a promise, wouldn't it be equivalent to remove the additional Promise layer and have .then(resp.send.bind(resp.send.bing(resp) ... right here?

	Chris Manchester (limited bugmail, email directly)
	Comment 26 • 9 years ago

https://reviewboard.mozilla.org/r/5449/#review4419

::: testing/marionette/dispatcher.js
(Diff revision 1)
> +loader.loadSubScript("resource://gre/modules/devtools/transport/transport.js");

Is there anything in this file from transport.js?

::: testing/marionette/dispatcher.js
(Diff revision 1)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

I don't see anything in this file using XPCOMUtils

	David Burns :automatedtester
	Comment 27 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4439

::: testing/marionette/marionette-server.js
(Diff revision 1)
> -    if (this.context == "chrome") {
> +    switch (this.context) {

Hopefully this is just a mozreview thing but can you indent you `case` statements

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    case Context.CHROME:

Hopefully this is just a mozreview thing but can you indent you `case` statements

::: testing/marionette/marionette-server.js
(Diff revision 1)
> -    if (this.context == "chrome") {
> +    switch (this.context) {

Hopefully this is just a mozreview thing but can you indent you `case` statements

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    case Context.CHROME:

as above

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    switch (this.context) {

and here :)

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    switch (this.context) {

Check the rest of the file too, not going to flag all of them :)

	Chris Manchester (limited bugmail, email directly)
	Comment 28 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4423

::: testing/marionette/components/marionettecomponent.js
(Diff revision 1)
> -  this._loaded = false;
> +  this.loaded_ = false;

I haven't seen a trailing underscore around our codebase anywhere.

::: testing/marionette/components/marionettecomponent.js
(Diff revision 1)
> -      aWindow.removeEventListener("load", onLoad);
> +    win.removeEventListener("load", this);

I don't think "this" is what you want here.

	Chris Manchester (limited bugmail, email directly)
	Comment 29 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

https://reviewboard.mozilla.org/r/5441/#review4445

::: testing/marionette/server.js
(Diff revision 1)
> +Cu.import("resource://gre/modules/FileUtils.jsm");

I don't see FileUtils used in this file.

::: testing/marionette/server.js
(Diff revision 1)
> +  Services.prefs.setBoolPref(SPECIAL_POWERS_PREF, true);

It looks like the circumstances where this is set changes with this patch.

::: testing/marionette/dispatcher.js
(Diff revision 1)
> +  if (Services.appinfo.name != "Firefox") {

I get the impression appName isn't the same as Services.appinfo.name for mulet, so this has changed slightly.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 30 • 9 years ago

https://reviewboard.mozilla.org/r/5449/#review4443

::: testing/marionette/dispatcher.js
(Diff revision 1)
> + *     A factory function that takes an Emulator as arguments and produces

s/arguments/argument/

::: testing/marionette/dispatcher.js
(Diff revision 1)
> + * Requests handlers defined in this.requests take presedence

s/Requests/Request/

::: testing/marionette/dispatcher.js
(Diff revision 1)
> + * Responses from commands as well as messages from listener.

This documentation is just appalling.

::: testing/marionette/dispatcher.js
(Diff revision 1)
> + * cmdId is a unique identifier assigned to the client's request

Again, documentation isn't great.  Should mention why emulator callbacks are handled specially (i.e. they are “transparent”) and why `this.commandId` is only reset for client responses.

::: testing/marionette/dispatcher.js
(Diff revision 1)
> + * Sends given payload as-is to the connected client

s/Sends/Send/

Documentation should also include that it does a series of checks, which `sendRaw` skips.

::: testing/marionette/dispatcher.js
(Diff revision 1)
> +Dispatcher.prototype.beginNewCommand = function() {

Documentation.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 31 • 9 years ago

https://reviewboard.mozilla.org/r/5457/#review4447

::: testing/marionette/marionette-simpletest.js
(Diff revision 1)
> +Cu.import("chrome://marionette/content/emulator.js");

Unused imports

	David Burns :automatedtester
	Comment 32 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4441

::: testing/marionette/error.js
(Diff revision 1)
> +  this.status = "";  // not defined in spec

defined as `no such alert`. See w3c.github.io/webdriver/webdriver-spec.html#handling-errors

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 33 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4449

::: testing/marionette/command.js
(Diff revision 1)
> + * @param {(Error|Object)} err The error to send, either an instance of

Line break

::: testing/marionette/command.js
(Diff revision 1)
> + */

Argument docs

::: testing/marionette/command.js
(Diff revision 1)
> +CommandProcessor.prototype.execute = function(

Argument docs

::: testing/marionette/command.js
(Diff revision 1)
> +    {sessionId: this.driver.sessionId});

Assign {sessionId: …} to a legible variable first.

::: testing/marionette/command.js
(Diff revision 1)
> +  get name() { return this.data.get("name") },

Missing lots of semicolons

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 34 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5461 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 68d42c9da326dd70453488a34501bc0b6d94334a

	Chris Manchester (limited bugmail, email directly)
	Comment 35 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4451

::: testing/marionette/driver.js
(Diff revision 1)
> +      for (let [t,o] of this) {
> +        Services.obs.addObserver(o, t, false);
> +      }

Doing this here means we don't handle modals that arise as a result of anything happening in chrome scope.

::: testing/marionette/driver.js
(Diff revision 1)
> +    let obs = new Map();

This doesn't look like it should be a Map.

	David Burns :automatedtester
	Comment 36 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4453

::: testing/marionette/command.js
(Diff revision 1)
> +CommandProcessor.prototype.execute = function(

can you put the function signature on one line to make it easier to read. It puts the line to 87 chars and I am ok with that :)

	David Burns :automatedtester
	Comment 37 • 9 years ago

https://reviewboard.mozilla.org/r/5461/#review4455

Ship It!

	David Burns :automatedtester
	Comment 38 • 9 years ago

https://reviewboard.mozilla.org/r/5459/#review4457

Ship It!

	David Burns :automatedtester
	Comment 39 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4461

::: testing/marionette/components/marionettecomponent.js
(Diff revision 1)
> -  onSocketAccepted: function mc_onSocketAccepted(aSocket, aTransport) {
> +MarionetteComponent.prototype.onSocketAccepted = function(so, transport) {

s/so/socket

the original argument name was more legible

::: testing/marionette/components/marionettecomponent.js
(Diff revision 1)
> -  onStopListening: function mc_onStopListening(aSocket, status) {
> +MarionetteComponent.prototype.onStopListening = function(so, status) {

s/so/socket

the original argument name was more legible

::: testing/marionette/server.js
(Diff revision 1)
> +MarionetteServer.prototype.onSocketAccepted = function(serverSo, clientSo) {

can we put socket instead of so to make this more readable and then people can understand what we are expecting

::: testing/marionette/server.js
(Diff revision 1)
> +Cu.import("resource://gre/modules/NetUtil.jsm");

Is this import required? Can't see it being used?

::: testing/marionette/server.js
(Diff revision 1)
> +

Does this need documentation? Well, do all the methods in this function require docs?

	David Burns :automatedtester
	Comment 40 • 9 years ago

https://reviewboard.mozilla.org/r/5455/#review4465

Ship It!

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 41 • 9 years ago

https://reviewboard.mozilla.org/r/5453/#review4489

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 42 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5461 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 17b496358653c4326159c681762347c9faaa5cd0

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 43 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4491

> It looks like the circumstances where this is set changes with this patch.

It used to be set on `MarionetteServerConnection.prototype.newSession`.  Now it's set only once for every instance of `MarionetteServer`.  Do you think it's a problem?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 44 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5461 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r aaff6848958a580e2f65fe04ca742085cfe333c6

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 45 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4493

> I don't think it's a rule written down anywhere, but I am strongly inclined to use braces in cases like these.

I don't care either way.  Changed all instances I could find to be wrapped in curly braces.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 46 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4495

> This is the only place in this file I see a dependency on chrome, is this required?

This is the same as what devtools do for their error reporting.  As I understand it, it means Marionette errors will appear in the Browser Console which can be useful if you have the debugger attached.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 47 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G

Pull down these commits:

hg pull review -r f15a90f8c61b19ed490b3824ceef371bf2fa9c95

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 48 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4497

> can you put the function signature on one line to make it easier to read. It puts the line to 87 chars and I am ok with that :)

OK, I was just following the Google Javascript Style Guide here.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 49 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 12957f71543ea7c7fe43e45a8b2a42a065ea3446

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 50 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 61a501e238e7e1ffc3fa5e74483da332723f15af

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 51 • 9 years ago

https://reviewboard.mozilla.org/r/5449/#review4501

> Is there anything in this file from transport.js?

Nope, removing.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 52 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4505

> I haven't seen a trailing underscore around our codebase anywhere.

I was following the Google JavaScript Style Guide (for the lack of any such equivalent at Mozilla) which says private properties should have a trailing underscore.  I found some support for this in the Mozilla codebase.  (https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#JavaScript_Style_Rules)

I'm happy to change it to be the other way around, but there doesn't seem to be concensus in our codebase in any direction on this.

> I don't think "this" is what you want here.

Isn't `this` the function scope?  The function doesn't rebind `this`.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 53 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4507

> This doesn't look like it should be a Map.

It's a map so that we can use the `for (let [k,v] of obs)` iterator.  What data structure do you think would be better?

> Doing this here means we don't handle modals that arise as a result of anything happening in chrome scope.

This isn't currently a problem as the modals arise from content events.  At least test_modal_dialogs.py is passing.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 54 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 2c5b9285bf09b3cedc076de35589440777b3cbfb

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 55 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 914cd77db333580d1a245b3c4d348553e0a462ed

	Chris Manchester (limited bugmail, email directly)
	Comment 56 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4523

> It used to be set on `MarionetteServerConnection.prototype.newSession`.  Now it's set only once for every instance of `MarionetteServer`.  Do you think it's a problem?

I'm not sure, but I would double check.

	Chris Manchester (limited bugmail, email directly)
	Comment 57 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4529

> This isn't currently a problem as the modals arise from content events.  At least test_modal_dialogs.py is passing.

I can imagine this being significant for firefox-ui-tests, maybe not immediately.

	Chris Manchester (limited bugmail, email directly)
	Comment 58 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4525

> Isn't `this` the function scope?  The function doesn't rebind `this`.

I'd wager "this" is undefined at this point.

	Chris Manchester (limited bugmail, email directly)
	Comment 59 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4527

::: testing/marionette/driver.js
(Diff revision 1)
> +    let obs = new Map();

Oh, I misread. The map works :)

	Chris Manchester (limited bugmail, email directly)
	Comment 60 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4533

::: testing/marionette/driver.js
(Diff revision 7)
> +  let listenerWindow = Services.wm.getOuterWindowWithId(id);

I ran into problems with this and e10s, did you establish this isn't a problem?

::: testing/marionette/driver.js
(Diff revision 7)
> +  let {inactivityTimeout: inactivityTimeout,
> +       scriptTimeout: timeout,
> +       script: script,
> +       newSandbox: newSandbox,
> +       args: args,
> +       specialPowers: specialPowers,
> +       filename: filename,
> +       line: line} = cmd.parameters;

You don't have to restate the property names unless you want to rename them locally, this can be let {inactivityTimeout, scriptTimeout, ... } = cmd.parameters;

::: testing/marionette/driver.js
(Diff revision 7)
> +GeckoDriver.prototype.registerPromise = Task.async(function*() {
> +  const li = "Marionette:register";
> +
> +  yield new Promise((resolve) => {

This Task isn't doing a lot that the promise isn't -- this would probably work as a regular function that returns the promise immediately without the Task.async part. It looks like all that happens with the Promse is it gets constructed and then a little later the caller blocks on it.

I noticed this pattern a few other places, I think the comment applies there too.

	Jonathan Griffin (:jgriffin)
	Comment 61 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

https://reviewboard.mozilla.org/r/5441/#review4531

::: testing/marionette/dispatcher.js
(Diff revision 8)
> +const loader = Cc["@mozilla.org/moz/jssubscript-loader;1"]

loader is unused in this file.

::: testing/marionette/marionette-simpletest.js
(Diff revision 8)
>              'runEmulatorCmd', 'runEmulatorShell', 'TEST_PASS', 'TEST_KNOWN_FAIL',

the emulator methods can be removed from exports.

::: testing/marionette/driver.js
(Diff revision 8)
> +    

nit: extra whitespace

So, this is pretty complicated patch, and it wouldn't surprise me if I missed something.  That said, the proof is in the pudding, and if it passes try and sticks on landing, then any other issues can be dealt with in follow-ups!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 62 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 55b502aadaa0964fd2e4e6a1167e54685df60849

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 63 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

Carrying on r+ from jmaher and jgriffin because there's a bug with mozreview when rewriting history and pushing that resets to r?.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 64 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4597

> You don't have to restate the property names unless you want to rename them locally, this can be let {inactivityTimeout, scriptTimeout, ... } = cmd.parameters;

Thanks for pointing this out!  I've fixed up all of the instances where it makes sense to drop the renames.

There are a few cases where we still have to, but those can more than likely be fixed when we upgrade the listener to also use the new dispatching strategy.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 65 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4599

> I ran into problems with this and e10s, did you establish this isn't a problem?

It doesn't appear to be when I run `./mach marionette-test` with `--e10s`.  But I'll be sure to include marionette-e10s in my next try run to be sure.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 66 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4603

> This Task isn't doing a lot that the promise isn't -- this would probably work as a regular function that returns the promise immediately without the Task.async part. It looks like all that happens with the Promse is it gets constructed and then a little later the caller blocks on it.
> 
> I noticed this pattern a few other places, I think the comment applies there too.

I agree.  It would've been different if `registerPromise` had to wait for the promise and then complete another action before returning.  Just returning the promise makes sense.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 67 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 03ee5852be3eff9cab47d492042a31b686388eac

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 68 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G

Pull down these commits:

hg pull review -r 03ee5852be3eff9cab47d492042a31b686388eac

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 69 • 9 years ago

try run with e10s: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1cb5750712d&exclusion_profile=false

	David Burns :automatedtester
	Comment 70 • 9 years ago

https://reviewboard.mozilla.org/r/5533/#review4627

Ship It!

	David Burns :automatedtester
	Comment 71 • 9 years ago

https://reviewboard.mozilla.org/r/5459/#review4629

Ship It!

	Chris Manchester (limited bugmail, email directly)
	Comment 72 • 9 years ago

(In reply to Andreas Tolfsen (:ato) from comment #69)
> try run with e10s:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f1cb5750712d&exclusion_profile=false

mn-e10s is only linux32 :/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 73 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support

Pull down these commits:

hg pull review -r c26d9f95ea3f71494c0d7720ca0cf109a6a0b578

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 74 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4703

> I'm not sure, but I would double check.

All the tests pass, so I'm assuming it's fine.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 75 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support

Pull down these commits:

hg pull review -r c26d9f95ea3f71494c0d7720ca0cf109a6a0b578

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 76 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4705

> I can imagine this being significant for firefox-ui-tests, maybe not immediately.

Thanks for this feedback.  I've fixed this now in part 11.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 77 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support

Pull down these commits:

hg pull review -r 09d5857aea8fc164668db0ca9d3363689e4ab764

	Chris Manchester (limited bugmail, email directly)
	Comment 78 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4707

> All the tests pass, so I'm assuming it's fine.

There was a bug recently about this, but I can't seem to find it now. The pref has implications outside of tests failing if it's set more often than it should be.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 79 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4711

> There was a bug recently about this, but I can't seem to find it now. The pref has implications outside of tests failing if it's set more often than it should be.

Reopening but I'm not sure what to check.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 80 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files

Pull down these commits:

hg pull review -r 1977dc2838f18b9da7eeea7b2bb8c37295fb3d90

	Chris Manchester (limited bugmail, email directly)
	Comment 81 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4713

> Reopening but I'm not sure what to check.

I'd just double check with jgriffin. Although I guess he already looked at this, so I'm probably just being paranoid.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 82 • 9 years ago

try run with 32-bit Linux (for marionette-e10s): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5f2e266a7e&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 83 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4715

> I'd wager "this" is undefined at this point.

Good you noticed, fixed this.

	David Burns :automatedtester
	Comment 84 • 9 years ago

https://reviewboard.mozilla.org/r/5533/#review4717

Ship It!

	David Burns :automatedtester
	Comment 85 • 9 years ago

https://reviewboard.mozilla.org/r/5459/#review4719

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 86 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

Carrying forward r+ from jgriffin.

	David Burns :automatedtester
	Comment 87 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4727

Ship It!

	David Burns :automatedtester
	Comment 88 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4733

Ship It!

	David Burns :automatedtester
	Comment 89 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4729

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    case Context.CHROME:

indent case statements so they are not directly under the switch

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    case Context.CONTENT:

and here

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    case Context.CHROME:

indent case so that it is not directly under the switch

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    case Context.CONTENT:

indent

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    case Context.CHROME:

Please check switch statements throughout this file.

	Chris Manchester (limited bugmail, email directly)
	Comment 90 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

https://reviewboard.mozilla.org/r/5441/#review4731

::: testing/marionette/modal.js
(Diff revision 13)
> +        handlers.splice(i, 1);

Modifying the array while you're iterating over it in this fashion could cause a problem if you had lot more handlers, right? Maybe you can use a Set for this instead.

	David Burns :automatedtester
	Comment 91 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4735

Ship It!

	David Burns :automatedtester
	Comment 92 • 9 years ago

https://reviewboard.mozilla.org/r/5449/#review4737

Ship It!

	David Burns :automatedtester
	Comment 93 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4739

::: testing/marionette/driver.js
(Diff revision 10)
> +      if (this.curFrame)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +  if (cmdId)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +  if (file.exists())

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +  if (mainContent)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +    if (this.curBrowser.isNewSession)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +  if (ctx === null)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +  if (!scriptTimeout)

missing braces

::: testing/marionette/driver.js
(Diff revision 10)
> +        if (error.isError(val))

missing braces, there are quite a few through this file so do a check

::: testing/marionette/driver.js
(Diff revision 10)
> +  case Context.CHROME:

indent case to match other methods e.g L1919

::: testing/marionette/components/marionettecomponent.js
(Diff revision 10)
> -          this.logger.info("marionette enabled via build flag and pref");
> +  

a few trailing whitespaces found through the document

	David Burns :automatedtester
	Comment 94 • 9 years ago

https://reviewboard.mozilla.org/r/5455/#review4741

Ship It!

	David Burns :automatedtester
	Comment 95 • 9 years ago

https://reviewboard.mozilla.org/r/5735/#review4745

Ship It!

	David Burns :automatedtester
	Comment 96 • 9 years ago

https://reviewboard.mozilla.org/r/5737/#review4747

::: testing/marionette/common.js
(Diff revision 1)
> +  

Nit: what space

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 97 • 9 years ago

https://reviewboard.mozilla.org/r/5441/#review4751

> Modifying the array while you're iterating over it in this fashion could cause a problem if you had lot more handlers, right? Maybe you can use a Set for this instead.

Excellent point.  I've updated this to use sets.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 98 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4753

> I find that for functions with very long argument sequences it helps to say what an argument is for if it's not obvious from its name or type.
> 
> I'm not suggesting we make a rule of that.  Ideally the number of arguments to a function will be short and easy to remember.  When they aren't it's usually a warning sign (like this case) that there are too many arguments.

It's consistent across Marionette at least.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 99 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4755

> indent case statements so they are not directly under the switch

This is fixed in part 5.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 100 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4757

> I was following the Google JavaScript Style Guide (for the lack of any such equivalent at Mozilla) which says private properties should have a trailing underscore.  I found some support for this in the Mozilla codebase.  (https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#JavaScript_Style_Rules)
> 
> I'm happy to change it to be the other way around, but there doesn't seem to be concensus in our codebase in any direction on this.

At least it's now consistent.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 101 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes

Pull down these commits:

hg pull review -r fcdf56246d2cb3496bac7ec1e24e9b4e3c9a863b

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 102 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a6331516ee&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 103 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes

Pull down these commits:

hg pull review -r a06a9e20403939480f44696050530dbe9e7b9f55

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 104 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

rebased try, which should fix Mac OS X 10.6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f0e53865204&exclusion_profile=false

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 105 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes
/r/5805 - Bug 1107706: Part 14: Fix quitApplication

Pull down these commits:

hg pull review -r 7179c03849a9e9d89c45edcdb0351be6335ff809

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 106 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4849bc67b7&exclusion_profile=false

	Chris Manchester (limited bugmail, email directly)
	Comment 107 • 9 years ago

https://reviewboard.mozilla.org/r/5805/#review4781

::: testing/marionette/dispatcher.js
(Diff revision 1)
> +  this.stopSignal_();
> +  this.sendOk(id);
> +
>    this.driver.sessionTearDown();
>    Services.startup.quit(flags);

This ends up a bit different from where we ended up in 1142404, but as long as the server's listener is closed before the client starts trying to reconnect we're ok, which will be ok as long as closing the listener is synchronous, which I think it probably is.

	Chris Manchester (limited bugmail, email directly)
	Comment 108 • 9 years ago

https://reviewboard.mozilla.org/r/5735/#review4783

::: testing/marionette/modal.js
(Diff revision 4)
> +      if (win && win.parent)

Braces!

::: testing/marionette/modal.js
(Diff revision 4)
> +    if (win)
> +      return win.Dialog.ui;

Braces :)

::: testing/marionette/driver.js
(Diff revision 4)
> +    if (topic == modal.COMMON_DIALOG_LOADED)
> +      winr = Cu.getWeakReference(subject);

Braces.

	Chris Manchester (limited bugmail, email directly)
	Comment 109 • 9 years ago

https://reviewboard.mozilla.org/r/5805/#review4785

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 110 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes
/r/5805 - Bug 1107706: Part 14: Fix quitApplication
/r/5865 - Bug 1107706: Part 15: Fix emulator callbacks for content process

Pull down these commits:

hg pull review -r ab6bb2cf63f9434d8bd614f71a9685781a3d2737

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 111 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51967d7567c4&exclusion_profile=false

	Chris Manchester (limited bugmail, email directly)
	Comment 112 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

https://reviewboard.mozilla.org/r/5441/#review4813

Meant to r+ this before. Happy to check anything else before landing.

	David Burns :automatedtester
	Comment 113 • 9 years ago

https://reviewboard.mozilla.org/r/5443/#review4857

Ship It!

	David Burns :automatedtester
	Comment 114 • 9 years ago

https://reviewboard.mozilla.org/r/5445/#review4859

Ship It!

	David Burns :automatedtester
	Comment 115 • 9 years ago

https://reviewboard.mozilla.org/r/5447/#review4861

Ship It!

	David Burns :automatedtester
	Comment 116 • 9 years ago

https://reviewboard.mozilla.org/r/5449/#review4863

Ship It!

	David Burns :automatedtester
	Comment 117 • 9 years ago

https://reviewboard.mozilla.org/r/5451/#review4865

Ship It!

	David Burns :automatedtester
	Comment 118 • 9 years ago

https://reviewboard.mozilla.org/r/5455/#review4867

Ship It!

	David Burns :automatedtester
	Comment 119 • 9 years ago

https://reviewboard.mozilla.org/r/5457/#review4743

Ship It!

::: testing/marionette/emulator.js
(Diff revision 11)
> +    if (!this.onresult)

missing braces

	David Burns :automatedtester
	Comment 120 • 9 years ago

https://reviewboard.mozilla.org/r/5459/#review4869

Ship It!

	David Burns :automatedtester
	Comment 121 • 9 years ago

https://reviewboard.mozilla.org/r/5533/#review4871

Ship It!

	David Burns :automatedtester
	Comment 122 • 9 years ago

https://reviewboard.mozilla.org/r/5735/#review4873

Ship It!

	David Burns :automatedtester
	Comment 123 • 9 years ago

https://reviewboard.mozilla.org/r/5737/#review4875

Ship It!

	David Burns :automatedtester
	Comment 124 • 9 years ago

https://reviewboard.mozilla.org/r/5787/#review4879

Ship It!

	David Burns :automatedtester
	Comment 125 • 9 years ago

https://reviewboard.mozilla.org/r/5805/#review4881

Ship It!

	David Burns :automatedtester
	Comment 126 • 9 years ago

https://reviewboard.mozilla.org/r/5865/#review4883

Ship It!

	David Burns :automatedtester
	Comment 127 • 9 years ago

https://reviewboard.mozilla.org/r/5453/#review4885

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 128 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes
/r/5805 - Bug 1107706: Part 14: Fix quitApplication
/r/5865 - Bug 1107706: Part 15: Fix emulator callbacks for content process

Pull down these commits:

hg pull review -r 7a7d1bddb0cff5fecd1524cddc27edabe98345ae

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 129 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes
/r/5805 - Bug 1107706: Part 14: Fix quitApplication
/r/5865 - Bug 1107706: Part 15: Fix emulator callbacks for content process

Pull down these commits:

hg pull review -r 8101a841bd878d7c7377662beca08ad9287c14c9

	David Burns :automatedtester
	Comment 130 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

https://reviewboard.mozilla.org/r/5441/#review4889

Ship It!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 131 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

/r/5443 - Bug 1107706: Part 1: Change context from strings to enum
/r/5445 - Bug 1107706: Part 2: Add error module and WebDriver error objects
/r/5447 - Bug 1107706: Part 3: Add a command processor to Marionette
/r/5449 - Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
/r/5451 - Bug 1107706: Part 5: Refactor Marionette chrome/content communication
/r/5453 - Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
/r/5455 - Bug 1107706: Part 7: Add timeout test for async scripts
/r/5457 - Bug 1107706: Part 8: Adapt emulator callbacks
/r/5459 - Bug 1107706: Part 9: Disable XUL component tests on B2G
/r/5533 - Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
/r/5735 - Bug 1107706: Part 11: Global modal dialogue support
/r/5737 - Bug 1107706: Part 12: Drop marionette-* prefix on files
/r/5787 - Bug 1107706: Part 13: Style fixes
/r/5805 - Bug 1107706: Part 14: Fix quitApplication
/r/5865 - Bug 1107706: Part 15: Fix emulator callbacks for content process

Pull down these commits:

hg pull review -r 9a6ec3e07ddd8cb79c84839af7dfb1cd862e578d

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 132 • 9 years ago

Comment on attachment 8578113 [details]
MozReview Request: bz://1107706/ato

Carrying on r+ from chmanchester, dburns, and jgriffin.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 133 • 9 years ago

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3c25064e24da
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3c25064e24da

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 134 • 9 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c44d46087f59 for Mn and Mn-e10s bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=7963798&repo=mozilla-inbound

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 135 • 9 years ago

Fixed the rebase of action chains in chrome space and pushed to inbound again:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d026794b4c0b
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d026794b4c0b

	Ryan VanderMeulen [:RyanVM]
	Comment 136 • 9 years ago

Backed out for Gaia JS integration test permafail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2c50594967

https://treeherder.mozilla.org/logviewer.html#?job_id=7995011&repo=mozilla-inbound

	Ryan VanderMeulen [:RyanVM]
	Comment 137 • 9 years ago

Also B2G Desktop mochitest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=7998009&repo=mozilla-inbound

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 138 • 9 years ago

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=821bb2b2dca6
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=821bb2b2dca6

	Ryan VanderMeulen [:RyanVM]
	Comment 139 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/31df2186195a
https://hg.mozilla.org/mozilla-central/rev/42f14f453f1f
https://hg.mozilla.org/mozilla-central/rev/a76f6b26484d
https://hg.mozilla.org/mozilla-central/rev/dbd619c23f57
https://hg.mozilla.org/mozilla-central/rev/6613699885fa
https://hg.mozilla.org/mozilla-central/rev/8b695334df94
https://hg.mozilla.org/mozilla-central/rev/853e7da58109
https://hg.mozilla.org/mozilla-central/rev/4ef84ad178b5
https://hg.mozilla.org/mozilla-central/rev/6af978a7ee32
https://hg.mozilla.org/mozilla-central/rev/6dcbd23298de
https://hg.mozilla.org/mozilla-central/rev/cef97fe74c4a
https://hg.mozilla.org/mozilla-central/rev/59ad68ff1972
https://hg.mozilla.org/mozilla-central/rev/800504a8ac36
https://hg.mozilla.org/mozilla-central/rev/b46060440357
https://hg.mozilla.org/mozilla-central/rev/0736c222ecb9
https://hg.mozilla.org/mozilla-central/rev/821bb2b2dca6