Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825251:
Reposition the Disconnect dialog when work area dimensions change. (Closed)
|
Created:
8 years, 4 months ago by Wez Modified:
8 years, 4 months ago Reviewers:
alexeypa (please no reviews) CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReposition the Disconnect dialog when work area dimensions change.
This prevents the Disconnect dialog from becoming hidden by the Windows task bar, or falling off-screen when the display resolution is made smaller.
BUG=129907, 129835
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151313
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address review comments. #
Total comments: 4
Patch Set 3 : Address review comment. #Messages
Total messages: 8 (0 generated)
PTAL
http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:117: return FALSE; Return TRUE if the message was processed, FALSE otherwise. See http://msdn.microsoft.com/en-us/library/windows/desktop/ms645469(v=vs.85).aspx. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:122: return FALSE; return TRUE; http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:138: HDC hdc = BeginPaint(hwnd_, &ps); BeginPaint can return NULL. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:140: GetClientRect(hwnd_, &rect); GetClientRect can fail. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:143: base::win::ScopedSelectObject brush(hdc, GetStockObject(NULL_BRUSH)); GetStockObject can fail. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:144: RoundRect(hdc, rect.left, rect.top, rect.right - 1, rect.bottom - 1, RoundRect can fail. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:148: return TRUE; The application should return TRUE it processes WM_PAINT. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); nit: Define a constant for the window name. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); All the API functions can fail. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); It would be nice if the window could be correctly positioned even if the taskbar is not present. Say we could use primary monitor boundaries if there is no taskbar.
http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:117: return FALSE; On 2012/08/08 22:05:08, alexeypa wrote: > Return TRUE if the message was processed, FALSE otherwise. See > http://msdn.microsoft.com/en-us/library/windows/desktop/ms645469%28v=vs.85%29.... We'd only do that if we wanted to suppress any additional processing that the dialog message handler might do; in this case I don't think it makes any difference, so TRUE is fine. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:117: return FALSE; On 2012/08/08 22:05:08, alexeypa wrote: > Return TRUE if the message was processed, FALSE otherwise. See > http://msdn.microsoft.com/en-us/library/windows/desktop/ms645469%28v=vs.85%29.... Done. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:122: return FALSE; On 2012/08/08 22:05:08, alexeypa wrote: > return TRUE; Same here. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:122: return FALSE; On 2012/08/08 22:05:08, alexeypa wrote: > return TRUE; Done. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:138: HDC hdc = BeginPaint(hwnd_, &ps); On 2012/08/08 22:05:08, alexeypa wrote: > BeginPaint can return NULL. Technically yes, but unless your system is horribly b0rked,it won't. All we can do if it fails is not try to render; if we continue and try to render but don't check the results of the API calls then the effect is the same... http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:148: return TRUE; On 2012/08/08 22:05:08, alexeypa wrote: > The application should return TRUE it processes WM_PAINT. Yes, it is doing. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); On 2012/08/08 22:05:08, alexeypa wrote: > It would be nice if the window could be correctly positioned even if the taskbar > is not present. Say we could use primary monitor boundaries if there is no > taskbar. That's what this code does; see the comment and parameter to MonitorFromWindow(). http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); On 2012/08/08 22:05:08, alexeypa wrote: > All the API functions can fail. If FindWindow() fails then MonitorFromWindow() will fail and default to the primary monitor. If MonitorFromWindow() fails then GetMonitorInfo() will fail and we'll not try to reposition the window. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:275: HWND taskbar = FindWindow(L"Shell_TrayWnd", NULL); On 2012/08/08 22:05:08, alexeypa wrote: > nit: Define a constant for the window name. Done.
http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:138: HDC hdc = BeginPaint(hwnd_, &ps); On 2012/08/09 21:58:08, Wez wrote: > if we continue and try to render but don't > check the results of the API calls then the effect is the same... The default assumption a person reading the code makes is that any error returned from an API should be handled to make sure that the code does not rely on uninitialized data. So either you need to explain each time why ignoring the result is OK (just like it is done below for FindWindow() and MonitorFromWindow()) or you need to check the error code. In this specific example: - GetClientRect can fail leaving garbage in |rect|, which later will passed back to RoundRect(). - EndPaint can be called with uninitialized |ps|. The rest of the errors should be OK, because NULLs are handled much better than uninitialized data. PS. Yes, Win32 API do pretty good job on validating the parameters but they are not perfect. http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:148: return TRUE; On 2012/08/09 21:58:08, Wez wrote: > On 2012/08/08 22:05:08, alexeypa wrote: > > The application should return TRUE it processes WM_PAINT. > > Yes, it is doing. :-) I'm not sure what my comment was doing there. http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... remoting/host/disconnect_window_win.cc:34: const WCHAR kShellTrayWindowName[] = L"Shell_TrayWnd"; nit: WCHAR -> wchar_t. L"" gives a wide string, while WCHAR is UTF-16.
http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:138: HDC hdc = BeginPaint(hwnd_, &ps); On 2012/08/10 17:28:01, alexeypa wrote: > On 2012/08/09 21:58:08, Wez wrote: > > if we continue and try to render but don't > > check the results of the API calls then the effect is the same... > > The default assumption a person reading the code makes is that any error > returned from an API should be handled to make sure that the code does not rely > on uninitialized data. So either you need to explain each time why ignoring the > result is OK (just like it is done below for FindWindow() and > MonitorFromWindow()) or you need to check the error code. > > In this specific example: > - GetClientRect can fail leaving garbage in |rect|, which later will passed back > to RoundRect(). > - EndPaint can be called with uninitialized |ps|. > > The rest of the errors should be OK, because NULLs are handled much better than > uninitialized data. > > PS. Yes, Win32 API do pretty good job on validating the parameters but they are > not perfect. > I think this specific code should be pretty safe, but I'll create a bug to clean this up; it doesn't belong in this CL. http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... remoting/host/disconnect_window_win.cc:34: const WCHAR kShellTrayWindowName[] = L"Shell_TrayWnd"; On 2012/08/10 17:28:02, alexeypa wrote: > nit: WCHAR -> wchar_t. L"" gives a wide string, while WCHAR is UTF-16. Which are the same thing on Windows. ;) http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... remoting/host/disconnect_window_win.cc:34: const WCHAR kShellTrayWindowName[] = L"Shell_TrayWnd"; On 2012/08/10 17:28:02, alexeypa wrote: > nit: WCHAR -> wchar_t. L"" gives a wide string, while WCHAR is UTF-16. Done.
lgtm http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1/remoting/host/disconnect_windo... remoting/host/disconnect_window_win.cc:138: HDC hdc = BeginPaint(hwnd_, &ps); On 2012/08/10 20:06:17, Wez wrote: > On 2012/08/10 17:28:01, alexeypa wrote: > > On 2012/08/09 21:58:08, Wez wrote: > > > if we continue and try to render but don't > > > check the results of the API calls then the effect is the same... > > > > The default assumption a person reading the code makes is that any error > > returned from an API should be handled to make sure that the code does not > rely > > on uninitialized data. So either you need to explain each time why ignoring > the > > result is OK (just like it is done below for FindWindow() and > > MonitorFromWindow()) or you need to check the error code. > > > > In this specific example: > > - GetClientRect can fail leaving garbage in |rect|, which later will passed > back > > to RoundRect(). > > - EndPaint can be called with uninitialized |ps|. > > > > The rest of the errors should be OK, because NULLs are handled much better > than > > uninitialized data. > > > > PS. Yes, Win32 API do pretty good job on validating the parameters but they > are > > not perfect. > > > > I think this specific code should be pretty safe, but I'll create a bug to clean > this up; it doesn't belong in this CL. OK, thanks! http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... File remoting/host/disconnect_window_win.cc (right): http://codereview.chromium.org/10825251/diff/1002/remoting/host/disconnect_wi... remoting/host/disconnect_window_win.cc:34: const WCHAR kShellTrayWindowName[] = L"Shell_TrayWnd"; On 2012/08/10 20:06:17, Wez wrote: > On 2012/08/10 17:28:02, alexeypa wrote: > > nit: WCHAR -> wchar_t. L"" gives a wide string, while WCHAR is UTF-16. > > Which are the same thing on Windows. ;) Yeah, I know. It is messy.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wez@chromium.org/10825251/1003
Change committed as 151313 |
