Chromoting: the Me2me host now presents a notification on the console allowing a user to disconnect…

Chromoting: the Me2me host (on Mac and Windows) now presents a notification on the console allowing a user to disconnect an incoming session.

BUG=127321, 127322
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=136698

[alexeypa (please no reviews), 2012-05-11 19:07:24]

Please take a look.

dcaiafa@ -> Mac specifics
jamiewalch@ & simonmorris@ -> the rest.

[Jamie, 2012-05-11 19:51:39]

lgtm, but this was tricky to review, as it felt like a mix of refactoring and
implementation. Where possible, these should be kept as separate CLs.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/continue...
File remoting/host/continue_window_win.cc (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/continue...
remoting/host/continue_window_win.cc:128: ::DestroyWindow(hwnd_);
What's the effect of this change?

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
File remoting/host/disconnect_window_mac.h (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.h:13: typedef
remoting::DisconnectWindow::DisconnectCallback DisconnectCallback;
Is this just to save typing, or is it required? If the latter, then it probably
warrants a comment.

[alexeypa (please no reviews), 2012-05-11 19:52:59]

Lambros, could you please try if this change breaks Mac? 

http://codereview.chromium.org/download/issue10384127_1.diff is based on
r136616.

[simonmorris, 2012-05-11 19:58:40]

My useful comments would be a subset of jamiewalch@'s, so lgtm.

[alexeypa (please no reviews), 2012-05-11 20:01:09]

On 2012/05/11 19:51:39, Jamie wrote:
> lgtm, but this was tricky to review, as it felt like a mix of refactoring and
> implementation. Where possible, these should be kept as separate CLs.

This is true, but breaking it into separate CLs is problematic because of two
factors:

1. It will take time to submit them.
2. Merging the fix to the release branch will be harder.

[alexeypa (please no reviews), 2012-05-11 20:01:21]

FYI.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/continue...
File remoting/host/continue_window_win.cc (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/continue...
remoting/host/continue_window_win.cc:128: ::DestroyWindow(hwnd_);
On 2012/05/11 19:51:39, Jamie wrote:
> What's the effect of this change?

EndDialog() does not have effect on non-modal dialogs. They should be closed
just like any other windows. We were leaking window handles, basically.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
File remoting/host/disconnect_window_mac.h (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.h:13: typedef
remoting::DisconnectWindow::DisconnectCallback DisconnectCallback;
On 2012/05/11 19:51:39, Jamie wrote:
> Is this just to save typing, or is it required? If the latter, then it
probably
> warrants a comment.

That I don't know. :-) I was mimicking the existing code for the Continue window
on Mac.

[dcaiafa, 2012-05-11 20:10:38]

lgtm, with comments.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
File remoting/host/disconnect_window_mac.mm (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:68: @property (nonatomic, assign)
DisconnectCallback disconnect_callback;
This doesn't need to be a property. It's private and doesn't require special
actions on access. It looks like this is true for host and username as well, but
this can be fixed in a different CL.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:78: @synthesize disconnect_callback =
disconnect_callback_;
Again, not needed.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:99: if
(!self.disconnect_callback.is_null()) {
To access the field directly, just drop the 'self.'. E.g.:
if (disconnect_callback_.is_null()) ...

[alexeypa (please no reviews), 2012-05-11 20:19:29]

FYI.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
File remoting/host/disconnect_window_mac.mm (right):

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:68: @property (nonatomic, assign)
DisconnectCallback disconnect_callback;
On 2012/05/11 20:10:38, dcaiafa wrote:
> This doesn't need to be a property. It's private and doesn't require special
> actions on access. It looks like this is true for host and username as well,
but
> this can be fixed in a different CL. 

Removed.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:78: @synthesize disconnect_callback =
disconnect_callback_;
On 2012/05/11 20:10:38, dcaiafa wrote:
> Again, not needed.

Done.

https://chromiumcodereview.appspot.com/10384127/diff/1/remoting/host/disconne...
remoting/host/disconnect_window_mac.mm:99: if
(!self.disconnect_callback.is_null()) {
On 2012/05/11 20:10:38, dcaiafa wrote:
> To access the field directly, just drop the 'self.'. E.g.:
> if (disconnect_callback_.is_null()) ...

Done.