[Webapp Refactor] Group authentications fields into remoting.CredentialsProvider.

[Webapp Refactor] Group authentications properties into remoting.CredentialsProvider.

Currently, the connect call of the clientPlugin requires a
fetchPinCallback, a fetchThirdPartyTokenCallback, an accessCode and a pairingInfo.

This fields are passed down all the way from crd_connect.js to the plugin via
sessionConnector and clientSession.  As a result, each of the intermediary layers
has its own member variables to cached those properties

This CL creates a new class remoting.CredentialsProvider that represents connection
credentials.  This also eliminates a set of callbacks and useAsyncPinDialog() on the
client plugin interface, which make the credentials negotiation flow between the
plugin and the client session less chatty.

BUG=462383
Committed: https://crrev.com/a656e72c2ba19305be86a1edf05af5446240bda0
Cr-Commit-Position: refs/heads/master@{#318768}

[kelvinp, 2015-02-26 22:18:59]

Patchset #1 (id:1) has been deleted

[kelvinp, 2015-02-26 22:27:54]

kelvinp@chromium.org changed reviewers:
+ garykac@chromium.org, jamiewalch@chromium.org

[kelvinp, 2015-02-26 22:29:38]

PTAL

[Jamie, 2015-02-26 23:20:14]

Looks great! Just a few nits.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_client_plugin.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_client_plugin.js:51: function(host, localJid,
credentials) {
credentialsProvider would be more consistent naming (here and elsewhere).

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin.js (left):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:103:
remoting.ClientPlugin.prototype.useAsyncPinDialog = function() {};
Please mention in the CL description that you're removing this member.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin_impl.js:800: * @param
{remoting.ThirdPartyToken} credentials
I think |token| was a better name here. If you prefer, you can rename it
|thirdPartyToken|, but |credentials| is a bit vague, especially if you don't
change the CredentialsProvider instance naming.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/credentials_provider.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/credentials_provider.js:81: return this.accessCode_ ||
'';
Would an assert(supports(ACCESS_CODE)) be appropriate here (and similarly
below)?

Also, you've used access-code and shared-secret somewhat interchangeable. Can
you pick one and standardize on it?

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (left):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:280: 
Leave this blank line. It was intended to suggest that the comment above refers
only to the call to cancel(). Also below.

[kelvinp, 2015-02-27 01:03:11]

Patchset #2 (id:40001) has been deleted

[kelvinp, 2015-02-27 01:03:21]

PTAL

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_client_plugin.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_client_plugin.js:51: function(host, localJid,
credentials) {
On 2015/02/26 23:20:14, Jamie wrote:
> credentialsProvider would be more consistent naming (here and elsewhere).

Done.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin.js (left):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:103:
remoting.ClientPlugin.prototype.useAsyncPinDialog = function() {};
On 2015/02/26 23:20:14, Jamie wrote:
> Please mention in the CL description that you're removing this member.

Done.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin_impl.js:800: * @param
{remoting.ThirdPartyToken} credentials
On 2015/02/26 23:20:14, Jamie wrote:
> I think |token| was a better name here. If you prefer, you can rename it
> |thirdPartyToken|, but |credentials| is a bit vague, especially if you don't
> change the CredentialsProvider instance naming.

Done.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/credentials_provider.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/credentials_provider.js:81: return this.accessCode_ ||
'';
On 2015/02/26 23:20:14, Jamie wrote:
> Would an assert(supports(ACCESS_CODE)) be appropriate here (and similarly
> below)?
> 
> Also, you've used access-code and shared-secret somewhat interchangeable. Can
> you pick one and standardize on it?

Yes, will use AccessCode instead?
No, the problem is during connect, the plugin will always need an access Code to
be passed in, therefore, we need to return a dummy value.

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (left):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:280: 
On 2015/02/26 23:20:14, Jamie wrote:
> Leave this blank line. It was intended to suggest that the comment above
refers
> only to the call to cancel(). Also below.

Done.

[Jamie, 2015-02-27 01:53:03]

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/credentials_provider.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/credentials_provider.js:81: return this.accessCode_ ||
'';
On 2015/02/27 01:03:21, kelvinp wrote:
> On 2015/02/26 23:20:14, Jamie wrote:
> > Would an assert(supports(ACCESS_CODE)) be appropriate here (and similarly
> > below)?
>
> No, the problem is during connect, the plugin will always need an access Code
to
> be passed in, therefore, we need to return a dummy value.

Maybe a better way of phrasing it would be to ask what's the value of the
supports() method? If callers are expected to invoke that and respect the
result, then the accessor methods should assert (and the case you mention would
need to fall-back on a default of '' explicitly). If the accessors are going to
try to provide sensible defaults, then there's no need for the supports()
method. My preference is for the former because it guards against misuse of the
class and doesn't enforce a single (albeit sensible) default behaviour for all
callers.

[kelvinp, 2015-02-27 19:47:40]

PTAL

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/credentials_provider.js (right):

https://codereview.chromium.org/954373002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/credentials_provider.js:81: return this.accessCode_ ||
'';
On 2015/02/27 01:53:03, Jamie wrote:
> On 2015/02/27 01:03:21, kelvinp wrote:
> > On 2015/02/26 23:20:14, Jamie wrote:
> > > Would an assert(supports(ACCESS_CODE)) be appropriate here (and similarly
> > > below)?
> >
> > No, the problem is during connect, the plugin will always need an access
Code
> to
> > be passed in, therefore, we need to return a dummy value.
> 
> Maybe a better way of phrasing it would be to ask what's the value of the
> supports() method? If callers are expected to invoke that and respect the
> result, then the accessor methods should assert (and the case you mention
would
> need to fall-back on a default of '' explicitly). If the accessors are going
to
> try to provide sensible defaults, then there's no need for the supports()
> method. My preference is for the former because it guards against misuse of
the
> class and doesn't enforce a single (albeit sensible) default behaviour for all
> callers.

Agreed.  As discussed, this is due to the fact that the client_plugin expects us
to call useAsyncPinDialog when an accessCode is NOT present.  Underneath the
hood, if useAsyncPinDialog is not called, it will internally binds the fetchPIN
callback to the accessCode.
I am going to mimic the same logic in Credentials provider instead.

[Jamie, 2015-02-28 00:41:12]

LGTM :)

https://codereview.chromium.org/954373002/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/credentials_provider.js (right):

https://codereview.chromium.org/954373002/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/credentials_provider.js:46: this.fetchPin_ =
(args.accessCode) ? this.getAccessCode_ : args.fetchPin;
Assert that exactly one of accessCode or fetchPin is defined?

[kelvinp, 2015-03-02 19:48:38]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-03-02 19:55:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/954373002/80001

[commit-bot: I haz the power, 2015-03-02 21:40:55]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-02 21:41:58]

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