Issue 7613009: Add support for Disconnect hot key in Windows.

Issue 7613009: Add support for Disconnect hot key in Windows. (Closed)

Created:
9 years, 4 months ago by garykac

Modified:
9 years, 4 months ago

Reviewers:
Wez, Jamie

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Add support for Disconnect hot key in Windows. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96574

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 13

Patch Set 6 : '' #

Total comments: 4

Patch Set 7 : '' #

Created: 9 years, 4 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+47 lines, -15 lines)			Patch
	M	remoting/host/disconnect_window_win.cc	View	1 2 3 4 5 6	7 chunks	+47 lines, -15 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Wez

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc#newcode28 remoting/host/disconnect_window_win.cc:28: const int DISCONNECT_HOTKEY_ID = 1000; Is there any significance ...

9 years, 4 months ago (2011-08-11 22:15:24 UTC) #2

garykac

9 years, 4 months ago (2011-08-11 23:04:48 UTC) #3

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:28: const int DISCONNECT_HOTKEY_ID =
1000;
On 2011/08/11 22:15:24, Wez wrote:
> Is there any significance to this choice of value?  If not then could we get
rid
> of this ID and just pass 0 to the API for the ID, given that we don't check
the
> ID in any case?

It must be between 0x0000 and 0xbfff.

Having a named constant seems far preferable (from a code-readability POV) to
passing a mysterious a '0' value to a function. Esp. since it needs to be
consistent for multiple calls.

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:134: hwnd_ = NULL;
On 2011/08/11 22:15:24, Wez wrote:
> These lines look almost identical to EndDialog() below; why not just allow it
to
> take a result parameter so it can be used here?

I was sortof trying to avoid having a |result| param in EndDialog since it's 0
everywhere except here.

But it's better to share code, so I changed it.

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:134: hwnd_ = NULL;
On 2011/08/11 22:15:24, Wez wrote:
> These lines look almost identical to EndDialog() below; why not just allow it
to
> take a result parameter so it can be used here?

I was sortof trying to avoid having a |result| param in EndDialog since it's 0
everywhere except here. But I was bothered by the multiple |EndDialog|s.

It's better to share code, so I changed it.

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:160: VK_ESCAPE);
On 2011/08/11 22:15:24, Wez wrote:
> I thought we were going to not display the hot-key test unless we actually
> successfully registered the hot-key?

Added SetDisconnectButtonText call here. Previously, this was relying on
WM_INITDIALOG to do it (which doesn't work).

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:182: void
DisconnectWindowWin::RemoveKeyHandler() {
On 2011/08/11 22:15:24, Wez wrote:
> nit: Does this really need to be a separate function?

*need*? No.  But it's called from multiple places and is convenient to have one
location that performs all the unregister tasks.

Wez

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc#newcode160 remoting/host/disconnect_window_win.cc:160: VK_ESCAPE); On 2011/08/11 23:04:49, garykac wrote: > On 2011/08/11 ...

9 years, 4 months ago (2011-08-11 23:58:29 UTC) #4

Jamie

http://codereview.chromium.org/7613009/diff/11002/remoting/host/disconnect_window_win.cc File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/7613009/diff/11002/remoting/host/disconnect_window_win.cc#newcode155 remoting/host/disconnect_window_win.cc:155: has_hotkey_ = true; Surely this depends on the return ...

9 years, 4 months ago (2011-08-12 00:00:33 UTC) #5

garykac

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_window_win.cc#newcode160 remoting/host/disconnect_window_win.cc:160: VK_ESCAPE); On 2011/08/11 23:58:30, Wez wrote: > On 2011/08/11 ...

9 years, 4 months ago (2011-08-12 00:23:15 UTC) #6

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:160: VK_ESCAPE);
On 2011/08/11 23:58:30, Wez wrote:
> On 2011/08/11 23:04:49, garykac wrote:
> > On 2011/08/11 22:15:24, Wez wrote:
> > > I thought we were going to not display the hot-key test unless we actually
> > > successfully registered the hot-key?
> > 
> > Added SetDisconnectButtonText call here. Previously, this was relying on
> > WM_INITDIALOG to do it (which doesn't work).
> 
> My point is not about when you set the button text, but the fact that since
> you're not checking the result of RegisterHotKey, you'll always display the
> hot-key text on the button, even if the hot-key wasn't successfully
registered.

Argh. I completely missed that point (which Jamie made as well). Done.

http://codereview.chromium.org/7613009/diff/9002/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:182: void
DisconnectWindowWin::RemoveKeyHandler() {
On 2011/08/11 23:58:30, Wez wrote:
> On 2011/08/11 23:04:49, garykac wrote:
> > On 2011/08/11 22:15:24, Wez wrote:
> > > nit: Does this really need to be a separate function?
> > 
> > *need*? No.  But it's called from multiple places and is convenient to have
> one
> > location that performs all the unregister tasks.
> 
> It's called from two places, both of which then do EndDialog() things, don't
> they?

Actually it's called in 1 place now that EndDialog has been merged.  So it's
gone.

http://codereview.chromium.org/7613009/diff/11002/remoting/host/disconnect_wi...
remoting/host/disconnect_window_win.cc:155: ShowWindow(hwnd_, SW_SHOW);
On 2011/08/12 00:00:33, Jamie wrote:
> Surely this depends on the return value of RegisterHotKey?

Surely I shouldn't be writing code today... :-(

http://codereview.chromium.org/7613009/diff/11002/remoting/host/disconnect_wi...
remoting/host/disconnect_window_win.cc:187: DisconnectWindow*
DisconnectWindow::Create() {
On 2011/08/12 00:00:33, Jamie wrote:
> Should be conditional on has_hotkey_

Done.

LGTM

Expand Messages | Collapse Messages