Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10071025:
[Chromoting] Make the Windows host controller ask the user to confirm host registration. (Closed)
|
Created:
8 years, 8 months ago by simonmorris Modified:
8 years, 8 months ago 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. |
Description[Chromoting] Make the Windows host controller ask the user to confirm host registration.
BUG=121749
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132240
Patch Set 1 #
Total comments: 38
Patch Set 2 : Review. #
Total comments: 2
Patch Set 3 : Add a comment. #Patch Set 4 : Remove global variable. #
Total comments: 8
Patch Set 5 : Minor improvements to interfaces and comments. #
Total comments: 1
Patch Set 6 : Add license boilerplate. #
Messages
Total messages: 18 (0 generated)
ptal
FYI http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:16: 100 "Chrome Remote Desktop Host Controller" Since you have introduced elevated_controller_resource.h you can define an IDS_XXX constant for this "100" http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST This is usually a bad idea to make a modal dialog a top-most window. Can we avoid that? http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_module_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; nit: g_hInstance -> g_instance http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; I believe you should be able to use _AtlModule.GetModuleInstance() since it is an ATL-based applications. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:33: int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE, LPSTR, int command) { nit: hInstance -> instance http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:106: scoped_ptr<base::Value> configValue(base::JSONReader::Read(content)); configValue -> config_value http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:110: base::DictionaryValue* configDict = NULL; configDict -> config_dict. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:122: remoting::VerifyConfigWindowWin verifyWin(email, host_id, host_secret_hash); verifyWin -> verify_win http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:124: return E_FAIL; HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) _maybe_ a better choice, but I'm really not sure. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:26: VerifyConfigWindowWin::VerifyConfigWindowWin(const std::string& email, Broken indentation http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:46: if (msg == WM_INITDIALOG) { Using dialog classes from ATL should help you to get rid of this. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:62: case WM_INITDIALOG: It can be much more readable ATL message handles map. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:85: SetWindowText(hwnd_, L"Chrome Remote Desktop"); Why are we doing this? Shouldn't this come from the resources? If the goal is to make it i18n-able that it is not going to work. Translated strings are longer, and they will not fit a fixed-size dialog. MUI should really be used to do i18n. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:118: scoped_array<WCHAR> pinWSTR(new WCHAR[kMaxPinLength]); ATL has a string class which will be more convenient. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... File remoting/host/verify_config_window_win.h (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.h:10: #include <string> Shouldn't this go before "base/callback.h"? http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.h:14: class VerifyConfigWindowWin { We use ATL already. It really make sense to use CDialog from ATL here.
http://codereview.chromium.org/10071025/diff/5001/remoting/host/verify_config... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/5001/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:125: // Refactor. See PinIsValid in lambroslambrou@'s CL 10008092.
http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:16: 100 "Chrome Remote Desktop Host Controller" On 2012/04/12 21:20:41, alexeypa wrote: > Since you have introduced elevated_controller_resource.h you can define an > IDS_XXX constant for this "100" Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST On 2012/04/12 21:20:41, alexeypa wrote: > This is usually a bad idea to make a modal dialog a top-most window. Can we > avoid that? Without that style, the dialog may be hidden behind the browser. Is there a way to open the dialog at the top of z-order, without making it be kept there? http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_module_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; On 2012/04/12 21:20:41, alexeypa wrote: > nit: g_hInstance -> g_instance Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; On 2012/04/12 21:20:41, alexeypa wrote: > I believe you should be able to use _AtlModule.GetModuleInstance() since it is > an ATL-based applications. This way is consistent with host_plugin.cc. Is there a benefit to doing it as you suggest? http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:33: int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE, LPSTR, int command) { On 2012/04/12 21:20:41, alexeypa wrote: > nit: hInstance -> instance Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:106: scoped_ptr<base::Value> configValue(base::JSONReader::Read(content)); On 2012/04/12 21:20:41, alexeypa wrote: > configValue -> config_value Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:110: base::DictionaryValue* configDict = NULL; On 2012/04/12 21:20:41, alexeypa wrote: > configDict -> config_dict. Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:122: remoting::VerifyConfigWindowWin verifyWin(email, host_id, host_secret_hash); On 2012/04/12 21:20:41, alexeypa wrote: > verifyWin -> verify_win Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_win.cc:124: return E_FAIL; On 2012/04/12 21:20:41, alexeypa wrote: > HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) _maybe_ a better choice, but I'm really > not sure. This way is consistent with the existing code, which seems likely to make things simpler for the consumer of the return code. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:26: VerifyConfigWindowWin::VerifyConfigWindowWin(const std::string& email, On 2012/04/12 21:20:41, alexeypa wrote: > Broken indentation Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:46: if (msg == WM_INITDIALOG) { On 2012/04/12 21:20:41, alexeypa wrote: > Using dialog classes from ATL should help you to get rid of this. I've added a TODO to the header file. Given the time constraints, I don't think there's a need to rewrite this code now. Also, it's consistent with continue_window_win and disconnect_window_win. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:62: case WM_INITDIALOG: On 2012/04/12 21:20:41, alexeypa wrote: > It can be much more readable ATL message handles map. Added a TODO. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:85: SetWindowText(hwnd_, L"Chrome Remote Desktop"); On 2012/04/12 21:20:41, alexeypa wrote: > Why are we doing this? Shouldn't this come from the resources? If the goal is > to make it i18n-able that it is not going to work. Translated strings are > longer, and they will not fit a fixed-size dialog. MUI should really be used to > do i18n. The string code here is only intended to be good enough for now. Existing code (continue_window_win and disconnect_window_win) takes l10n strings from the host, so that's the approach I suggest as a follow-up. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.cc:118: scoped_array<WCHAR> pinWSTR(new WCHAR[kMaxPinLength]); On 2012/04/12 21:20:41, alexeypa wrote: > ATL has a string class which will be more convenient. Added a TODO. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... File remoting/host/verify_config_window_win.h (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.h:10: #include <string> On 2012/04/12 21:20:41, alexeypa wrote: > Shouldn't this go before "base/callback.h"? Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/verify_config_wi... remoting/host/verify_config_window_win.h:14: class VerifyConfigWindowWin { On 2012/04/12 21:20:41, alexeypa wrote: > We use ATL already. It really make sense to use CDialog from ATL here. Added a TODO.
http://codereview.chromium.org/10071025/diff/5001/remoting/host/verify_config... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/5001/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:125: // Refactor. On 2012/04/12 22:06:28, Wez wrote: > See PinIsValid in lambroslambrou@'s CL 10008092. Duly noted.
http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST On 2012/04/12 22:11:44, simonmorris wrote: > On 2012/04/12 21:20:41, alexeypa wrote: > > This is usually a bad idea to make a modal dialog a top-most window. Can we > > avoid that? > > Without that style, the dialog may be hidden behind the browser. Is there a way > to open the dialog at the top of z-order, without making it be kept there? As per our discussion. It is up to the user to decide which window should be on top. Please add TODO that we need to pass HWND of the windows to be used as parent of the dialog. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_module_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; On 2012/04/12 22:11:44, simonmorris wrote: > On 2012/04/12 21:20:41, alexeypa wrote: > > I believe you should be able to use _AtlModule.GetModuleInstance() since it is > > an ATL-based applications. > > This way is consistent with host_plugin.cc. Is there a benefit to doing it as > you suggest? This way you don't pollute the global namespace and it is consistent with other ATL code (since this is an ATL application)
http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST On 2012/04/12 23:10:33, alexeypa wrote: > On 2012/04/12 22:11:44, simonmorris wrote: > > On 2012/04/12 21:20:41, alexeypa wrote: > > > This is usually a bad idea to make a modal dialog a top-most window. Can we > > > avoid that? > > > > Without that style, the dialog may be hidden behind the browser. Is there a > way > > to open the dialog at the top of z-order, without making it be kept there? > > As per our discussion. It is up to the user to decide which window should be on > top. > Yes, but if the user doesn't realise that there is a new dialog window, that choice isn't very useful. > Please add TODO that we need to pass HWND of the windows to be used as parent of > the dialog. Done. http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller_module_win.cc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = NULL; On 2012/04/12 23:10:33, alexeypa wrote: > On 2012/04/12 22:11:44, simonmorris wrote: > > On 2012/04/12 21:20:41, alexeypa wrote: > > > I believe you should be able to use _AtlModule.GetModuleInstance() since it > is > > > an ATL-based applications. > > > > This way is consistent with host_plugin.cc. Is there a benefit to doing it as > > you suggest? > > This way you don't pollute the global namespace and it is consistent with other > ATL code (since this is an ATL application) Done.
On 2012/04/13 00:11:50, simonmorris wrote: > http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... > File remoting/host/elevated_controller.rc (right): > > http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... > remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST > On 2012/04/12 23:10:33, alexeypa wrote: > > On 2012/04/12 22:11:44, simonmorris wrote: > > > On 2012/04/12 21:20:41, alexeypa wrote: > > > > This is usually a bad idea to make a modal dialog a top-most window. Can > we > > > > avoid that? > > > > > > Without that style, the dialog may be hidden behind the browser. Is there a > > way > > > to open the dialog at the top of z-order, without making it be kept there? > > > > As per our discussion. It is up to the user to decide which window should be > on > > top. > > > > Yes, but if the user doesn't realise that there is a new dialog window, that > choice isn't very useful. > > > Please add TODO that we need to pass HWND of the windows to be used as parent > of > > the dialog. > > Done. > > http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... > File remoting/host/elevated_controller_module_win.cc (right): > > http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... > remoting/host/elevated_controller_module_win.cc:31: HINSTANCE g_hInstance = > NULL; > On 2012/04/12 23:10:33, alexeypa wrote: > > On 2012/04/12 22:11:44, simonmorris wrote: > > > On 2012/04/12 21:20:41, alexeypa wrote: > > > > I believe you should be able to use _AtlModule.GetModuleInstance() since > it > > is > > > > an ATL-based applications. > > > > > > This way is consistent with host_plugin.cc. Is there a benefit to doing it > as > > > you suggest? > > > > This way you don't pollute the global namespace and it is consistent with > other > > ATL code (since this is an ATL application) > > Done. ping
lgtm with a couple of nits, assuming Alexey and Wez don't have any outstanding concerns. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:19: // create a class with the shared code. Is this comment still true? http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:85: // TODO(simonmorris): Get these strings from the host, for l10n. We'll need to do something for l10n, but getting them from the host doesn't feel right, since that doesn't need to be localized. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:133: LOG(FATAL) << "Base64Encode failed"; I think a "return false" here would help readability, even if it's not strictly necessary. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... File remoting/host/verify_config_window_win.h (right): http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.h:23: int Run(); Nit: Why not bool?
http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:19: // create a class with the shared code. On 2012/04/13 16:40:03, Jamie wrote: > Is this comment still true? No: the aim is now to use ATL instead. Done. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:85: // TODO(simonmorris): Get these strings from the host, for l10n. On 2012/04/13 16:40:03, Jamie wrote: > We'll need to do something for l10n, but getting them from the host doesn't feel > right, since that doesn't need to be localized. Done. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.cc:133: LOG(FATAL) << "Base64Encode failed"; On 2012/04/13 16:40:03, Jamie wrote: > I think a "return false" here would help readability, even if it's not strictly > necessary. Done. http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... File remoting/host/verify_config_window_win.h (right): http://codereview.chromium.org/10071025/diff/4004/remoting/host/verify_config... remoting/host/verify_config_window_win.h:23: int Run(); On 2012/04/13 16:40:03, Jamie wrote: > Nit: Why not bool? DialogBoxParam returns an int, so it's more flexible to return the same int here. But that's not useful now. Done.
LGTM but I *really* don't like the idea of using WS_EX_TOPMOST. It is a bad user experience that we are making. Please file a bug saying that it should be removed (or addressed in a user-friendly way). http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST On 2012/04/13 00:11:50, simonmorris wrote: > Yes, but if the user doesn't realise that there is a new dialog window, that > choice isn't very useful. The user doesn't need to realize that if he is working with a different window at the moment. The new dialog should have a button in the task bar that will be flashing to draw user's attention.
On 2012/04/13 17:34:29, alexeypa wrote: > LGTM but > > I *really* don't like the idea of using WS_EX_TOPMOST. It is a bad user > experience that we are making. Please file a bug saying that it should be > removed (or addressed in a user-friendly way). > crbug.com/123342
http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... File remoting/host/elevated_controller.rc (right): http://codereview.chromium.org/10071025/diff/1/remoting/host/elevated_control... remoting/host/elevated_controller.rc:31: EXSTYLE WS_EX_TOPMOST On 2012/04/13 17:34:29, alexeypa wrote: > On 2012/04/13 00:11:50, simonmorris wrote: > > Yes, but if the user doesn't realise that there is a new dialog window, that > > choice isn't very useful. > > The user doesn't need to realize that if he is working with a different window > at the moment. The new dialog should have a button in the task bar that will be > flashing to draw user's attention. The button doesn't flash when I test.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10071025/14001
Presubmit check for 10071025-14001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: remoting/host/elevated_controller_resource.h Presubmit checks took 1.2s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10071025/14010
Change committed as 132240
http://codereview.chromium.org/10071025/diff/14001/remoting/host/verify_confi... File remoting/host/verify_config_window_win.cc (right): http://codereview.chromium.org/10071025/diff/14001/remoting/host/verify_confi... remoting/host/verify_config_window_win.cc:122: // Refactor to use PinIsValid(), from CL 10008092. If you pass in the DictionaryValue containing the config then you can use the code from that CL already. :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
