Don't transition to in-session mode until the connection has been established.

Don't transition to in-session mode until the connection has been established.

BUG=83648
TEST=

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

[Jamie, 2011-07-25 23:09:50]

This causes errors in the access code to be reported, even if they're in the
host secret portion.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.html
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.h...
remoting/webapp/me2mom/choice.html:242: <section id="session-mode" class="mode
in-session-element client-element">
The plugin has to be visible in order for it to load, which we now want to
happen before the state transition.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:115: this.plugin.height = 0;
Although the plugin is blank until we transition to a new state, this feels like
the correct behaviour.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:328: this.state = state;
Defer setting the current state so that the callback can examine it.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:466: showConnectError_(404);
This is not quite correct, as it will report "Invalid access code", regardless
of what actually went wrong. I'd like to fix this up by having the host send a
suitable error code, but that's a separate CL.

[awong, 2011-07-26 18:51:27]

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.html
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.h...
remoting/webapp/me2mom/choice.html:242: <section id="session-mode" class="mode
in-session-element client-element">
On 2011/07/25 23:09:50, Jamie wrote:
> The plugin has to be visible in order for it to load, which we now want to
> happen before the state transition.

Hmm...I wonder if we ever want it to not load.  I guess it takes up more
resources.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:328: this.state = state;
On 2011/07/25 23:09:50, Jamie wrote:
> Defer setting the current state so that the callback can examine it.

Huh...We should document clearly that onStateChange is a "pre-change" event. 
Normally, I code things post-change, but as long as we're consistent and clear
about the synchronization, either method works.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:466: showConnectError_(404);
On 2011/07/25 23:09:50, Jamie wrote:
> This is not quite correct, as it will report "Invalid access code", regardless
> of what actually went wrong. I'd like to fix this up by having the host send a
> suitable error code, but that's a separate CL.

Add TODO?

[Jamie, 2011-07-26 19:02:34]

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.html
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.h...
remoting/webapp/me2mom/choice.html:242: <section id="session-mode" class="mode
in-session-element client-element">
On 2011/07/26 18:51:27, awong wrote:
> On 2011/07/25 23:09:50, Jamie wrote:
> > The plugin has to be visible in order for it to load, which we now want to
> > happen before the state transition.
> 
> Hmm...I wonder if we ever want it to not load.  I guess it takes up more
> resources.

I'm not sure I understand your point. We wouldn't want it to load in host mode,
for example, and if a second client connection is made, we want to tear down the
first plugin and recreate it, so we need the on-demand creation semantics.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:328: this.state = state;
On 2011/07/26 18:51:27, awong wrote:
> On 2011/07/25 23:09:50, Jamie wrote:
> > Defer setting the current state so that the callback can examine it.
> 
> Huh...We should document clearly that onStateChange is a "pre-change" event. 
> Normally, I code things post-change, but as long as we're consistent and clear
> about the synchronization, either method works.

Yes, that's generally my preference too, but if the callbacks needs both the
before and after states, pre-change makes more sense. I've added a comment.

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:466: showConnectError_(404);
On 2011/07/26 18:51:27, awong wrote:
> On 2011/07/25 23:09:50, Jamie wrote:
> > This is not quite correct, as it will report "Invalid access code",
regardless
> > of what actually went wrong. I'd like to fix this up by having the host send
a
> > suitable error code, but that's a separate CL.
> 
> Add TODO?

Done.

[awong, 2011-07-26 19:08:11]

On Tue, Jul 26, 2011 at 12:02 PM, <jamiewalch@chromium.org> wrote:

>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/choice.html<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.html>
> File remoting/webapp/me2mom/choice.**html (right):
>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/choice.html#**newcode242<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/choice.html#newcode242>
> remoting/webapp/me2mom/choice.**html:242: <section id="session-mode"
> class="mode in-session-element client-element">
> On 2011/07/26 18:51:27, awong wrote:
>
>> On 2011/07/25 23:09:50, Jamie wrote:
>> > The plugin has to be visible in order for it to load, which we now
>>
> want to
>
>> > happen before the state transition.
>>
>
>  Hmm...I wonder if we ever want it to not load.  I guess it takes up
>>
> more
>
>> resources.
>>
>
> I'm not sure I understand your point. We wouldn't want it to load in
> host mode, for example, and if a second client connection is made, we
> want to tear down the first plugin and recreate it, so we need the
> on-demand creation semantics.
>
>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/client_session.**js<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_session.js>
> File remoting/webapp/me2mom/client_**session.js (right):
>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/client_session.**js#newcode328<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/client_session.js#newcode328>
> remoting/webapp/me2mom/client_**session.js:328: this.state = state;
> On 2011/07/26 18:51:27, awong wrote:
>
>> On 2011/07/25 23:09:50, Jamie wrote:
>> > Defer setting the current state so that the callback can examine it.
>>
>
>  Huh...We should document clearly that onStateChange is a "pre-change"
>>
> event.
>
>> Normally, I code things post-change, but as long as we're consistent
>>
> and clear
>
>> about the synchronization, either method works.
>>
>
> Yes, that's generally my preference too, but if the callbacks needs both
> the before and after states, pre-change makes more sense. I've added a
> comment.


Not sure I agree. signal() on unix does it the otherway...


>
>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/remoting.js<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting.js>
> File remoting/webapp/me2mom/**remoting.js (right):
>
> http://codereview.chromium.**org/7511001/diff/1/remoting/**
>
webapp/me2mom/remoting.js#**newcode466<http://codereview.chromium.org/7511001/diff/1/remoting/webapp/me2mom/remoting.js#newcode466>
> remoting/webapp/me2mom/**remoting.js:466: showConnectError_(404);
> On 2011/07/26 18:51:27, awong wrote:
>
>> On 2011/07/25 23:09:50, Jamie wrote:
>> > This is not quite correct, as it will report "Invalid access code",
>>
> regardless
>
>> > of what actually went wrong. I'd like to fix this up by having the
>>
> host send a
>
>> > suitable error code, but that's a separate CL.
>>
>
>  Add TODO?
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/7511001/<http://codereview.chromium.org/7511...
>

[Jamie, 2011-07-26 19:54:28]

> > Yes, that's generally my preference too, but if the callbacks needs both
> > the before and after states, pre-change makes more sense. I've added a
> > comment.
> 
> 
> Not sure I agree. signal() on unix does it the otherway...

As discussed, I've changed the semantics so that the callback is passed the old
state and the new one is accessible via the |state| property.

[garykac, 2011-07-26 21:11:05]

LGTM