Blimp: add support for SSL connections.

blimp/client/session/assignment_source.h

Index: blimp/client/session/assignment_source.h
diff --git a/blimp/client/session/assignment_source.h b/blimp/client/session/assignment_source.h
index dabe72cbaf7b4f1583db648a293294d043b232d9..a278609f98ae3381a3de112819f190eb6c869a45 100644
--- a/blimp/client/session/assignment_source.h
+++ b/blimp/client/session/assignment_source.h
@@ -8,17 +8,21 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "blimp/client/blimp_client_export.h"
 #include "net/base/ip_endpoint.h"
 #include "net/url_request/url_fetcher_delegate.h"
 
 namespace base {
+class FilePath;
 class SingleThreadTaskRunner;
+class Value;
 }
 
 namespace net {
 class URLFetcher;
 class URLRequestContextGetter;
+class X509Certificate;
 }
 
 namespace blimp {
@@ -44,15 +48,22 @@ struct BLIMP_CLIENT_EXPORT Assignment {
   Assignment();
   ~Assignment();
 
+  // Returns true if the net::IPEndPoint has an unspecified IP, port, or
+  // transport protocol.
+  bool is_null() const;

[Ryan Sleevi, 2016/02/25 22:16:25]

DESIGN NIT: I would have suggested this be called "IsValid()" (with the inverted
logic); null on a concrete object just feels like it 'reads' weird.

[Wez, 2016/02/26 18:32:09]

nit: is_valid() seems a more appropriate name for this - having one of those
fields unspecified does not seem conceptually null-like.

[Kevin M, 2016/02/26 19:57:23]

Done.

+
+  // The transport protocol used to contact the engine.

[Wez, 2016/02/26 18:32:09]

nit: This is a somewhat tautologous and ambiguous comment - at a first glance I
can't tell whether you mean it identifies the _type_ of transport protocol
(which seems to be the case) or it is the actual implementation of a transport
protocol.

Suggest: Specifies the transport to use to connect to the engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

   TransportProtocol transport_protocol;
+
+  // The IP address of the engine.

[Wez, 2016/02/26 18:32:09]

nit: This comment also seems tautologous, especially if you rename ip_endpoint
to something more descriptive, e.g. engine_endpoint

Suggest: Specifies the IP address & port on which to reach the engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

   net::IPEndPoint ip_endpoint;
+
+  // The token used to authenticate the client to the engine.

[Wez, 2016/02/26 18:32:09]

Suggest simply: Used to authenticate to the specified engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

   std::string client_token;

[Wez, 2016/02/26 18:32:09]

Also suggest renaming this to auth_token.

[Kevin M, 2016/02/26 19:57:23]

That would make this inconsistent with the Assigner API's nomenclature.

[Wez, 2016/03/01 00:23:55]

Yes, but we're already inconsistent with the names of the other fields
(engine_endpoint vs host_and_port, cert vs certificate), so I'm not sure that's
a concern.

[Kevin M, 2016/03/01 18:23:16]

It's not just the Assigner response; "client_token" is used all over the
codebase. I'd have to touch a lot of files to incorporate this change and keep
it consistent across all of blimp/'s source files, which would dilute the
reviewing focus of this CL. I'd prefer to do that in its own CL.

Filed bug 591074.

-  std::string certificate;
-  std::string certificate_fingerprint;
 
-  // Returns true if the net::IPEndPoint has an unspecified IP, port, or
-  // transport protocol.
-  bool is_null() const;
+  // The expected certificate of the engine. The peer certificate must match
+  // |cert| exactly, otherwise the connection will be dropped.

[Wez, 2016/02/26 18:32:09]

The suggestions I've given above switch the sense of the comments to describe
how the field is intended to be used, rather than just describing what it
contains (although the two are often pretty similar!) - I'd suggest doing the
same here, e.g. "Specifies the X509 certificate that the engine must report.
..."

[Kevin M, 2016/02/26 19:57:23]

Done.

+  scoped_refptr<net::X509Certificate> cert;
 };
 
 // AssignmentSource provides functionality to find out how a client should
@@ -72,18 +83,15 @@ class BLIMP_CLIENT_EXPORT AssignmentSource : public net::URLFetcherDelegate {
     RESULT_OUT_OF_VMS = 7,
     RESULT_SERVER_ERROR = 8,
     RESULT_SERVER_INTERRUPTED = 9,
-    RESULT_NETWORK_FAILURE = 10
+    RESULT_NETWORK_FAILURE = 10,
+    RESULT_INVALID_CERT = 11,
   };
 
   typedef base::Callback<void(AssignmentSource::Result, const Assignment&)>
       AssignmentCallback;
 
-  // The |main_task_runner| should be the task runner for the UI thread because
-  // this will in some cases be used to trigger user interaction on the UI
-  // thread.
-  AssignmentSource(
-      const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner,
-      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner);
+  explicit AssignmentSource(
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);

[Wez, 2016/02/26 18:32:09]

nit: Document why the |io_task_runner| is needed, e.g. to use for File I/O? Or
URL fetching, etc?

[Kevin M, 2016/02/26 19:57:23]

Done.

   ~AssignmentSource() override;
 
   // Retrieves a valid assignment for the client and posts the result to the
@@ -94,21 +102,29 @@ class BLIMP_CLIENT_EXPORT AssignmentSource : public net::URLFetcherDelegate {
   void GetAssignment(const std::string& client_auth_token,
                      const AssignmentCallback& callback);
 
+  // Called when GetCustomAssignment() has completed.

[Wez, 2016/02/26 18:32:09]

GetCustomAssignment() isn't declared here, so it doesn't make sense to refer to
it in documenting a public API.

Why is this API, which seems to be a completion-handler function really, public?

What is a "custom assignment"?

[Kevin M, 2016/02/26 19:57:23]

Doesn't need to be public. "custom assignment" means "command line parsed
assignment".

Changed the name and visibility.

+  // Uses |custom_assignment| if provided; queries the Assigner for one
+  // otherwise.
+  void OnGetCustomAssignmentDone(const std::string& client_auth_token,
+                                 Assignment custom_assignment);
+
   // net::URLFetcherDelegate implementation:
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
  private:
   void ParseAssignerResponse();
-
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
-
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
-  scoped_ptr<net::URLFetcher> url_fetcher_;
+  void AssignerJSONParseOK(scoped_ptr<base::Value> json);
+  void AssignerJSONParseError(const std::string& error);

[Wez, 2016/02/26 18:32:09]

These names don't scan - do you mean OnJsonParsed() and OnJsonParseError(),
perhaps?

 
   // This callback is set during a call to GetAssignment() and is cleared after
   // the request has completed (whether it be a success or failure).

[Wez, 2016/02/26 18:32:09]

nit: (And I realise this was already here) This comment describes _what_ you're
doing, but not _why_ - the only reason I can see offhand is to so that we can
DCHECK that GetAssignment isn't called twice?

[Kevin M, 2016/02/26 19:57:23]

Mostly because URLFetcher takes a delegate object for completion, not a
base::Callback, so this is how we stash the callback for later invocation.

Added the "why".

   AssignmentCallback callback_;
 
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+  base::WeakPtrFactory<AssignmentSource> weak_factory_;

[Wez, 2016/02/26 18:32:09]

nit: Suggest separating the weak_factory_ from the rest by a blank-line since it
is Special. :P

[Kevin M, 2016/02/26 19:57:23]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(AssignmentSource);
 };