From c70694472b1df0da39989dde5fa22e73f26679e6 Mon Sep 17 00:00:00 2001 From: Jonathan Lipps Date: Wed, 8 Apr 2015 17:22:59 -0700 Subject: [PATCH] update chromedriver support with callback-based functions that fit more snugly into the current paradigm --- lib/devices/android/android-hybrid.js | 70 +++++++++++---------------- lib/devices/android/chrome.js | 28 ++++++----- lib/server/proxy.js | 12 +++-- package.json | 2 +- 4 files changed, 52 insertions(+), 60 deletions(-) diff --git a/lib/devices/android/android-hybrid.js b/lib/devices/android/android-hybrid.js index 94d4e408..66e462f3 100644 --- a/lib/devices/android/android-hybrid.js +++ b/lib/devices/android/android-hybrid.js @@ -16,7 +16,6 @@ var logger = require('../../server/logger.js').get('appium') var androidHybrid = {}; androidHybrid.chromedriver = null; -androidHybrid.chromedriverStopCbs = {}; androidHybrid.sessionChromedrivers = {}; androidHybrid.listWebviews = function (cb) { @@ -134,7 +133,7 @@ androidHybrid.startChromedriverProxy = function (context, cb) { } }; -androidHybrid.setupNewChromedriver = function (context, ocb) { +androidHybrid.setupNewChromedriver = function (context, cb) { var chromeArgs = { port: this.args.chromeDriverPort, executable: this.args.chromedriverExecutable @@ -167,21 +166,19 @@ androidHybrid.setupNewChromedriver = function (context, ocb) { }); } caps = this.decorateChromeOptions(caps); - // see note in chrome.js::createSession for explanation of this pattern - this.chromedriver.once(Chromedriver.EVENT_ERROR, ocb); this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) { - if (msg.state === Chromedriver.STATE_ONLINE) { - // save the chromedriver object under the context - this.sessionChromedrivers[context] = this.chromedriver; - // let whoever called us know that we're done setting up session - ocb(); - } else if (msg.state === Chromedriver.STATE_STOPPED) { + if (msg.state === Chromedriver.STATE_STOPPED) { // bind our stop/exit handler, passing in context so we know which - // one stopped + // one stopped unexpectedly this.onChromedriverStop(context); } }.bind(this)); - this.chromedriver.start(caps); + this.chromedriver.start(caps).nodeify(function (err) { + if (err) return cb(err); + // save the chromedriver object under the context + this.sessionChromedrivers[context] = this.chromedriver; + cb(); + }.bind(this)); }; @@ -193,42 +190,25 @@ androidHybrid.setupExistingChromedriver = function (context, cb) { this.proxyReqRes = this.chromedriver.proxyReq.bind(this.chromedriver); this.isProxy = true; - // make sure that no matter how many times Chromedriver emits a changed - // state event, we only call back on the first ONLINE message - var restartCb = _.once(function () { - // once we're back online, let upstream know - this.chromedriverRestartingContext = null; - cb(); - }.bind(this)); - // check the status by sending a simple window-based command to ChromeDriver // if there is an error, we want to recreate the ChromeDriver session - this.chromedriver.hasWorkingWebview().then(function (works) { + this.chromedriver.hasWorkingWebview().nodeify(function (err, works) { + if (err) return cb(err); if (works) return cb(); logger.debug("ChromeDriver is not associated with a window. " + "Re-initializing the session."); - // catch any errors the restart process bubbles up - this.chromedriver.once(Chromedriver.EVENT_ERROR, cb); - // once the restart is successful, reset context flag and continue - this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) { - if (msg.state === Chromedriver.STATE_ONLINE) { - restartCb(); - } - }.bind(this)); this.chromedriverRestartingContext = context; - this.chromedriver.restart(); - }.bind(this)).catch(cb); + this.chromedriver.restart().nodeify(function (err) { + if (err) return cb(err); + this.chromedriverRestartingContext = null; + cb(); + }.bind(this)); + }.bind(this)); }; androidHybrid.onChromedriverStop = function (context) { - logger.debug("Chromedriver for context " + context + " stopped"); - // chromedriver isn't valid anymore, so remove it from context list - if (_.has(this.chromedriverStopCbs, context)) { - // if we intentionally stopped this context's chromedriver, we'll have a - // callback for it in this.chromedriverStopCbs - delete this.sessionChromedrivers[context]; - this.chromedriverStopCbs[context](); - } else if (context === this.curContext) { + logger.warn("Chromedriver for context " + context + " stopped unexpectedly"); + if (context === this.curContext) { // if we don't have a stop callback, we exited unexpectedly and so want // to shut down the session and respond with an error // TODO: this kind of thing should be emitted and handled by a higher-level @@ -260,10 +240,14 @@ androidHybrid.suspendChromedriverProxy = function (cb) { androidHybrid.stopChromedriverProxies = function (ocb) { async.eachSeries(Object.keys(this.sessionChromedrivers), function (context, cb) { logger.debug("Stopping chromedriver for context " + context); - // add a stop cb for this context so we get called back once this context's - // chromedriver finishes exiting - this.chromedriverStopCbs[context] = _.once(cb); - this.sessionChromedrivers[context].stop(); + // stop listening for the stopped state event + this.sessionChromedrivers[context].removeAllListeners(Chromedriver.EVENT_CHANGED); + this.sessionChromedrivers[context].stop().nodeify(function (err) { + if (err) logger.warn("Error stopping Chromedriver: " + err.message); + // chromedriver isn't valid anymore, so remove it from context list + delete this.sessionChromedrivers[context]; + cb(); + }.bind(this)); }.bind(this), function (err) { // if one of these fails, go back to last proxy state and error out this.restoreProxyState(); diff --git a/lib/devices/android/chrome.js b/lib/devices/android/chrome.js index 98a86e15..ce055181 100644 --- a/lib/devices/android/chrome.js +++ b/lib/devices/android/chrome.js @@ -24,7 +24,7 @@ ChromeAndroid.prototype._androidInit = Android.prototype.init; ChromeAndroid.prototype.init = function () { this._androidInit(); this.adb = null; - this.stopCb = null; + this.onDieCb = null; this.setChromedriverMode(); }; @@ -84,7 +84,7 @@ ChromeAndroid.prototype.start = function (cb, onDie) { this.adb = new ADB(this.args); this.uiautomator = new UiAutomator(this.adb, this.args); this.uiautomator.setExitHandler(this.onUiautomatorExit.bind(this)); - this.stopCb = onDie; + this.onDieCb = onDie; async.series([ this.initAdb.bind(this), @@ -124,7 +124,6 @@ ChromeAndroid.prototype.pushAndUnlock = function (cb) { }; ChromeAndroid.prototype.createSession = function (cb) { - cb = _.once(cb); var caps = { chromeOptions: { androidPackage: this.args.appPackage @@ -143,24 +142,27 @@ ChromeAndroid.prototype.createSession = function (cb) { } caps = this.decorateChromeOptions(caps); - this.chromedriver.once(Chromedriver.EVENT_ERROR, cb); this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) { - if (msg.state === Chromedriver.STATE_ONLINE) { - cb(); - } else if (msg.state === Chromedriver.STATE_STOPPED) { - this.onChromedriverStop(); + if (msg.state === Chromedriver.STATE_STOPPED) { + logger.info("Chromedriver stopped unexpectedly on us, shutting down " + + "then calling back up with the on-die callback"); + this.onChromedriverStop(this.onDieCb); } }.bind(this)); - this.chromedriver.start(caps); + this.chromedriver.start(caps).nodeify(cb); }; ChromeAndroid.prototype.stop = function (cb) { - this.stopCb = cb; // change stopCb from original onDie to this cb - this.chromedriver.stop(); + // stop listening for the stopped state event + this.chromedriver.removeAllListeners(Chromedriver.EVENT_CHANGED); + // now we can handle the stop on our own + this.chromedriver.stop().nodeify(function (err) { + if (err) logger.warn("Error stopping Chromedriver: " + err.message); + this.onChromedriverStop(cb); + }.bind(this)); }; -ChromeAndroid.prototype.onChromedriverStop = function () { - var cb = this.stopCb; +ChromeAndroid.prototype.onChromedriverStop = function (cb) { if (this.adb) { this.uiautomator.shutdown(function () { this.adb.forceStop(this.args.appPackage, function (err) { diff --git a/lib/server/proxy.js b/lib/server/proxy.js index 2a750b40..234d4fb4 100644 --- a/lib/server/proxy.js +++ b/lib/server/proxy.js @@ -42,10 +42,16 @@ module.exports.doProxy = function (req, res) { if (req.device.proxyReqRes) { // this section is triggered when we have defined the proxy device to use // the appium-jsonwp-proxy method proxyReqRes. Ultimately we'll be moving - // everything to this paradigm and will delete the code after this block - req.device.proxyReqRes(req, res).catch(function (err) { + // everything to this paradigm and will delete the code after this block. + // proxyReqRes might be a promise or a callback-based function, so handle + // both + var handler = function (err) { logger.error(err.message); - }); + }; + var p = req.device.proxyReqRes(req, res, handler); + if (p.catch) { + p.catch(handler); + } return; } diff --git a/package.json b/package.json index cb657ac9..9c3ac637 100644 --- a/package.json +++ b/package.json @@ -43,7 +43,7 @@ "adm-zip": "~0.4.7", "appium-adb": "=1.7.5", "appium-atoms": "=0.0.5", - "appium-chromedriver": "=0.2.0", + "appium-chromedriver": "=0.2.2", "appium-instruments": "=1.5.4", "appium-support": "=0.1.1", "appium-uiauto": "=1.10.7",