[AppRemoting] Implement ConnectionDroppedDialog.

[AppRemoting] Implement ConnectionDroppedDialog.

This CL implements a ConnectionDroppedDialog using HTML5 dialog element.
The dialog will be shown to the user whenever a connection is dropped.

BUG=486888
Committed: https://crrev.com/81d05c568abebd6712402afbc7bb309ff181a735
Cr-Commit-Position: refs/heads/master@{#330849}

[kelvinp, 2015-05-20 01:22:15]

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

[kelvinp, 2015-05-20 01:25:49]

Patchset #1 (id:1) has been deleted

[kelvinp, 2015-05-20 01:27:31]

PTAL

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/html/template_lg.html (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/html/template_lg.html:13: <link rel="stylesheet"
href="ar_dialog.css">
This css file should be here in the first place.
It is currently unreferenced by mistake.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:180: new base.DomEventHook(dialog,
'cancel', this.onCancel_.bind(this), false),
This is called when the user press Escape.

[Jamie, 2015-05-20 02:20:37]

Does this CL include the code to centre the dialog w.r.t. the remote window
bounding region?

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/html/ar_dialog.css (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/html/ar_dialog.css:12:
#connection-dropped-dialog.kd-modaldialog,
Is the lack of a space in this selector deliberate? It looks like an error since
it's followed immediately by two similar-looking rules that do have a space.
Maybe add a comment on this line, along the lines of:

   /* no space */

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_connected_view.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_connected_view.js:79: this.contextMenu_,
menuAdapter);
Is this change related to this CL?

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting_activity.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:197:
remoting.AppRemotingActivity.prototype.onConnectionDropped_ = function() {
I think there's value in passing the error into here. We should at least log it,
and maybe surface it in the dialog as well (perhaps behind a "more information"
expander).

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:203: var Result =
remoting.MessageDialog.Result;
I'm surprised that this assignment works as far as using it as a JSCompile type
is concerned. In any case, it doesn't seem to result in shorter code, so I would
just write out the type longhand in the two places you're using it.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:208: // Hide the
desktop window before tearing down the plugin.
This comment doesn't seem to be accurate.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:164: * A promise-based dialog
implementation using HTML Dialog element.
s/Dialog/<dialog>/

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:219: e.preventDefault();
I'm not sure that a generic Html5ModalDialog class should prevent the default
behaviour of the Escape key. In the specific case of the connection-dropped
dialog, it probably makes sense, because the user didn't take any explicit
action to pop up the dialog, but I think we want to start using this class for
all our dialogs, at which point it does make sense to handle Escape in a
standard way. Maybe make it configurable, either with a ctor parameter, or via a
setter method.

[kelvinp, 2015-05-20 18:13:00]

The centering logic is already checked-in in an earlier change
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/ba...

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/html/ar_dialog.css (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/html/ar_dialog.css:12:
#connection-dropped-dialog.kd-modaldialog,
On 2015/05/20 02:20:36, Jamie wrote:
> Is the lack of a space in this selector deliberate? It looks like an error
since
> it's followed immediately by two similar-looking rules that do have a space.
> Maybe add a comment on this line, along the lines of:
> 
>    /* no space */

Yes, it is deliberate.  #foo.bar selects the element with both the id foo and
the class bar, while #foo .bar selects the element with class bar which is a
descendant of foo.
The class name .kd-modaldialog, is not necessary in the selector.  I include
here to show that we are overriding styles.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_connected_view.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_connected_view.js:79: this.contextMenu_,
menuAdapter);
On 2015/05/20 02:20:36, Jamie wrote:
> Is this change related to this CL?

Because, this change allow us to restart the AppRemotingActivity., each
component must be able to shutdown cleanly.  We add the menuAdaptor to the list
of disposables so that it can clean up its context menu, or else ChromeOS will
complain double registration of the same menu item.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting_activity.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:197:
remoting.AppRemotingActivity.prototype.onConnectionDropped_ = function() {
On 2015/05/20 02:20:37, Jamie wrote:
> I think there's value in passing the error into here. We should at least log
it,
> and maybe surface it in the dialog as well (perhaps behind a "more
information"
> expander).

The error is already logged in the earlier stack (in clientSession.js)  As
discussed, leaving the dialog as is for now.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:203: var Result =
remoting.MessageDialog.Result;
On 2015/05/20 02:20:37, Jamie wrote:
> I'm surprised that this assignment works as far as using it as a JSCompile
type
> is concerned. In any case, it doesn't seem to result in shorter code, so I
would
> just write out the type longhand in the two places you're using it.

Done.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting_activity.js:208: // Hide the
desktop window before tearing down the plugin.
On 2015/05/20 02:20:37, Jamie wrote:
> This comment doesn't seem to be accurate.

Maybe I need to phrase it better.  By calling setDesktopRect to an empty array,
we are telling the windowshape to not render any desktop window of the remote
application.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:164: * A promise-based dialog
implementation using HTML Dialog element.
On 2015/05/20 02:20:37, Jamie wrote:
> s/Dialog/<dialog>/

Done.

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:219: e.preventDefault();
On 2015/05/20 02:20:37, Jamie wrote:
> I'm not sure that a generic Html5ModalDialog class should prevent the default
> behaviour of the Escape key. In the specific case of the connection-dropped
> dialog, it probably makes sense, because the user didn't take any explicit
> action to pop up the dialog, but I think we want to start using this class for
> all our dialogs, at which point it does make sense to handle Escape in a
> standard way. Maybe make it configurable, either with a ctor parameter, or via
a
> setter method.

Done.

[Jamie, 2015-05-20 19:55:09]

lgtm

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/html/ar_dialog.css (right):

https://codereview.chromium.org/1146833002/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/html/ar_dialog.css:12:
#connection-dropped-dialog.kd-modaldialog,
On 2015/05/20 18:12:59, kelvinp wrote:
> On 2015/05/20 02:20:36, Jamie wrote:
> > Is the lack of a space in this selector deliberate? It looks like an error
> since
> > it's followed immediately by two similar-looking rules that do have a space.
> > Maybe add a comment on this line, along the lines of:
> > 
> >    /* no space */
> 
> Yes, it is deliberate.  #foo.bar selects the element with both the id foo and
> the class bar, while #foo .bar selects the element with class bar which is a
> descendant of foo.
> The class name .kd-modaldialog, is not necessary in the selector.  I include
> here to show that we are overriding styles. 

Yes, I'm aware of the difference :) My point was just that at first glace it
looks like a typo. After all, why would we have different DOM structures for
three very similar dialogs? (AIUI, it's because the new one uses <dialog>,
right?) A comment explaining that the lack of space is deliberate would be
useful in the same way that a comment is useful if there's a delibrate missing
break statement in a switch. It doesn't need to go into detail about why it's
not needed, just to indicate that it's not a typo.

[kelvinp, 2015-05-20 21:51:11]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-05-20 21:51:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1146833002/#ps60001
(title: "WindowShape fixes + reviewer's feedback")

[commit-bot: I haz the power, 2015-05-20 21:52:01]

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

[commit-bot: I haz the power, 2015-05-20 23:46:52]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-20 23:51:37]

Patchset 3 (id:??) landed as
https://crrev.com/81d05c568abebd6712402afbc7bb309ff181a735
Cr-Commit-Position: refs/heads/master@{#330849}