Closed Bug 1107706 Opened 10 years ago Closed 10 years ago

Refactor server by introducing a more expressive internal architecture

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

(Keywords: pi-marionette-big, pi-marionette-server, Whiteboard: [marionette=1.0])

Attachments

(17 files, 1 obsolete file)

Part 5a: Port bug 1096488 to the new architecture
20.72 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1107706: Part 3: Add a command processor to Marionette
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 1: Change context from strings to enum
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 2: Add error module and WebDriver error objects
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 11: Global modal dialogue support
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 15: Fix emulator callbacks for content process
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 5: Refactor Marionette chrome/content communication
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 14: Fix quitApplication
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 7: Add timeout test for async scripts
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 8: Adapt emulator callbacks
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 9: Disable XUL component tests on B2G
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 6: Make SpecialPowersError a prototypal Error
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 12: Drop marionette-* prefix on files
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 13: Style fixes
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 10: Disable test_anonymous_content.py on B2G
39 bytes, text/x-review-board-request
ato
: review+
Details
MozReview Request: Bug 1107706: Part 4: Add dispatching mechanism to encapsulate connection
39 bytes, text/x-review-board-request
ato
: review+
Details
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
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: 1100545
Blocks: 945729
Priority: -- → P1
Whiteboard: [marionette=1.0]
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.
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
try desktop, with fix for marionette-frame-manager.js, shows expected results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=511665f0b699
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.
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).
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.
Assignee: ato → cmanchester
Thanks bzexport, but no thanks!
Assignee: cmanchester → ato
try with :chmanchester's rebased remoteness change diff applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a7dadcfca3&exclusion_profile=false
Attached file MozReview Request: bz://1107706/ato (obsolete) —
/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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(dburns)
Attachment #8578113 - Flags: review?(cmanchester)
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
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.
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/
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.
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?
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
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 :)
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.
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.
Attachment #8578113 - Flags: review?(cmanchester)
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.
https://reviewboard.mozilla.org/r/5457/#review4447 ::: testing/marionette/marionette-simpletest.js (Diff revision 1) > +Cu.import("chrome://marionette/content/emulator.js"); Unused imports
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
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
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
Attachment #8578113 - Flags: review?(jmaher)
Attachment #8578113 - Flags: review?(cmanchester)
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.
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 :)
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?
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
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?
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
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.
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.
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
Attachment #8578113 - Flags: review?(jmaher)
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.
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
Attachment #8578113 - Flags: review?(jmaher)
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
https://reviewboard.mozilla.org/r/5449/#review4501 > Is there anything in this file from transport.js? Nope, removing.
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`.
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.
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
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
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.
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.
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.
https://reviewboard.mozilla.org/r/5451/#review4527 ::: testing/marionette/driver.js (Diff revision 1) > + let obs = new Map(); Oh, I misread. The map works :)
Attachment #8578113 - Flags: review?(jmaher)
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.
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!
Attachment #8578113 - Flags: review?(jgriffin) → review+
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
Attachment #8578113 - Flags: review?(jmaher)
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review+
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?.
Attachment #8578113 - Flags: review?(jmaher)
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review+
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.
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.
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.
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
Attachment #8578113 - Flags: review?(jmaher)
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review+
Attachment #8578113 - Flags: review?(jmaher)
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review+
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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review+
Attachment #8578113 - Flags: review?(jgriffin) → review+
(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 :/
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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
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.
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
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.
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
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.
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.
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
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.
https://reviewboard.mozilla.org/r/5451/#review4715 > I'd wager "this" is undefined at this point. Good you noticed, fixed this.
Comment on attachment 8578113 [details] MozReview Request: bz://1107706/ato Carrying forward r+ from jgriffin.
Attachment #8578113 - Flags: review?(jgriffin) → review+
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.
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.
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
https://reviewboard.mozilla.org/r/5737/#review4747 ::: testing/marionette/common.js (Diff revision 1) > + Nit: what space
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.
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.
https://reviewboard.mozilla.org/r/5443/#review4755 > indent case statements so they are not directly under the switch This is fixed in part 5.
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.
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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
Attachment #8578113 - Flags: review?(jgriffin) → review+
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
Attachment #8578113 - Flags: review+ → review?(jgriffin)
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
Attachment #8578113 - Flags: review?(jgriffin) → review+
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
Attachment #8578113 - Flags: review+ → review?(jgriffin)
Comment on attachment 8578113 [details] MozReview Request: bz://1107706/ato try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4849bc67b7&exclusion_profile=false
Attachment #8578113 - Flags: review?(jgriffin) → review+
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.
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.
Attachment #8578113 - Flags: review+ → review?(jgriffin)
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
Comment on attachment 8578113 [details] MozReview Request: bz://1107706/ato try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51967d7567c4&exclusion_profile=false
Attachment #8578113 - Flags: review?(jgriffin) → review+
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.
Attachment #8578113 - Flags: review?(cmanchester) → review+
https://reviewboard.mozilla.org/r/5457/#review4743 Ship It! ::: testing/marionette/emulator.js (Diff revision 11) > + if (!this.onresult) missing braces
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
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
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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
Comment on attachment 8578113 [details] MozReview Request: bz://1107706/ato https://reviewboard.mozilla.org/r/5441/#review4889 Ship It!
Attachment #8578113 - Flags: review?(dburns) → review+
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
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(dburns)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
Comment on attachment 8578113 [details] MozReview Request: bz://1107706/ato Carrying on r+ from chmanchester, dburns, and jgriffin.
Attachment #8578113 - Flags: review?(jgriffin)
Attachment #8578113 - Flags: review?(dburns)
Attachment #8578113 - Flags: review?(cmanchester)
Attachment #8578113 - Flags: review+
Depends on: 1150170
Attachment #8578113 - Attachment is obsolete: true
Attachment #8618796 - Flags: review+
Attachment #8618797 - Flags: review+
Attachment #8618798 - Flags: review+
Attachment #8618799 - Flags: review+
Attachment #8618800 - Flags: review+
Attachment #8618801 - Flags: review+
Attachment #8618802 - Flags: review+
Attachment #8618803 - Flags: review+
Attachment #8618804 - Flags: review+
Attachment #8618805 - Flags: review+
Attachment #8618806 - Flags: review+
Attachment #8618807 - Flags: review+
Attachment #8618808 - Flags: review+
Attachment #8618809 - Flags: review+
Attachment #8618810 - Flags: review+
Attachment #8618811 - Flags: review+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: