RPC Explorer

server/static/rpcexplorer/rpc-call.html

Index: server/static/rpcexplorer/rpc-call.html
diff --git a/server/static/rpcexplorer/rpc-call.html b/server/static/rpcexplorer/rpc-call.html
new file mode 100644
index 0000000000000000000000000000000000000000..4cece5f580753ee63262cef3649afe3636b5e1e4
--- /dev/null
+++ b/server/static/rpcexplorer/rpc-call.html
@@ -0,0 +1,278 @@
+<!--
+  ~ // Copyright 2016 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.
+  ~
+  -->
+
+<link rel="import" href="/static/common/third_party/iron-ajax/iron-ajax.html">
+<link rel="import" href="/static/common/third_party/polymer/polymer.html">
+
+<link rel="import" href="rpc-codes.html">
+<link rel="import" href="rpc-error.html">
+
+<!-- The `rpc-call` element can send a pRPC request.
+pRPC protocol: https://godoc.org/github.com/luci/luci-go/common/prpc#hdr-Protocol.
+-->
+<dom-module id="rpc-call">
+  <template>
+    <iron-ajax
+        id="ajax"
+        auto="[[auto]]"
+        method="POST" url="[[_url]]"

[Bons, 2016/02/13 17:18:23]

place each attribute on it's own line. makes it easier to parse.

[nodir, 2016/02/17 02:02:12]

Done.

+        body="[[request]]" content-type="application/json"
+        debounce-duration="[[debounceDuration]]"
+        timeout="[[timeout]]"
+        verbose="[[verbose]]"
+        handle-as="json" json-prefix=")]}'"

[Bons, 2016/02/13 17:18:25]

json is already the default for iron-ajax:
https://elements.polymer-project.org/elements/iron-ajax#property-handleAs

[nodir, 2016/02/17 02:02:12]

Done.

+        on-request="_onRequest"
+        on-loading-changed="_loadingChanged"
+        on-response="_onResponse"
+        on-error="_onError"
+    ></iron-ajax>
+  </template>
+  <script>
+    'use strict';
+
+    Polymer({
+      is: 'rpc-call',

[Bons, 2016/02/13 17:18:22]

blank lines between top-level properties.

Polymer({
  is: 'rpc-call',

  properties: {
...

[nodir, 2016/02/17 02:02:12]

Done.

+      properties: {
+        /**
+         * if true, use HTTP instead of HTTPS.
+         * If null (default) or undefined, determined automatically:
+         * - if host equals current host and current protocol is http, then
+         *   false

[Bons, 2016/02/13 17:18:24]

this will fit on the previous line. also full sentences with period.

[nodir, 2016/02/17 02:02:12]

does not fit with "then"

[Bons, 2016/02/17 19:27:47]

Still needs to be a full sentence.

+         * - otherwise true.
+         */
+        insecure: {

[Bons, 2016/02/13 17:18:25]

the property already has a default value of undefined which == null. you can
declare this as 

insecure: Boolean,

with the same effect.

[nodir, 2016/02/17 02:02:12]

weirdly enough, if I replace it with `insecure: Boolean`, _generateUrl is not
longer called

[Bons, 2016/02/17 19:27:47]

That’s because it is then `undefined` and observers/compute functions wait until
all parameters are !== undefined before firing, which is why `_generateUrl`
isn’t firing.

+          type: Boolean,
+          value: null
+        },
+
+        /**
+         * pRPC server host, defaults to current document host.
+         */
+        host: {
+          type: String,
+          value: document.location.host,
+        },
+
+        /**
+         * Full service name, including package name.
+         */
+        service: String,
+
+        /**
+         * Service method name.
+         */
+        method: String,
+
+        _url: {

[Bons, 2016/02/13 17:18:25]

place private properties below public ones to maintain logical grouping.

[nodir, 2016/02/17 02:02:12]

Done.

+          type: String,
+          computed: '_generateUrl(insecure, host, service, method)'
+        },
+
+        /**
+         * Request object.
+         */
+        request: {
+          type: Object,
+          value: function() {
+            return {};
+          }
+        },
+
+        /**
+         * Request timeout in milliseconds.
+         */
+        timeout: {
+          type: Number,
+          value: 0,
+          observer: '_onTimeoutChanged'
+        },
+
+        /**
+         * If true, automatically sends a request when host, service, method
+         * or request changes.
+         */
+        auto: {
+          type: Boolean,
+          value: false
+        },
+
+        /**
+         * If true, error messages will automatically be logged to the console.
+         */
+        verbose: {
+          type: Boolean,
+          value: false
+        },
+
+        /**
+         * The most recent request made by this element.
+         */
+        lastRequest: {
+          type: Object,
+          notify: true,
+          readOnly: true
+        },
+
+        /**
+         * True while lastRequest is in flight.
+         */
+        loading: {
+          type: Boolean,
+          notify: true,
+          readOnly: true
+        },
+
+        /**
+         * lastRequest's response.
+         *
+         * Note that lastResponse and lastError are set when
+         * lastRequest finishes, so if loading is true, lastResponse and lastError

[nodir, 2016/02/17 02:02:12]

Done.

+         * will correspond to the result of the previous request.
+         */
+        lastResponse: {
+          type: Object,
+          notify: true,
+          readOnly: true
+        },
+
+        /**
+         * lastRequest's error, if any.
+         * May be an instance of rpcExplorer.GrpcError which has response code
+         * and description.
+         */
+        lastError: {
+          type: Object,
+          notify: true,
+          readOnly: true
+        },
+
+        /**
+         * Length of time in milliseconds to debounce multiple automatically
+         * generated requests.
+         */
+        debounceDuration: {
+          type: Number,
+          value: 0,
+          notify: true
+        }
+      },
+
+      _loadingChanged: function() {
+        this._setLoading(this.$.ajax.loading);

[Bons, 2016/02/13 17:18:23]

why not bind to iron-ajax's loading property?
https://elements.polymer-project.org/elements/iron-ajax#property-loading

[nodir, 2016/02/17 02:02:12]

two-way data-binding does not work with read-only properties

[Bons, 2016/02/17 19:27:47]

in that case, instead of querying for the element, use the value passed to the
function.

+      },
+
+      _onTimeoutChanged: function(val) {
+        var headers = this.$.ajax.requestHeaders;
+        if (val) {
+          headers['X-Prpc-Timeout'] = val + 'm';
+        } else {
+          delete headers['X-Prpc-Timeout']
+        }
+      },
+
+      _generateUrl: function(insecure, host, service, method) {
+        if (!host || !service || !method) {
+          return '';
+        }
+
+        var protocol;
+        switch (insecure) {

[Bons, 2016/02/13 17:18:23]

i think this block would read better as a set of if statements. you then
wouldn't have to be explicit about accounting for undefined AND null

[nodir, 2016/02/17 02:02:12]

simplified a bit, but I am not sure how to be less explicit about
undefined/null.
Also not sure why "not explicit" is considered good.
I guess I wrote too much go

+          case true:
+            protocol = 'http:';
+            break;
+          case undefined:
+          case null:
+            if (host == document.location.host) {
+              protocol = document.location.protocol;
+              break;
+            }
+          default:
+            protocol = 'https:';
+        }
+
+        return protocol + '//' + host + '/prpc/' + service + '/' + method;
+      },
+
+      send: function() {
+        if (!this.$.ajax.url) {
+          throw new Error('rpc-call is in invalid state');

[Bons, 2016/02/13 17:18:25]

"new" isn't required.

throw Error(...)

[nodir, 2016/02/17 02:02:12]

Done.

+        }
+        return this.$.ajax.generateRequest();
+      },
+
+      _onRequest: function() {
+        this._setLastRequest(this.request);
+        var e = {
+          host: this.host,
+          service: this.service,
+          method: this.method,
+          request: this.request
+        };
+        this.fire('request', e, {bubbles: false});
+      },
+      _onError: function(e) {

[Bons, 2016/02/13 17:18:25]

put a blank line between top-level function declarations

[nodir, 2016/02/17 02:02:12]

Done.

+        this._done(e.detail.request.xhr);
+      },
+      _onResponse: function(e) {
+        this._done(e.detail.xhr);
+      },
+      _done: function(xhr) {
+        var ajax = this.$.ajax;
+
+        function parseResponse() {
+          if (ajax.lastError && typeof xhr.status !== 'number') {

[Bons, 2016/02/13 17:18:25]

you mix strict equality in with non strict. I would pick one and be consistent.

[nodir, 2016/02/17 02:02:12]

I've updated code to use only ===, except for cases when I want to check for
both null and undefined (x==null)

+            // Response was not received.
+            return {
+              error: ajax.lastError
+            };
+          }
+
+          var codeHeader = xhr.getResponseHeader('X-Prpc-Grpc-Code');
+          if (!codeHeader) {
+            return {
+              error: new Error(
+                'Invalid response: no X-Prpc-Grpc-Code response header')
+            };
+          }
+
+          var code;
+          try {
+            code = parseInt(codeHeader, 10);
+            if (code == null) {
+              throw new Error('code is not defined');

[Bons, 2016/02/13 17:18:25]

throw Error

[nodir, 2016/02/17 02:02:12]

Done.

+            }
+          } catch (e) {
+            return {
+              error: new Error(
+                'Invalid X-Prpc-Grpc-Code response header "' + codeHeader +
+                '": ' + e
+              )
+            };
+          }
+
+          if (code !== rpcExplorer.Codes.OK) {
+            return {
+              error: new rpcExplorer.GrpcError(code, xhr.responseText)
+            };
+          }
+          return {
+            response: ajax.lastResponse
+          };
+        }
+
+        var result = parseResponse();
+        result.response = result.response || null;
+        result.error = result.error || null;
+
+        this._setLastResponse(result.response);
+        this._setLastError(result.error);
+
+        this.fire('response', result, {bubbles: false});
+      }
+    });
+  </script>
+</dom-module>