Add separate nonce version of connect call.

Add separate nonce version of connect calls to ChromotingScriptableObject.

BUG=none
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86573

[Wez, 2011-05-19 00:04:00]

What's the purpose of this CL?

[garykac, 2011-05-19 00:10:16]

From discussion with Albert, to have separate interfaces for the
connect-via-nonce and connect-via-auth cases.

Not critical now, but this is leftover from my non CL.

[awong, 2011-05-19 00:13:31]

Right...This was that discussion about trying to keep the plugin so that it
could be used with, and without a nonce.

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:199: pp::Var
DoConnect_(const std::vector<pp::Var>& args,
This trailing _ seems odd.  Can we rename it something else?

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:207: pp::Var
DoConnectSandboxed_(const std::vector<pp::Var>& args,
Same here. Also indentation.

[Jamie, 2011-05-19 00:16:51]

LGTM with one caveat.

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:81: //   void
connectNonce(string username, string host_jid, string auth_token,
Can you clean up some of the terminology to refer to access code rather than
nonce? I've tried to make this more consistent elsewhere.

[garykac, 2011-05-20 01:33:01]

I folded all the functions into a single connect() and made |sandboxed| an
attribute.

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:81: //   void
connectNonce(string username, string host_jid, string auth_token,
On 2011/05/19 00:16:52, Jamie wrote:
> Can you clean up some of the terminology to refer to access code rather than
> nonce? I've tried to make this more consistent elsewhere.

Switched to using |access_code|.

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:199: pp::Var
DoConnect_(const std::vector<pp::Var>& args,
On 2011/05/19 00:13:31, awong wrote:
> This trailing _ seems odd.  Can we rename it something else?

Oh sure, Jamie can get away with naming things like this, but I can't. ^_^

This is no longer an issue since all the functions have been collapsed into a
single connect().

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:207: pp::Var
DoConnectSandboxed_(const std::vector<pp::Var>& args,
On 2011/05/19 00:13:31, awong wrote:
> Same here. Also indentation.

Done.

[garykac, 2011-05-20 21:34:08]

PTAL since I made changes to the connect() calling convention.

[awong, 2011-05-20 21:36:30]

Jamie, Wez,

Could one of you pickup the review of this CL?  I'm having a hard time
working through this while being build sheriff.

-A

On Fri, May 20, 2011 at 2:34 PM, <garykac@chromium.org> wrote:

> PTAL since I made changes to the connect() calling convention.
>
>
> http://codereview.chromium.org/7042022/
>

[Jamie, 2011-05-20 22:07:58]

I haven't reviewed the whole thing, as if these recommendations are adopted it
will need to change.

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:20: //   readonly
attribute bool sandboxed;
I don't think I like this. An attribute would make sense if it controlled the
behaviour of the connect function without needing to supply different arguments.
However, in this case, I have to set sandboxed and then call an appropriate
version of connect. If I'm going to have to call a different version of connect
anyway, why have the attribute?

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:98: //
From what I can see, the only difference between me2me and me2mom is that in
me2mom we pass an access code. Can we make that parameter optional and go back
to two methods for the connect (we call the one we're getting rid of
connectUnsandboxed if we want, so that the correct approach has the more
sensible name). This seems cleaner to me, and means we can get rid of the
sandboxed attribute and the auth_type parameter.

[Jamie, 2011-05-20 22:09:04]

I haven't reviewed the whole thing, as if these recommendations are adopted it
will need to change.

[awong, 2011-05-20 22:10:26]

FYI, I'd also like to be able to specify the "crednetial" type.  Currently
we're calling it "xmpp_token" but I'd like to be able to select between
xmpp_token and an oauth2_token.

You don't need to absorb that into this change, but keep it in mind when
modifying the API.

On Fri, May 20, 2011 at 3:07 PM, <jamiewalch@chromium.org> wrote:

> I haven't reviewed the whole thing, as if these recommendations are adopted
> it
> will need to change.
>
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
>
> File remoting/client/plugin/chromoting_scriptable_object.h (right):
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
> remoting/client/plugin/chromoting_scriptable_object.h:20: //   readonly
> attribute bool sandboxed;
> I don't think I like this. An attribute would make sense if it
> controlled the behaviour of the connect function without needing to
> supply different arguments. However, in this case, I have to set
> sandboxed and then call an appropriate version of connect. If I'm going
> to have to call a different version of connect anyway, why have the
> attribute?
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
> remoting/client/plugin/chromoting_scriptable_object.h:98: //
> From what I can see, the only difference between me2me and me2mom is
> that in me2mom we pass an access code. Can we make that parameter
> optional and go back to two methods for the connect (we call the one
> we're getting rid of connectUnsandboxed if we want, so that the correct
> approach has the more sensible name). This seems cleaner to me, and
> means we can get rid of the sandboxed attribute and the auth_type
> parameter.
>
>
> http://codereview.chromium.org/7042022/
>

[Wez, 2011-05-20 22:38:59]

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.cc:92:
&ChromotingScriptableObject::DoConnectSandboxedNonce);
Why are you still adding these?

http://codereview.chromium.org/7042022/diff/2002/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.cc:333:
LogDebugInfo("Exception when invoking sendiq JS callback.");
Is This the correct callback name?

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.cc:474: return Var();
Why do we have a method for this, rather than setting it directly?

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:20: //   readonly
attribute bool sandboxed;
On 2011/05/20 22:07:58, Jamie wrote:
> I don't think I like this. An attribute would make sense if it controlled the
> behaviour of the connect function without needing to supply different
arguments.
> However, in this case, I have to set sandboxed and then call an appropriate
> version of connect. If I'm going to have to call a different version of
connect
> anyway, why have the attribute?

The aim was to have the attribute control whether or not we make a sandboxed
connection, and then have a single "connect" call whose arguments differ
depending on the auth scheme the app uses.

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:98: //
On 2011/05/20 22:07:58, Jamie wrote:
> From what I can see, the only difference between me2me and me2mom is that in
> me2mom we pass an access code. Can we make that parameter optional and go back
> to two methods for the connect (we call the one we're getting rid of
> connectUnsandboxed if we want, so that the correct approach has the more
> sensible name). This seems cleaner to me, and means we can get rid of the
> sandboxed attribute and the auth_type parameter.

I'd prefer a single connect() API with "name=value" pair strings, or a connect()
call that gets passed a JS dictionary to pull values from, I think.

[Jamie, 2011-05-23 16:50:18]

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:98: //
On 2011/05/20 22:38:59, Wez wrote:
> On 2011/05/20 22:07:58, Jamie wrote:
> > From what I can see, the only difference between me2me and me2mom is that in
> > me2mom we pass an access code. Can we make that parameter optional and go
back
> > to two methods for the connect (we call the one we're getting rid of
> > connectUnsandboxed if we want, so that the correct approach has the more
> > sensible name). This seems cleaner to me, and means we can get rid of the
> > sandboxed attribute and the auth_type parameter.
> 
> I'd prefer a single connect() API with "name=value" pair strings, or a
connect()
> call that gets passed a JS dictionary to pull values from, I think.

I think we should make the sandboxed API look the way we ultimately want it to
so that it doesn't have to change when we remove the unsandboxed version. I
don't particularly care what the unsandboxed API looks like as it's only
temporary. I think I'd go with:

connect(string host_jid, string client_jid, optional string access_code);
connectUnsandboxed(string host_jid, string username,
                   string xmpp_token, optional string access_code);

[garykac, 2011-05-23 16:58:05]

chromotocol_test_client

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:20: //   readonly
attribute bool sandboxed;
On 2011/05/20 22:38:59, Wez wrote:
> On 2011/05/20 22:07:58, Jamie wrote:
> > I don't think I like this. An attribute would make sense if it controlled
the
> > behaviour of the connect function without needing to supply different
> arguments.
> > However, in this case, I have to set sandboxed and then call an appropriate
> > version of connect. If I'm going to have to call a different version of
> connect
> > anyway, why have the attribute?
> 
> The aim was to have the attribute control whether or not we make a sandboxed
> connection, and then have a single "connect" call whose arguments differ
> depending on the auth scheme the app uses.

I agree with Jamie here. Having a separate signature depending on whether some
global is set doesn't feel right. This is something that I hadn't considered
when we talked about moving 'sandboxed' into an attribute.

http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.h:98: //
On 2011/05/23 16:50:18, Jamie wrote:
> On 2011/05/20 22:38:59, Wez wrote:
> > On 2011/05/20 22:07:58, Jamie wrote:
> > > From what I can see, the only difference between me2me and me2mom is that
in
> > > me2mom we pass an access code. Can we make that parameter optional and go
> back
> > > to two methods for the connect (we call the one we're getting rid of
> > > connectUnsandboxed if we want, so that the correct approach has the more
> > > sensible name). This seems cleaner to me, and means we can get rid of the
> > > sandboxed attribute and the auth_type parameter.
> > 
> > I'd prefer a single connect() API with "name=value" pair strings, or a
> connect()
> > call that gets passed a JS dictionary to pull values from, I think.
> 
> I think we should make the sandboxed API look the way we ultimately want it to
> so that it doesn't have to change when we remove the unsandboxed version. I
> don't particularly care what the unsandboxed API looks like as it's only
> temporary. I think I'd go with:
> 
> connect(string host_jid, string client_jid, optional string access_code);
> connectUnsandboxed(string host_jid, string username,
>                    string xmpp_token, optional string access_code);

I agree with this.

[Wez, 2011-05-23 17:17:18]

As we discussed on Friday, the alternative I was suggesting was to have
something like:

  connect(string host_jid, string client_jid, optional string access_code);

And then global parameters for the XMPP settings for connecting
un-sandboxed.  So connect() would have one signature.

I'm primarily interested in avoiding us having four separate ways of calling
connect().  While it'd be nice to stabilise the connect interface, it's
going to change again if we move to make the plugin more generic, such that
authentication is handled by the web-app.


On 23 May 2011 09:58, <garykac@chromium.org> wrote:

> chromotocol_test_client
>
>
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
> File remoting/client/plugin/chromoting_scriptable_object.h (right):
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
> remoting/client/plugin/chromoting_scriptable_object.h:20: //   readonly
> attribute bool sandboxed;
> On 2011/05/20 22:38:59, Wez wrote:
>
>> On 2011/05/20 22:07:58, Jamie wrote:
>> > I don't think I like this. An attribute would make sense if it
>>
> controlled the
>
>> > behaviour of the connect function without needing to supply
>>
> different
>
>> arguments.
>> > However, in this case, I have to set sandboxed and then call an
>>
> appropriate
>
>> > version of connect. If I'm going to have to call a different version
>>
> of
>
>> connect
>> > anyway, why have the attribute?
>>
>
>  The aim was to have the attribute control whether or not we make a
>>
> sandboxed
>
>> connection, and then have a single "connect" call whose arguments
>>
> differ
>
>> depending on the auth scheme the app uses.
>>
>
> I agree with Jamie here. Having a separate signature depending on
> whether some global is set doesn't feel right. This is something that I
> hadn't considered when we talked about moving 'sandboxed' into an
> attribute.
>
>
>
>
http://codereview.chromium.org/7042022/diff/11001/remoting/client/plugin/chro...
> remoting/client/plugin/chromoting_scriptable_object.h:98: //
> On 2011/05/23 16:50:18, Jamie wrote:
>
>> On 2011/05/20 22:38:59, Wez wrote:
>> > On 2011/05/20 22:07:58, Jamie wrote:
>> > > From what I can see, the only difference between me2me and me2mom
>>
> is that in
>
>> > > me2mom we pass an access code. Can we make that parameter optional
>>
> and go
>
>> back
>> > > to two methods for the connect (we call the one we're getting rid
>>
> of
>
>> > > connectUnsandboxed if we want, so that the correct approach has
>>
> the more
>
>> > > sensible name). This seems cleaner to me, and means we can get rid
>>
> of the
>
>> > > sandboxed attribute and the auth_type parameter.
>> >
>> > I'd prefer a single connect() API with "name=value" pair strings, or
>>
> a
>
>> connect()
>> > call that gets passed a JS dictionary to pull values from, I think.
>>
>
>  I think we should make the sandboxed API look the way we ultimately
>>
> want it to
>
>> so that it doesn't have to change when we remove the unsandboxed
>>
> version. I
>
>> don't particularly care what the unsandboxed API looks like as it's
>>
> only
>
>> temporary. I think I'd go with:
>>
>
>  connect(string host_jid, string client_jid, optional string
>>
> access_code);
>
>> connectUnsandboxed(string host_jid, string username,
>>                    string xmpp_token, optional string access_code);
>>
>
> I agree with this.
>
>
> http://codereview.chromium.org/7042022/
>

[commit-bot: I haz the power, 2011-05-25 05:57:29]

Change committed as 86573