Protocol-level changes required to support PIN-less authentication.

Protocol-level changes required to support PIN-less authentication.

This CL adds the protocol messages needed to support PIN-less authentication,
and the C++ glue to route them as far as the HostStub and get the reply back
to a client callback.

BUG=156182
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199371

[Jamie, 2013-05-09 01:37:41]

ptal

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_stub.h...
remoting/protocol/host_stub.h:45: // implementation CLs have landed.
I can make this pure virtual, but it means this CL touches more files than it
needs to. LMK if you'd prefer that.

[alexeypa (please no reviews), 2013-05-09 02:49:20]

Just a drive-by.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
File remoting/protocol/client_control_dispatcher.cc (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
remoting/protocol/client_control_dispatcher.cc:93: if
(!latest_pairing_callback_.is_null()) {
nit: LOG(WARNING) << some message if no callback available.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
remoting/protocol/client_control_dispatcher.cc:95:
latest_pairing_callback_.Reset();
I'd say this logic belongs to |client_stub|. Up until now
ClientControlDispatcher was simply passing messages to the corresponding stub.
This CL makes it more complicated.

[rmsousa, 2013-05-09 03:55:02]

https://codereview.chromium.org/14715012/diff/1/remoting/proto/control.proto
File remoting/proto/control.proto (right):

https://codereview.chromium.org/14715012/diff/1/remoting/proto/control.proto#...
remoting/proto/control.proto:62: message PairingReply {
nit: We typically use Request/Response for proto rpc messages. Even though we're
not using Protobuf's rpc facility, these are following similar semantics, so I
think we should keep the naming scheme.
https://developers.google.com/protocol-buffers/docs/style

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
File remoting/protocol/client_control_dispatcher.cc (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
remoting/protocol/client_control_dispatcher.cc:95:
latest_pairing_callback_.Reset();
On 2013/05/09 02:49:20, alexeypa wrote:
> I'd say this logic belongs to |client_stub|. Up until now
> ClientControlDispatcher was simply passing messages to the corresponding stub.
> This CL makes it more complicated. 

Agreed. ClientStub is already essentially a callback interface. No point using a
different callback mechanism just for this message.

Ideally it should look like SetCursorShape or InjectClipboardEvent - go all the
way up to ClientStub (or PairingStub), ChromotingClient, ChromotingInstance,
Webapp. (yes, those are quite a few classes to pipe through).

There is no way for the webapp to pass in a bound callback on the RequestPairing
call, so it already has to be able to receive a "stateless" RespondToPairing
message from the client plugin and route it properly. There's no need to
duplicate such routing (and deal with cases such as orphan replies, double
requests, etc, etc) here as well.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
File remoting/protocol/host_control_dispatcher.cc (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
remoting/protocol/host_control_dispatcher.cc:76:
base::Bind(&HostControlDispatcher::OnPairingReply,
If you don't use a bound callback you can remove the weakfactory as well.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
remoting/protocol/host_control_dispatcher.cc:83: void
HostControlDispatcher::OnPairingReply(const PairingReply& pairing_reply) {
nit: Make this a verb (RespondToPairing?), move it up, with the other
marshallers.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_stub.h...
remoting/protocol/host_stub.h:45: // implementation CLs have landed.
On 2013/05/09 01:37:41, Jamie wrote:
> I can make this pure virtual, but it means this CL touches more files than it
> needs to. LMK if you'd prefer that.

Leaving it blank is fine for now. Same for the equivalent on ClientStub.

[Jamie, 2013-05-09 17:59:51]

ptal

https://codereview.chromium.org/14715012/diff/1/remoting/proto/control.proto
File remoting/proto/control.proto (right):

https://codereview.chromium.org/14715012/diff/1/remoting/proto/control.proto#...
remoting/proto/control.proto:62: message PairingReply {
On 2013/05/09 03:55:02, rmsousa wrote:
> nit: We typically use Request/Response for proto rpc messages. Even though
we're
> not using Protobuf's rpc facility, these are following similar semantics, so I
> think we should keep the naming scheme.
> https://developers.google.com/protocol-buffers/docs/style

Done.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
File remoting/protocol/client_control_dispatcher.cc (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
remoting/protocol/client_control_dispatcher.cc:93: if
(!latest_pairing_callback_.is_null()) {
On 2013/05/09 02:49:20, alexeypa wrote:
> nit: LOG(WARNING) << some message if no callback available.

No longer needed in light of your and Renato's comments about this belonging in
ClientStub.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/client_cont...
remoting/protocol/client_control_dispatcher.cc:95:
latest_pairing_callback_.Reset();
On 2013/05/09 03:55:02, rmsousa wrote:
> On 2013/05/09 02:49:20, alexeypa wrote:
> > I'd say this logic belongs to |client_stub|. Up until now
> > ClientControlDispatcher was simply passing messages to the corresponding
stub.
> > This CL makes it more complicated. 
> 
> Agreed. ClientStub is already essentially a callback interface. No point using
a
> different callback mechanism just for this message.
> 
> Ideally it should look like SetCursorShape or InjectClipboardEvent - go all
the
> way up to ClientStub (or PairingStub), ChromotingClient, ChromotingInstance,
> Webapp. (yes, those are quite a few classes to pipe through).
> 
> There is no way for the webapp to pass in a bound callback on the
RequestPairing
> call, so it already has to be able to receive a "stateless" RespondToPairing
> message from the client plugin and route it properly. There's no need to
> duplicate such routing (and deal with cases such as orphan replies, double
> requests, etc, etc) here as well.

Fair enough. It gets rid of a TODO and makes this CL more self-contained as
well. Done.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
File remoting/protocol/host_control_dispatcher.cc (right):

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
remoting/protocol/host_control_dispatcher.cc:76:
base::Bind(&HostControlDispatcher::OnPairingReply,
On 2013/05/09 03:55:02, rmsousa wrote:
> If you don't use a bound callback you can remove the weakfactory as well.

Done.

https://codereview.chromium.org/14715012/diff/1/remoting/protocol/host_contro...
remoting/protocol/host_control_dispatcher.cc:83: void
HostControlDispatcher::OnPairingReply(const PairingReply& pairing_reply) {
On 2013/05/09 03:55:02, rmsousa wrote:
> nit: Make this a verb (RespondToPairing?), move it up, with the other
> marshallers.

Done.

[rmsousa, 2013-05-09 18:12:56]

lgtm

[commit-bot: I haz the power, 2013-05-09 18:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14715012/19001

[commit-bot: I haz the power, 2013-05-09 18:30:13]

Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-09 18:35:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14715012/19001

[commit-bot: I haz the power, 2013-05-09 21:19:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14715012/19001

[commit-bot: I haz the power, 2013-05-10 04:35:10]

Change committed as 199371