Show a 'remember me' checkbox in the web-app when connecting to a host that supports pairing.

Show a 'remember me' checkbox in the web-app when connecting to a host that supports pairing.

Currently this checkbox does nothing. The next CL will add code to request
pairing if it was checked.

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

[Jamie, 2013-05-22 22:55:44]

PTAL. I still need to clean up the HTML/CSS a bit.

[rmsousa, 2013-05-23 00:14:58]

Looks good in general, just some formatting/string nits.

https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:179: const
protocol::SecretFetchedCallback& secret_fetched_callback,
Nit: The convention is to have the callback as the last parameter, here and
below.

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Remember this PIN so that I don't
need to enter it again.
"Remember PIN" is a bit misleading... I'd go for a phrasing more focused on the
end-user result and less on how we're supposedly implementing it (especially
since we're not really implementing it that way).

The 2-step verification dialog uses "Don't ask for codes again on this
computer". I'd go for something on those lines,like "Don't ask for a PIN again
when connecting to this host from this computer".

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
remoting/webapp/client_plugin_async.js:268: if (pairingSupported !== undefined
&&
Nit: I think it's clearer to explicitly test for 'pairingSupported' in
message.data (and add a comment saying that this is valid for messages from
older client plugins), and then do error handling.

Regardless of style, please ensure that pairingSupported always holds a boolean
(not undefined).

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
remoting/webapp/client_screen.js:272: var requestPin = function(onPinFetched,
supportsPairing) {
Nit: Does javascript also have the convention of putting the callback last? If
so, please do.

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
remoting/webapp/client_screen.js:294: onPinFetched(pin);
Nit: add todo here

[Jamie, 2013-05-23 01:10:07]

ptal

https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:179: const
protocol::SecretFetchedCallback& secret_fetched_callback,
On 2013/05/23 00:14:59, rmsousa wrote:
> Nit: The convention is to have the callback as the last parameter, here and
> below.

Done.

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Remember this PIN so that I don't
need to enter it again.
On 2013/05/23 00:14:59, rmsousa wrote:
> "Remember PIN" is a bit misleading... I'd go for a phrasing more focused on
the
> end-user result and less on how we're supposedly implementing it (especially
> since we're not really implementing it that way).
> 
> The 2-step verification dialog uses "Don't ask for codes again on this
> computer". I'd go for something on those lines,like "Don't ask for a PIN again
> when connecting to this host from this computer".

Done.

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
remoting/webapp/client_plugin_async.js:268: if (pairingSupported !== undefined
&&
On 2013/05/23 00:14:59, rmsousa wrote:
> Nit: I think it's clearer to explicitly test for 'pairingSupported' in
> message.data (and add a comment saying that this is valid for messages from
> older client plugins), and then do error handling.
> 
> Regardless of style, please ensure that pairingSupported always holds a
boolean
> (not undefined).

Done.

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
remoting/webapp/client_screen.js:272: var requestPin = function(onPinFetched,
supportsPairing) {
On 2013/05/23 00:14:59, rmsousa wrote:
> Nit: Does javascript also have the convention of putting the callback last? If
> so, please do.

Done.

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
remoting/webapp/client_screen.js:294: onPinFetched(pin);
On 2013/05/23 00:14:59, rmsousa wrote:
> Nit: add todo here

To check the state of the checkbox? That's actually done post-connection, and
it's only part of a larger CL, so I don't think it's worth calling out that
specific part as a TODO. It's also likely to be the next CL I'm landing.

[Jamie, 2013-05-23 21:08:37]

On 2013/05/23 01:10:07, Jamie wrote:
> ptal
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
> File remoting/client/plugin/chromoting_instance.h (right):
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/client/plugin/chr...
> remoting/client/plugin/chromoting_instance.h:179: const
> protocol::SecretFetchedCallback& secret_fetched_callback,
> On 2013/05/23 00:14:59, rmsousa wrote:
> > Nit: The convention is to have the callback as the last parameter, here and
> > below.
> 
> Done.
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
> File remoting/resources/remoting_strings.grd (right):
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
> remoting/resources/remoting_strings.grd:556: Remember this PIN so that I don't
> need to enter it again.
> On 2013/05/23 00:14:59, rmsousa wrote:
> > "Remember PIN" is a bit misleading... I'd go for a phrasing more focused on
> the
> > end-user result and less on how we're supposedly implementing it (especially
> > since we're not really implementing it that way).
> > 
> > The 2-step verification dialog uses "Don't ask for codes again on this
> > computer". I'd go for something on those lines,like "Don't ask for a PIN
again
> > when connecting to this host from this computer".
> 
> Done.
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
> File remoting/webapp/client_plugin_async.js (right):
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_plu...
> remoting/webapp/client_plugin_async.js:268: if (pairingSupported !== undefined
> &&
> On 2013/05/23 00:14:59, rmsousa wrote:
> > Nit: I think it's clearer to explicitly test for 'pairingSupported' in
> > message.data (and add a comment saying that this is valid for messages from
> > older client plugins), and then do error handling.
> > 
> > Regardless of style, please ensure that pairingSupported always holds a
> boolean
> > (not undefined).
> 
> Done.
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
> File remoting/webapp/client_screen.js (right):
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
> remoting/webapp/client_screen.js:272: var requestPin = function(onPinFetched,
> supportsPairing) {
> On 2013/05/23 00:14:59, rmsousa wrote:
> > Nit: Does javascript also have the convention of putting the callback last?
If
> > so, please do.
> 
> Done.
> 
>
https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
> remoting/webapp/client_screen.js:294: onPinFetched(pin);
> On 2013/05/23 00:14:59, rmsousa wrote:
> > Nit: add todo here
> 
> To check the state of the checkbox? That's actually done post-connection, and
> it's only part of a larger CL, so I don't think it's worth calling out that
> specific part as a TODO. It's also likely to be the next CL I'm landing.

Ping?

[rmsousa, 2013-05-23 21:17:52]

lgtm

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Remember this PIN so that I don't
need to enter it again.
On 2013/05/23 01:10:08, Jamie wrote:
> On 2013/05/23 00:14:59, rmsousa wrote:
> > "Remember PIN" is a bit misleading... I'd go for a phrasing more focused on
> the
> > end-user result and less on how we're supposedly implementing it (especially
> > since we're not really implementing it that way).
> > 
> > The 2-step verification dialog uses "Don't ask for codes again on this
> > computer". I'd go for something on those lines,like "Don't ask for a PIN
again
> > when connecting to this host from this computer".
> 
> Done.

Nit: also the description (and the ID)

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/webapp/client_scr...
remoting/webapp/client_screen.js:294: onPinFetched(pin);
On 2013/05/23 01:10:08, Jamie wrote:
> On 2013/05/23 00:14:59, rmsousa wrote:
> > Nit: add todo here
> 
> To check the state of the checkbox? That's actually done post-connection, and
> it's only part of a larger CL, so I don't think it's worth calling out that
> specific part as a TODO. It's also likely to be the next CL I'm landing.

For the next CL: I think it's better to save the checkbox state here (say, to
remoting.clientSession), and then use that saved state post-connection, rather
than going back to the checkbox UI element directly, long after the dialog was
closed.

[Jamie, 2013-05-23 21:36:12]

fyi

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/15685008/diff/5001/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Remember this PIN so that I don't
need to enter it again.
On 2013/05/23 21:17:52, rmsousa wrote:
> On 2013/05/23 01:10:08, Jamie wrote:
> > On 2013/05/23 00:14:59, rmsousa wrote:
> > > "Remember PIN" is a bit misleading... I'd go for a phrasing more focused
on
> > the
> > > end-user result and less on how we're supposedly implementing it
(especially
> > > since we're not really implementing it that way).
> > > 
> > > The 2-step verification dialog uses "Don't ask for codes again on this
> > > computer". I'd go for something on those lines,like "Don't ask for a PIN
> again
> > > when connecting to this host from this computer".
> > 
> > Done.
> 
> Nit: also the description (and the ID)

I've updated the description. I think the ID is fine as it is, since it's short
and descriptive and only wrong with regards to the implementation details :)

[commit-bot: I haz the power, 2013-05-23 21:37:08]

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

[commit-bot: I haz the power, 2013-05-24 07:06:47]

Change committed as 202001