Use a separate window to notify the user that permissions are required.

Use a separate window to notify the user that permissions are required.

This also moves the in-DOM equivalent under crd/ since it's no longer cross-application.

BUG=b/19484255

Committed: https://crrev.com/d0bb0dff1e0ac4c71dcd76caeadcbc32a23162ff
Cr-Commit-Position: refs/heads/master@{#321912}

[Jamie, 2015-03-21 01:18:23]

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

[Jamie, 2015-03-21 01:18:23]

PTAL

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/app_remoting.js (left):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/app_remoting.js:95:
remoting.LoadingWindow.show();
Showing this dialog in init() is not correct, as we haven't started connecting
yet. It also means that we either have to close the loading window explicitly
when we show the auth window (which introduces an awkward dependency) or have
two dialogs on screen at the same time. The only situation in which there is an
appreciable lag between init() and start() is when the user needs to approve
permissions, in which case the identity API is showing its own UI. In all cases,
it's safe to defer showing the loading window until start().

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/ar_auth_dialog.js (right):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/ar_auth_dialog.js:43:
remoting.AuthDialog.getInstance = function() {
I would prefer that this getInstance() singleton is removed in favour of a
getAuthDialog() method on the delegate, but since I'm not sure what the future
of the delegate is, I've left it for now.

[garykac, 2015-03-23 23:25:33]

lgtm

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/app_remoting.js (left):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/app_remoting.js:95:
remoting.LoadingWindow.show();
On 2015/03/21 01:18:23, Jamie wrote:
> Showing this dialog in init() is not correct, as we haven't started connecting
> yet. It also means that we either have to close the loading window explicitly
> when we show the auth window (which introduces an awkward dependency) or have
> two dialogs on screen at the same time. The only situation in which there is
an
> appreciable lag between init() and start() is when the user needs to approve
> permissions, in which case the identity API is showing its own UI. In all
cases,
> it's safe to defer showing the loading window until start().

Sounds good.

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/ar_auth_dialog.js (right):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/ar_auth_dialog.js:43:
remoting.AuthDialog.getInstance = function() {
On 2015/03/21 01:18:23, Jamie wrote:
> I would prefer that this getInstance() singleton is removed in favour of a
> getAuthDialog() method on the delegate, but since I'm not sure what the future
> of the delegate is, I've left it for now.

Add a TODO?

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/crd/html/di...
File remoting/webapp/crd/html/dialog_auth.html (right):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/crd/html/di...
remoting/webapp/crd/html/dialog_auth.html:2: Copyright (c) 2014 The Chromium
Authors. All rights reserved.
For consistency, it seems like this should have a 'crd_' prefix since it's
DesktopRemoting specific.

[Jamie, 2015-03-23 23:44:53]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-03-23 23:45:20]

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

[commit-bot: I haz the power, 2015-03-24 00:37:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-24 00:38:14]

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

[Jamie, 2015-04-15 00:02:57]

Looks like I forgot to send these comment replies; sorry.

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/ar_auth_dialog.js (right):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/ar_auth_dialog.js:43:
remoting.AuthDialog.getInstance = function() {
On 2015/03/23 23:25:33, garykac wrote:
> On 2015/03/21 01:18:23, Jamie wrote:
> > I would prefer that this getInstance() singleton is removed in favour of a
> > getAuthDialog() method on the delegate, but since I'm not sure what the
future
> > of the delegate is, I've left it for now.
> 
> Add a TODO?

There's one in auth_init.js which I think applies.

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/crd/html/di...
File remoting/webapp/crd/html/dialog_auth.html (right):

https://codereview.chromium.org/1023943004/diff/1/remoting/webapp/crd/html/di...
remoting/webapp/crd/html/dialog_auth.html:2: Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/03/23 23:25:33, garykac wrote:
> For consistency, it seems like this should have a 'crd_' prefix since it's
> DesktopRemoting specific.

My understanding is that we use a crd_ prefix for cases where equivalent
functionality is implemented in both apps, to disambiguate. Since there is no
explicit auth dialog for LG (it's just a message window), it's not necessary
here. If it were, then every file under crd/ would need the prefix, which seems
like overkill.