Add some closureised JS bindings for DevTools for use by headless embedder

headless/lib/browser/devtools_api/devtools_connection.js

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview Contains a class which marshals DevTools protocol messages over
+ * a provided low level message transport. This transport might be a headless
+ * TabSocket, or a WebSocket or a mock for testing.
+ */
+
+'use strict';
+
+goog.provide('chromium.DevTools.Connection');
+
+/**
+ * @typedef {!function(Object): undefined|!function(string): undefined}

[dpapad, 2017/06/06 17:37:46]

"!" not necessary for |function| which is considered by default non-nullable.
See
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the...,
specifically the part that says

"All object types are nullable by default whether or not they are declared with
the Nullable operator. An object type is defined as anything except a function,
string, number, or boolean."

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+ *
+ */
+chromium.DevTools.CallbackFunction;
+
+/**
+ * @class Handles sending and receiving DevTools JSON protocol messages over the
+ *   provided low level message transport.
+ * @param {!Object} transport The API providing transport for devtools commands.
+ * @constructor
+ */
+chromium.DevTools.Connection = function(transport) {
+  /**
+   * The DevTools connection.
+   *
+   * @private {!Object}
+   */
+  this.transport_ = transport;
+
+  /**

[dpapad, 2017/06/06 17:37:45]

Nit: Consider compacting those as where possible, as follows:

/** @private {number} */

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+   * @private {number}
+   */
+  this.commandId_ = 1;
+
+  /**
+   * An object containing pending DevTools protocol commands keyed by id.
+   *
+   * @type {!Object<!chromium.DevTools.CallbackFunction>}

[dpapad, 2017/06/06 17:37:46]

Why not using the shorthand syntax? Also why not using a Map instead of a plain
Object?

@private {!Object<!chromium.DevTools.CallbackFunction>}

+   * @private
+   */
+  this.pendingCommands_ = {};
+
+  /**
+   * An object containing DevTools protocol events we are listening for keyed by
+   * name.
+   *
+   * @type {!Object<!Array<!chromium.DevTools.CallbackFunction>>}

[dpapad, 2017/06/06 17:37:46]

Same question about Object vs Map

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+   * @private
+   */
+  this.eventListeners_ = {};
+
+  this.transport_.onmessage = this.onMessage_.bind(this);
+};
+
+
+/**
+ * Listens for DevTools protocol events of the specified name and issues the
+ * callback upon reception.
+ *
+ * @param {!string} eventName Name of the DevTools protocol event to listen for.

[dpapad, 2017/06/06 17:37:46]

"!" not necessary here (and elsewhere).

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+ * @param {!chromium.DevTools.CallbackFunction} listener The callback issued
+ *   when we receive a DevTools protocol event corresponding to the given name.
+ */
+chromium.DevTools.Connection.prototype.addEventListener = function(
+    eventName, listener) {
+  if (!this.eventListeners_.hasOwnProperty(eventName)) {
+    this.eventListeners_[eventName] = [];
+  }
+  this.eventListeners_[eventName].push(listener);
+};
+
+/**
+ * Removes an event listener previously added by <code>addEventListener</code>.
+ *
+ * @param {!string} eventName Name of the DevTools protocol event to remove.
+ * @param {!chromium.DevTools.CallbackFunction} listener The callback to remove.
+ */
+chromium.DevTools.Connection.prototype.removeEventListener = function(

[dpapad, 2017/06/06 17:37:46]

This is mimicking the addEventListener/removeEventListener API of HTMLElement,
which is a known shortcoming of the Web. Explaining below:

The misuse of this API is unfortunately far too common.

// Add an event listener.
foo.addEventListener('some-event', this.someFunction.bind(this));
// Erroneously fail to remove an event listener, because a different function
reference is passed.
foo.removeEventListener('some-event', this.someFunction.bind(this));

Where it should have been as follows:
var boundFunction = this.someFunction.bind(this);
foo.addEventListener('some-event', boundFunction);
foo.removeEventListener('some-event', boundFunction);


So if there is no reason to replicate this API, here is a far better alternative

var listenerId = foo.addEventListener('some-event',
this.someFunction.bind(this));
foo.removeEventListener(listenerId);

Where removeEventListener returns a boolean, with true if the listener was
indeed removed. This API is harder to misuse, and notice how the
removeEventListener() method does not even need the name of the event anymore.
Keeping a Map internally to associate listenerIds (just a number) with listeners
is also trivial. You can also avoid a lot of the linear searching that happens
in this method via the usage of indexOf.

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+    eventName, listener) {
+  if (!this.eventListeners_.hasOwnProperty(eventName)) return;
+  let index = this.eventListeners_[eventName].indexOf(listener);
+  if (index === -1) return;
+  this.eventListeners_[eventName].splice(index, 1);
+};
+
+
+/**
+ * Issues a DevTools protocol command and returns a promise for the results.
+ *
+ * @param {string} method The name of the DevTools protocol command method.
+ * @param {!Object=} opt_params An object containing the command parameters if
+ *    any.
+ * @return {Promise<!Object>} A promise for the results object.

[dpapad, 2017/06/06 17:37:46]

!Promise

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+ */
+chromium.DevTools.Connection.prototype.sendDevToolsMessage = function(
+    method, opt_params) {
+  if (opt_params === undefined) {
+    opt_params = {};
+  }
+  let id = this.commandId_;
+  // We increment by two because these bindings are intended to be used in
+  // conjunction with HeadlessDevToolsClient::RawProtocolListener and using odd
+  // numbers for js generated IDs lets the implementation of OnProtocolMessage
+  // easily distinguish between C++ and JS generated commands and route the
+  // response accordingly.
+  this.commandId_ += 2;
+  this.transport_.send(
+      JSON.stringify({'method': method, 'id': id, 'params': opt_params}));

[dpapad, 2017/06/06 17:37:46]

I suppose that the single quotes here are used because otherwise JS compiler
mangles the object names. If that is the case please mention that in the
comment, otherwise a future developer might remove them and possibly break
things.

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+  return new Promise((resolve, reject) => {
+    this.pendingCommands_[id] = resolve;
+  });
+};
+
+
+/**
+ * @param {string} json_message An object containing a DevTools protocol

[dpapad, 2017/06/06 17:37:45]

Per JS Styleguide:
s/json_message/jsonMessage

[alex clarke (OOO till 29th), 2017/06/07 15:15:39]

Done.

+ *    message.
+ * @private
+ */
+chromium.DevTools.Connection.prototype.onMessage_ = function(json_message) {
+  const message = JSON.parse(json_message);
+  if (message.hasOwnProperty('id')) {
+    if (!this.pendingCommands_.hasOwnProperty(message.id))
+      throw new Error('Unrecognized id:' + json_message);
+    if (message.hasOwnProperty('error'))
+      throw new Error('DevTools protocol error: ' + message.error);
+    this.pendingCommands_[message.id](message.result);
+    delete this.pendingCommands_[message.id];
+  } else {
+    if (!message.hasOwnProperty('method') ||
+        !message.hasOwnProperty('params')) {
+      throw new Error('Bad message:' + json_message);
+    }
+    const method = message['method'];
+    const params = message['params'];
+    if (this.eventListeners_.hasOwnProperty(method)) {
+      const listeners = this.eventListeners_[method];
+      for (let i = 0; i < listeners.length; i++) {
+        listeners[i](params);
+      }
+    }
+  }
+};