Implement 3 PIN browser tests

Cancel PIN browser test
Update PIN browser test
Invalid PIN browser test

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

[kelvinp, 2014-05-08 22:30:11]

ptal

[Jamie, 2014-05-09 01:02:17]

This would be a perfect opportunity to start using Promises. I think it would
make the code a lot more readable. It doesn't necessarily need to be done for
this CL, but I think the sooner it's done, the more browser tests will be able
to make use of it (and the fewer will need to be rewritten to do so).

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:26: LoadScript(content,
FILE_PAHT_LITERAL("browser_test.js"));
s/PAHT/PATH/ (here and elsewhere).

Does this compile?

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:27: LoadScript(content,
FILE_PAHT_LITERAL("cancel_pin_browser_test.js"));
These tests are identical to this point. Can you pull the initialization into
the SetUp method?

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:54: }
You're adding quotes anyway, so I don't think you need this conditional.

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:26: onSessionError:
this.onSessionError_.bind(this)
I don't think there's a lot of value in pre-binding these, since I don't think
you need to refer to them more than once.

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:38:
this.bound_.onSessionError);
What's the default time-out for waitForUIMode? If it's NONE, then the first call
should override it, as should most of the other calls in this file; if it's
anything else, then the second call should override it (otherwise, if it takes a
long time, the test may fail with a messaging complaining that it never reached
state FAILED, which is a bit confusing).

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:60:
browserTest.assert(actual == expected);
This should not be an assertion. If the error is not what is expected, that's a
test failure and should be reported with a suitable error message (maybe that's
what assert does, but the name implies a coding error in the test, which is not
the case here).

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:20:
browserTest.assert(typeof data.pin != undefined);
Copy/paste bug from the other tests?

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:61: // On Linux, we
restart the host when the PIN is changed. Sleep for five
You're actually sleeping for 30s.

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:64: // for new incoming
connections?
You could poll the daemon state. Better still, add a test event for it :)

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:70: // invalid PIN
temporarily blocks future logins.
Actually, this is worse because you're temporarily blocking connections for the
_next_ test, which has no way of knowing it needs to handle that case. It would
be better to modify the host to allow one bad PIN without blocking connections,
or handle that case explicitly (perhaps by waiting a few seconds between
connections). Then, this test (and all others for which it's relevant) should
make the failed attempt first.

[kelvinp, 2014-05-12 21:08:34]

PTAL

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:26: LoadScript(content,
FILE_PAHT_LITERAL("browser_test.js"));
On 2014/05/09 01:02:18, Jamie wrote:
> s/PAHT/PATH/ (here and elsewhere).
> 
> Does this compile?

Done.

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:27: LoadScript(content,
FILE_PAHT_LITERAL("cancel_pin_browser_test.js"));
On 2014/05/09 01:02:18, Jamie wrote:
> These tests are identical to this point. Can you pull the initialization into
> the SetUp method?

Done.

https://codereview.chromium.org/273753002/diff/20001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:54: }
On 2014/05/09 01:02:18, Jamie wrote:
> You're adding quotes anyway, so I don't think you need this conditional.

Done.

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:38:
this.bound_.onSessionError);
On 2014/05/09 01:02:18, Jamie wrote:
> What's the default time-out for waitForUIMode? If it's NONE, then the first
call
> should override it, as should most of the other calls in this file; if it's
> anything else, then the second call should override it (otherwise, if it takes
a
> long time, the test may fail with a messaging complaining that it never
reached
> state FAILED, which is a bit confusing).

Done.

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:60:
browserTest.assert(actual == expected);
On 2014/05/09 01:02:18, Jamie wrote:
> This should not be an assertion. If the error is not what is expected, that's
a
> test failure and should be reported with a suitable error message (maybe
that's
> what assert does, but the name implies a coding error in the test, which is
not
> the case here).

Rename to expect equal

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

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:20:
browserTest.assert(typeof data.pin != undefined);
On 2014/05/09 01:02:18, Jamie wrote:
> Copy/paste bug from the other tests?

Done.

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:61: // On Linux, we
restart the host when the PIN is changed. Sleep for five
On 2014/05/09 01:02:18, Jamie wrote:
> You're actually sleeping for 30s.

Done.

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:64: // for new incoming
connections?
On 2014/05/09 01:02:18, Jamie wrote:
> You could poll the daemon state. Better still, add a test event for it :)

Discussed offline.  Will follow up with a different CL.

https://codereview.chromium.org/273753002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:70: // invalid PIN
temporarily blocks future logins.
On 2014/05/09 01:02:18, Jamie wrote:
> Actually, this is worse because you're temporarily blocking connections for
the
> _next_ test, which has no way of knowing it needs to handle that case. It
would
> be better to modify the host to allow one bad PIN without blocking
connections,
> or handle that case explicitly (perhaps by waiting a few seconds between
> connections). Then, this test (and all others for which it's relevant) should
> make the failed attempt first.

Done.  In practice, it should be fine because the setup/teardown between each
test is long enough to allow the timer to reset.  However, to reduce the
reliance on the side effect, I added a 2s timeout after each failure attempt.

[Jamie, 2014-05-13 00:15:47]

This is much clearer, thanks. Just a few more suggestions, mostly minor.

https://codereview.chromium.org/273753002/diff/50001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/50001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:27: LoadScript(app_web_content(),
FILE_PATH_LITERAL("browser_test.js"));
Nit: No need for blank lines between these.

https://codereview.chromium.org/273753002/diff/50001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:66: "new_pin: '314159'"
This test is not idempotent. Specifically, once it's been run once, the PIN will
be set to 314159, and subsequent invocations won't actually test anything. Or am
I missing something?

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/base.js
File remoting/webapp/base.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/base.js#...
remoting/webapp/base.js:115: base.Promise = function () {};
Nit: No space after "function".

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/base.js#...
remoting/webapp/base.js:121: base.Promise.timeout = function (delay) {
"timeout" usually suggests a failure; perhaps "sleep" would be a better name for
this?

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:76: var stack_trace;
Does this need to be initialized, or will it be "undefined"?

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:134: function onUIModeChanged
(mode) {
Nit: No space before parens.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:144: timerId =
window.setTimeout(onTimeout.bind(window, timerId), opt_timeout);
onTimeout doesn't use the timerId parameter, so this bind is unnecessary.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:159: 'Expected the Me2Me connection
to fail instead of connected');
Nit: Remove "instead of connected".

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:180: // Timeout if the session is
not connected within 15 seconds.
15s should be declared somewhere as a named constant (local to this function is
fine, unless you're re-using it). I would also recommend increasing it; I would
be concerned if connections were regularly taking longer than 15s, but I
wouldn't fail a test for a one-off slow connection.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:26: var promise =
browserTest.onUIMode(AppMode.HOME);
Is there a reason to create the promise before clicking the control? If not,
then I think the code reads more clearly the other way around; i.e. "click on
the control and then wait for the mode to change." It also saves a line of code
as there's no need to declare a variable for the promise.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:33: }.bind(this)).then(
Binding something that isn't a member function always seems a bit iffy to me.
Since there's no need for clickOnMe2MeHost to be a member function, I think it's
better to move it inline.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:49: // Enter the PIN.
This comment basically duplicates the name of the function. I don't think it
adds anything.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
File remoting/webapp/browser_test/invalid_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:31: }, function
(reason) {
Nit: No space before parens.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:44: // Enter the PIN.
Unnecessary comment.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:32:
browserTest.clickOnControl.bind(window, 'host-config-done-dismiss')
Why bind to window here, but null below? My previous comment about not binding
non-member function applies here too, but if bind does make the code clearer, I
think null is a more explicit indication that you're binding a global function.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:36:
base.Promise.timeout.bind(null, 10000)
Declare this as a named constant.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:36:
base.Promise.timeout.bind(null, 10000)
Does this need to be a separate step from the previous one?

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:43:
base.Promise.timeout.bind(null, 2000)
Declare this as a named constant.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:78: return
base.Promise.timeout(500).then(function(){
Why is this sleep needed? Assuming it is, please make this a named constant.

[weitao, 2014-05-13 17:13:09]

https://codereview.chromium.org/273753002/diff/50001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/50001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:16: void SetUpTest() {
This seems common enough that it may be better to move it to the base class so
that other tests can use it. If you do that I'd also recommend a more
descriptive name like SetUpTestForMe2Me.

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/base.js
File remoting/webapp/base.js (right):

https://codereview.chromium.org/273753002/diff/50001/remoting/webapp/base.js#...
remoting/webapp/base.js:121: base.Promise.timeout = function (delay) {
Second that.
On 2014/05/13 00:15:48, Jamie wrote:
> "timeout" usually suggests a failure; perhaps "sleep" would be a better name
for
> this?

[kelvinp, 2014-05-13 18:26:45]

I deleted the previous patch set by accident.
I have addressed all CR feedbacks.
PTAL

[Jamie, 2014-05-13 18:59:53]

Very close now :) I'm almost ready to LG this, but the additional
promisification of onUIMode might warrant a final look.

https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:16: LoadScript(content,
FILE_PATH_LITERAL("browser_test.js"));
Wasn't this previously done in the helper function? Is there any reason not to
do so?

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:28: // changes before
clicking the button.
My understanding is that one of the advantages of promises is that they isolate
callers from these sorts of race conditions. Does it make sense to have the
onUIMode promise satisifiable even if the mode is already set?

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:32: // On Linux, we
restart the host after changing the password, need to sleep
s/password/PIN/

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:34: return
base.Promise.sleep(HOST_RESTART_WAIT);
There are a few places you change the PIN. Would it be better to move this sleep
into changePIN_?

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:53:
this.changePIN_.bind(this, data.old_pin)
Do promises have a concept of "finally"? Restoring the PIN is a clean-up action
that seems ideally suited to a finally handler. If not (or if it's important
that the PIN be restored before pass or fail is called), then I think a single
comment explaining that the PIN is restored regardless of success or failure
would be more useful than separate comments highlighting the success and failure
statements.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:99: // the PIN prompt is
still up without the timeout.
s/is still up/does not dismiss/

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:108: }.bind(this));
Is this the bind you mentioned wanting to remove?

[kelvinp, 2014-05-13 23:11:00]

https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
chrome/test/remoting/pin_browsertest.cc:16: LoadScript(content,
FILE_PATH_LITERAL("browser_test.js"));
On 2014/05/13 18:59:54, Jamie wrote:
> Wasn't this previously done in the helper function? Is there any reason not to
> do so?

Yes, because I thought including the script file is specific to me2me browser
test that is written in JavaScript.  But I guess it doesn't hurt to include
browser_test.js in the helper function as well.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:28: // changes before
clicking the button.
On 2014/05/13 18:59:54, Jamie wrote:
> My understanding is that one of the advantages of promises is that they
isolate
> callers from these sorts of race conditions. Does it make sense to have the
> onUIMode promise satisifiable even if the mode is already set?

Good Idea. Done.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:32: // On Linux, we
restart the host after changing the password, need to sleep
On 2014/05/13 18:59:54, Jamie wrote:
> s/password/PIN/

Done.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:34: return
base.Promise.sleep(HOST_RESTART_WAIT);
On 2014/05/13 18:59:54, Jamie wrote:
> There are a few places you change the PIN. Would it be better to move this
sleep
> into changePIN_?

Done.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:53:
this.changePIN_.bind(this, data.old_pin)
On 2014/05/13 18:59:54, Jamie wrote:
> Do promises have a concept of "finally"? Restoring the PIN is a clean-up
action
> that seems ideally suited to a finally handler. If not (or if it's important
> that the PIN be restored before pass or fail is called), then I think a single
> comment explaining that the PIN is restored regardless of success or failure
> would be more useful than separate comments highlighting the success and
failure
> statements.

The WinJS implementation has the done() method which is similar to finally. 
Unfortunately, this is not available in the V8 implementation.  (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

Comments updated.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:99: // the PIN prompt is
still up without the timeout.
On 2014/05/13 18:59:54, Jamie wrote:
> s/is still up/does not dismiss/

Done.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:108: }.bind(this));
On 2014/05/13 18:59:54, Jamie wrote:
> Is this the bind you mentioned wanting to remove?

Bingo!

[kelvinp, 2014-05-13 23:19:10]

On 2014/05/13 23:11:00, kelvinp wrote:
>
https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
> File chrome/test/remoting/pin_browsertest.cc (right):
> 
>
https://codereview.chromium.org/273753002/diff/90001/chrome/test/remoting/pin...
> chrome/test/remoting/pin_browsertest.cc:16: LoadScript(content,
> FILE_PATH_LITERAL("browser_test.js"));
> On 2014/05/13 18:59:54, Jamie wrote:
> > Wasn't this previously done in the helper function? Is there any reason not
to
> > do so?
> 
> Yes, because I thought including the script file is specific to me2me browser
> test that is written in JavaScript.  But I guess it doesn't hurt to include
> browser_test.js in the helper function as well.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/cancel_pin_browser_test.js:28: // changes before
> clicking the button.
> On 2014/05/13 18:59:54, Jamie wrote:
> > My understanding is that one of the advantages of promises is that they
> isolate
> > callers from these sorts of race conditions. Does it make sense to have the
> > onUIMode promise satisifiable even if the mode is already set?
> 
> Good Idea. Done.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> File remoting/webapp/browser_test/update_pin_browser_test.js (right):
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/update_pin_browser_test.js:32: // On Linux, we
> restart the host after changing the password, need to sleep
> On 2014/05/13 18:59:54, Jamie wrote:
> > s/password/PIN/
> 
> Done.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/update_pin_browser_test.js:34: return
> base.Promise.sleep(HOST_RESTART_WAIT);
> On 2014/05/13 18:59:54, Jamie wrote:
> > There are a few places you change the PIN. Would it be better to move this
> sleep
> > into changePIN_?
> 
> Done.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/update_pin_browser_test.js:53:
> this.changePIN_.bind(this, data.old_pin)
> On 2014/05/13 18:59:54, Jamie wrote:
> > Do promises have a concept of "finally"? Restoring the PIN is a clean-up
> action
> > that seems ideally suited to a finally handler. If not (or if it's important
> > that the PIN be restored before pass or fail is called), then I think a
single
> > comment explaining that the PIN is restored regardless of success or failure
> > would be more useful than separate comments highlighting the success and
> failure
> > statements.
> 
> The WinJS implementation has the done() method which is similar to finally. 
> Unfortunately, this is not available in the V8 implementation.  (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 
> Comments updated.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/update_pin_browser_test.js:99: // the PIN prompt
is
> still up without the timeout.
> On 2014/05/13 18:59:54, Jamie wrote:
> > s/is still up/does not dismiss/
> 
> Done.
> 
>
https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
> remoting/webapp/browser_test/update_pin_browser_test.js:108: }.bind(this));
> On 2014/05/13 18:59:54, Jamie wrote:
> > Is this the bind you mentioned wanting to remove?
> 
> Bingo!

PTAL

[Jamie, 2014-05-13 23:27:04]

LGTM with a couple of nits.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:28: // changes before
clicking the button.
On 2014/05/13 23:11:01, kelvinp wrote:
> On 2014/05/13 18:59:54, Jamie wrote:
> > My understanding is that one of the advantages of promises is that they
> isolate
> > callers from these sorts of race conditions. Does it make sense to have the
> > onUIMode promise satisifiable even if the mode is already set?
> 
> Good Idea. Done.

I think you can get rid of the comment now. Also, you can save a few lines of
code by doing the click before creating the promise, and simply returning the
promise directly, rather than assigning it to a variable, as you've done in some
of the other places.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:99: // the PIN prompt is
still up without the timeout.
On 2014/05/13 23:11:01, kelvinp wrote:
> On 2014/05/13 18:59:54, Jamie wrote:
> > s/is still up/does not dismiss/
> 
> Done.

I don't see this change.

[kelvinp, 2014-05-13 23:50:59]

Thank you Jamie.

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/273753002/diff/90001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:99: // the PIN prompt is
still up without the timeout.
On 2014/05/13 23:27:05, Jamie wrote:
> On 2014/05/13 23:11:01, kelvinp wrote:
> > On 2014/05/13 18:59:54, Jamie wrote:
> > > s/is still up/does not dismiss/
> > 
> > Done.
> 
> I don't see this change.

My bad.  Fixed.

[kelvinp, 2014-05-13 23:51:06]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-05-13 23:52:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/273753002/140001

[commit-bot: I haz the power, 2014-05-14 03:09:04]

Change committed as 270317