Add AssignmentSource to BlimpClientContextImpl

Add AssignmentSource to BlimpClientContextImpl

This CL adds support for using the AssignmentSource for getting
assignments for the blimp engine.

Compared to the previous implementation in BlimpClientSession, this CL
constructs the AssignmentSource lazily, for two reasons:
- Right now, we can then easily use a Java-backed preference store for
  which assigner to use.
- Later, we can easily make the change of an assigner URL just reset
  the AssignmentSource, and another call to Connect() would then
  automatically create a new one.

BUG=611097
Committed: https://crrev.com/ae227aa6323de6cef1d9289208d1a7a94aa4d83f
Cr-Commit-Position: refs/heads/master@{#410957}

[nyquist, 2016-08-03 21:49:33]

nyquist@chromium.org changed reviewers:
+ dtrainor@chromium.org, kmarshall@chromium.org

[nyquist, 2016-08-03 21:49:34]

dtrainor: PTAL

kmarshall: PTAL when you have time as well.

[David Trainor- moved to gerrit, 2016-08-04 16:15:52]

lgtm % nits.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:78: std::string
url_string = ConvertJavaStringToUTF8(env, jurl);
Just inline in GURL(...)?

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:123: void
BlimpClientContextImpl::OnAssignmentConnectionAttempted(
Should this actually hit the native delegate?

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
File blimp/client/core/dummy_blimp_client_context.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.cc:43: void
DummyBlimpClientContext::Connect(const std::string& client_auth_token) {}
NOTREACHED() maybe?

[Kevin M, 2016-08-04 17:44:17]

Looks pretty good, I just have some signal-to-noise ratio nits about the
comments.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:115: VLOG(1) << "Assignment
succeeded";
How about VLOGging all result values at the top?

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.h:60: // Notified every time the
AssignmentSource returns the result of an attempted
Notified => Called

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:38: // The thread to use
for IO related tasks.
Not needed

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:42: // Message loop for
the test thread.
Not needed

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
File blimp/client/core/dummy_blimp_client_context.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.cc:17: // This function is declared
in //blimp/client/public/blimp_client_context.h,
This comment doesn't really add any more context than is already communicated by
the code itself.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
File blimp/client/core/dummy_blimp_client_context.h (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.h:15: // A dummy implementation of
the BlimpClientContext.
Already understood by the class name. ;)

What's it used for?

nit: If this is basically a null object, I recommend calling it a
NullBlimpClientContext.

[nyquist, 2016-08-05 21:22:39]

All done.

kmarshall: PTAL
dtrainor: The changes were a little bit bigger than a simple thing, so feel free
to take another look as well to see if I solved it the way you expected.

Most comments were addressed in patch set 2.
Moving the Assignment and the call to the delegate is in patch set 3. This was
because this was a much bigger change than the rest.
Then I merged in origin/master in patch set 4 to ensure there'll be no merge
conflicts.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:78: std::string
url_string = ConvertJavaStringToUTF8(env, jurl);
On 2016/08/04 16:15:51, David Trainor wrote:
> Just inline in GURL(...)?

Done.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:115: VLOG(1) << "Assignment
succeeded";
On 2016/08/04 17:44:17, Kevin M wrote:
> How about VLOGging all result values at the top?

Done.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.h:60: // Notified every time the
AssignmentSource returns the result of an attempted
On 2016/08/04 17:44:17, Kevin M wrote:
> Notified => Called

Done.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:38: // The thread to use
for IO related tasks.
On 2016/08/04 17:44:17, Kevin M wrote:
> Not needed

Done.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl_unittest.cc:42: // Message loop for
the test thread.
On 2016/08/04 17:44:17, Kevin M wrote:
> Not needed

Done.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
File blimp/client/core/dummy_blimp_client_context.cc (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.cc:17: // This function is declared
in //blimp/client/public/blimp_client_context.h,
On 2016/08/04 17:44:17, Kevin M wrote:
> This comment doesn't really add any more context than is already communicated
by
> the code itself.

It does mention the other place the method is implemented. Now, with the updated
comment in the header file, would you be OK with keeping it? Or would you want
me to add more context here as well, or just remove it?

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.cc:43: void
DummyBlimpClientContext::Connect(const std::string& client_auth_token) {}
On 2016/08/04 16:15:51, David Trainor wrote:
> NOTREACHED() maybe?

Done. I wasn't sure whether we wanted to guard the call to Connect() in the
embedder itself, or just make it a no-op.

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
File blimp/client/core/dummy_blimp_client_context.h (right):

https://codereview.chromium.org/2211613002/diff/1/blimp/client/core/dummy_bli...
blimp/client/core/dummy_blimp_client_context.h:15: // A dummy implementation of
the BlimpClientContext.
On 2016/08/04 17:44:17, Kevin M wrote:
> Already understood by the class name. ;)
> 
> What's it used for?
> 
> nit: If this is basically a null object, I recommend calling it a
> NullBlimpClientContext.

Tried to clarify how to get to this class and why it is sometimes in use in the
comment.

I did not do the rename for now though. I'd be OK with doing that in a follow-up
CL if you want me to.

[Kevin M, 2016-08-09 21:00:34]

lgtm % nits

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:62: BlimpClientContextDelegate*
delegate_;
= nullptr;

and remove initializer from ctor list

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/sessi...
File blimp/client/core/session/assignment_source.cc (right):

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/sessi...
blimp/client/core/session/assignment_source.cc:302: if
(!json->GetAsDictionary(&dict)) {
nit: Add LOG(WARNING)s here and wherever BAD_RESPONSE is used in this function -
it's an overloaded error code

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
File blimp/client/public/session/assignment.h (right):

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
blimp/client/public/session/assignment.h:50: // Returns true if the
net::IPEndPoint has an unspecified IP, port, or
Returns false?

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
blimp/client/public/session/assignment.h:63: // Specifies the X.509 certificate
that the engine must report.
"must report" is a little misleading - it's more than just a matter of
"reporting" a value; it's also used for key exchange?

"Specifies the engine's X.509 certificate." ?

[nyquist, 2016-08-10 01:19:52]

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:62: BlimpClientContextDelegate*
delegate_;
On 2016/08/09 21:00:34, Kevin M wrote:
> = nullptr;
> 
> and remove initializer from ctor list

Done.

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
File blimp/client/public/session/assignment.h (right):

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
blimp/client/public/session/assignment.h:50: // Returns true if the
net::IPEndPoint has an unspecified IP, port, or
On 2016/08/09 21:00:34, Kevin M wrote:
> Returns false?

Ouch! Done.

https://codereview.chromium.org/2211613002/diff/60001/blimp/client/public/ses...
blimp/client/public/session/assignment.h:63: // Specifies the X.509 certificate
that the engine must report.
On 2016/08/09 21:00:34, Kevin M wrote:
> "must report" is a little misleading - it's more than just a matter of
> "reporting" a value; it's also used for key exchange?
> 
> "Specifies the engine's X.509 certificate." ?

Agreed! Done.

[nyquist, 2016-08-10 01:20:03]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-08-10 01:20:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2211613002/#ps100001
(title: "merged origin/master for good measure")

[commit-bot: I haz the power, 2016-08-10 01:20:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 01:24:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 01:24:44]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nyquist, 2016-08-10 03:49:42]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-08-10 03:49:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2211613002/#ps120001
(title: "merge origin/master")

[commit-bot: I haz the power, 2016-08-10 03:50:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 04:46:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-10 04:48:38]

Description was changed from

==========
Add AssignmentSource to BlimpClientContextImpl

This CL adds support for using the AssignmentSource for getting
assignments for the blimp engine.

Compared to the previous implementation in BlimpClientSession, this CL
constructs the AssignmentSource lazily, for two reasons:
- Right now, we can then easily use a Java-backed preference store for
  which assigner to use.
- Later, we can easily make the change of an assigner URL just reset
  the AssignmentSource, and another call to Connect() would then
  automatically create a new one.

BUG=611097
==========

to

==========
Add AssignmentSource to BlimpClientContextImpl

This CL adds support for using the AssignmentSource for getting
assignments for the blimp engine.

Compared to the previous implementation in BlimpClientSession, this CL
constructs the AssignmentSource lazily, for two reasons:
- Right now, we can then easily use a Java-backed preference store for
  which assigner to use.
- Later, we can easily make the change of an assigner URL just reset
  the AssignmentSource, and another call to Connect() would then
  automatically create a new one.

BUG=611097

Committed: https://crrev.com/ae227aa6323de6cef1d9289208d1a7a94aa4d83f
Cr-Commit-Position: refs/heads/master@{#410957}
==========

[commit-bot: I haz the power, 2016-08-10 04:48:40]

Patchset 7 (id:??) landed as
https://crrev.com/ae227aa6323de6cef1d9289208d1a7a94aa4d83f
Cr-Commit-Position: refs/heads/master@{#410957}