Clean up Assignment Create in BlimpClientContextImpl.

blimp/client/core/context/assignment_fetcher.h

+// 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.
+
+#ifndef BLIMP_CLIENT_CORE_CONTEXT_ASSIGNMENT_FETCHER_H_
+#define BLIMP_CLIENT_CORE_CONTEXT_ASSIGNMENT_FETCHER_H_
+
+#include <memory>
+#include <string>
+
+#include "blimp/client/core/session/assignment_source.h"
+#include "blimp/client/core/session/identity_source.h"
+
+namespace blimp {
+namespace client {
+
+class AssignmentFetcher {
+ public:
+  AssignmentFetcher(
+      std::unique_ptr<IdentityProvider> identity_provider,
+      const base::Callback<void(const GoogleServiceAuthError&)> error_callback,

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

using AuthErrorCallback = base::Callback<void(const GoogleServiceAuthError&)>;

using AssignmentResultCallback = base::Callback<void(AssignmentRequestResult,
const Assignment&)>;

[CJ, 2016/10/20 20:52:57]

Done.

+      GURL assigner_url,
+      scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner);
+
+  ~AssignmentFetcher();
+
+  void Fetch(base::Callback<void(AssignmentRequestResult, const Assignment&)>

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

Hmm what do you think?  Now that I'm looking at this, does it make sense to pull
the error callback into fetch or this callback in to the constructor?  It looks
like we never clear the callback so it might make sense just to set it once and
keep it forever.

Sorry for the back and forth.  Does one seem cleaner to you?

[CJ, 2016/10/20 20:52:57]

Sounds good to me. It doesn't even make testing easier, so no need to pass it in
this way.

+                 assignment_receieved_callback);

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

receieved -> received

[CJ, 2016/10/20 20:52:57]

Done.

+
+  IdentitySource* GetIdentitySource();

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

Do we still have to expose this?

[CJ, 2016/10/20 20:52:57]

I believe so. BlimpClientContextImpl is expected to be able to provide an
IdentitySource due to its fulfillment of BlimpSettingsDelegate. Unless this has
changed, or if we want to store the IdentitySource on BllimpClientContextImpl
somehow, this has to stay.

+
+ private:
+  // Called when an OAuth2 token is received.  Will then ask the
+  // AssignmentSource for an Assignment with this token.
+  void OnAuthTokenReceived(const std::string& client_auth_token);
+
+  // Provide OAuth2 token and propagate account sign in states change.
+  std::unique_ptr<IdentitySource> identity_source_;

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

Yay!

[CJ, 2016/10/20 20:52:58]

Acknowledged.

+
+  GURL assigner_url_;

[David Trainor- moved to gerrit, 2016/10/20 16:50:32]

The comments still seemed valid :).

[CJ, 2016/10/20 20:52:58]

Done.

+
+  base::Callback<void(AssignmentRequestResult, const Assignment&)>
+      assignment_received_callback_;
+
+  // The task runner to use for IO operations.
+  scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner_;
+
+  // The task runner to use for file operations.
+  scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner_;
+
+  // The AssignmentSource is used when the user of AssignmentFetcher calls
+  // Fetch() to get a valid assignment and later connect to the engine.
+  std::unique_ptr<AssignmentSource> assignment_source_;
+
+  DISALLOW_COPY_AND_ASSIGN(AssignmentFetcher);
+};
+
+}  // namespace client
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_CORE_CONTEXT_ASSIGNMENT_FETCHER_H_