Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11090063:
[Chromoting] Add a simple Windows app that registers and starts a host. (Closed)
|
Created:
8 years, 2 months ago by simonmorris Modified:
8 years, 2 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, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Add a simple Windows app that registers and starts a host.
BUG=153453
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162600
Patch Set 1 #
Total comments: 77
Patch Set 2 : Review. #
Total comments: 22
Patch Set 3 : Review. #
Total comments: 14
Patch Set 4 : Review. #
Total comments: 11
Patch Set 5 : Review. #
Total comments: 4
Patch Set 6 : Review. #Patch Set 7 : Sync. #Patch Set 8 : Fix non-standard resource header file. #Messages
Total messages: 23 (0 generated)
ptal
http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... File remoting/host/setup/pin_validator_unittest.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... remoting/host/setup/pin_validator_unittest.cc:29: EXPECT_EQ(false, IsPinValid(":123456")); nit: also try letters, e.g. "a123123" http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:41: message_loop.Run(); base::RunLoop is the preferred way to run MessageLoops now. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.rc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 These strings need to be localizeable. It might be better to create controls manually, without dialog resources http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:32: IDS_TITLE, "Chrome Remote Desktop Host Configurer" These will need to me moved to string_resources.grd - I've just checked it in: crrev.com/161247 Also opened crbug.com/155204 to track localization on windows. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_resource.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_resource.h:1: #define IDD_MAIN 101 We shouldn't need this file if you move resources to string_resources.grd
http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... File remoting/host/setup/pin_validator.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... remoting/host/setup/pin_validator.cc:15: if (c < '0' || c > '9') { This code is not i18n-friendly. Should we treat |pin| as UTF-8 string? If yes, we, probably, should classify characters using Unicode character classes. BTW: isdigit() is still better even if we go ahead with ASCII pins. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:19: CommandLine::Init(0, NULL); Add a comment explaining why it is acceptable to pass (0, NULL) on Windows. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:25: // Provide message loops and threads for the URLRequestContextGetter. Take a look at AutoThread and AutoThreadTaskRunner that wez@ have checked in recently. I believe it is a better way to manage threads. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:31: net::URLRequestContextGetter* url_request_context_getter_ = USe scoped_refptr pointer. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:40: OleInitialize(NULL); You might want to move this call before initialization of |host_configurer_window| because it might be issuing some COM calls. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:41: message_loop.Run(); Check out remoting/host/win/elevcated_controller_module.cc. ATL assumes in many places that you have _AtlModule defined. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.rc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 On 2012/10/11 01:15:24, sergeyu wrote: > These strings need to be localizeable. It might be better to create controls > manually, without dialog resources If we are going to do localization in Windows way, we should use dialog resources. We might need to adjust dialog layout to accommodate longer strings. We will have to use MUI resources or local resources manually then. Alternatively the dialog layout should be able to fit the longest translated strings. Then we will be able to get away with a single copy of resources and patching strings in runtime. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 nit: indentation. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:32: IDS_TITLE, "Chrome Remote Desktop Host Configurer" On 2012/10/11 01:15:24, sergeyu wrote: > These will need to me moved to string_resources.grd - I've just checked it in: > crrev.com/161247 > Also opened crbug.com/155204 to track localization on windows. Let's talk offline about this. There are some Windows-specific stuff I wanted to go though. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:19: int result = ::LoadString(NULL, id, str.get(), kMaxStringSize); According to MSDN if you pass 0 as the size of the buffer, LoadString will return a read-only pointer (to the passed buffer). You can use this feature to avoid copying the string to the buffer. You will not need to allocate a large buffer on the stack as well. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:20: str[std::min(result, kMaxStringSize)] = 0; LoadString NULL terminates the string during truncation, according to MSDN. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:21: return string16(str.get()); nit: This will scan the string again to find '\0'. Use the constructor accepting a pointer and the number of characters. You will not need line #20 then too. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:34: ExitProcess(0); ExitProcess is a bad way to exit a process having multiple threads running. To make it safe you need to make sure that all but main thread are exited. This means that you have to implement controlled shutdown and then you just return from main as usual. TerminalProcess is more reliable for exiting is the middle of a process. By using TerminateProcess you also agree that you are intentionally don't allow any shutdown. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:42: SetWindowText(LoadString(IDS_TITLE).c_str()); Since you are using ATL, CWindow class provides nice wrappers for SetWindowText, LoadString, EnableWindow, etc. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:43: ::EnableWindow(GetDlgItem(IDC_CHANGE_PIN), FALSE); nit you don't need to force the global scope here. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:93: LONG width = rect.right - rect.left; nit: Does this math work on 1024x768 resolution? http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:13: #include <atluser.h> nit: swap atlwin and atluser http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:41: // Sets an appropriate initial position for the dialog. nit: Separate the message handlers from the following code by a blank line. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; scoped_reptf? Please add a quick comment what this member is for. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:18: string16 LoadString(int id) { An identcal function is defined in host_configurer_window.cc http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:63: MessageBox(LoadString(IDS_SAME_PIN).c_str(), LoadString(IDS_TITLE).c_str(), MessageBox disrupts the UI flow. Is there a way to avoid it? Let's talk offline. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:68: MessageBox(LoadString(IDS_INVALID_PIN).c_str(), Looking at the way LoadString is used, it's ATL counterpart can be used as well without annoying c_str(). http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:75: base::Bind(&StartHostWindow::OnAuthCode, base::Unretained(this))); Now do you make sure that |auth_code_getter_| will not invoke StartHostWindow::OnAuthCode after StartHostWindow has been destroyed? http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:95: std::string StartHostWindow::GetDlgItemString(int id) { I'm pretty sure an ATL equivalent of this function exists. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:12: #include <atlwin.h> nit: swap atlwin and atluser http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:24: class StartHostWindow : public ATL::CDialogImpl<StartHostWindow> { nit: Add and explanation what this class is for. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:36: StartHostWindow(net::URLRequestContextGetter* url_request_context_getter); nit: scoped_refptr? http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:48: remoting::AuthCodeGetter auth_code_getter_; nit: sort the variables alphabetically. Add comments explaining what they are for. http://codereview.chromium.org/11090063/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/11090063/diff/1/remoting/remoting.gyp#newcode551 remoting/remoting.gyp:551: "\"/manifestdependency:type='win32' " InitCommonControlsEx() has to be called to make this work.
http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... File remoting/host/setup/pin_validator.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... remoting/host/setup/pin_validator.cc:15: if (c < '0' || c > '9') { On 2012/10/11 16:57:52, alexeypa wrote: > This code is not i18n-friendly. Should we treat |pin| as UTF-8 string? If yes, > we, probably, should classify characters using Unicode character classes. > > BTW: isdigit() is still better even if we go ahead with ASCII pins. I don't want users to enter anything but plain old digits, because of the scope for confusion. So I regard any character outside the range '0' to '9' as invalid. isdigit() has a locale-dependent definition, if <locale> is included, which doesn't seem appropriate here. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... File remoting/host/setup/pin_validator_unittest.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/pin_valida... remoting/host/setup/pin_validator_unittest.cc:29: EXPECT_EQ(false, IsPinValid(":123456")); On 2012/10/11 01:15:24, sergeyu wrote: > nit: also try letters, e.g. "a123123" Done (though the invalid characters I used are adjacent to the valid range). http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:19: CommandLine::Init(0, NULL); On 2012/10/11 16:57:52, alexeypa wrote: > Add a comment explaining why it is acceptable to pass (0, NULL) on Windows. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:25: // Provide message loops and threads for the URLRequestContextGetter. On 2012/10/11 16:57:52, alexeypa wrote: > Take a look at AutoThread and AutoThreadTaskRunner that wez@ have checked in > recently. I believe it is a better way to manage threads. I can see they can be useful classes. But I think they're overkill for this situation. Here, there are only two threads, whose lifetimes are very clear. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:31: net::URLRequestContextGetter* url_request_context_getter_ = On 2012/10/11 16:57:52, alexeypa wrote: > USe scoped_refptr pointer. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:40: OleInitialize(NULL); On 2012/10/11 16:57:52, alexeypa wrote: > You might want to move this call before initialization of > |host_configurer_window| because it might be issuing some COM calls. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:41: message_loop.Run(); On 2012/10/11 01:15:24, sergeyu wrote: > base::RunLoop is the preferred way to run MessageLoops now. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:41: message_loop.Run(); On 2012/10/11 01:15:24, sergeyu wrote: > base::RunLoop is the preferred way to run MessageLoops now. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.cc:41: message_loop.Run(); On 2012/10/11 16:57:52, alexeypa wrote: > Check out remoting/host/win/elevcated_controller_module.cc. ATL assumes in many > places that you have _AtlModule defined. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.rc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 On 2012/10/11 01:15:24, sergeyu wrote: > These strings need to be localizeable. It might be better to create controls > manually, without dialog resources This can be addressed in a separate CL, when we've decided how to tackle l10n. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 On 2012/10/11 16:57:52, alexeypa wrote: > On 2012/10/11 01:15:24, sergeyu wrote: > > These strings need to be localizeable. It might be better to create controls > > manually, without dialog resources > > If we are going to do localization in Windows way, we should use dialog > resources. We might need to adjust dialog layout to accommodate longer strings. > We will have to use MUI resources or local resources manually then. > > Alternatively the dialog layout should be able to fit the longest translated > strings. Then we will be able to get away with a single copy of resources and > patching strings in runtime. This can be addressed in a separate CL, when we've decided how to tackle l10n. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 On 2012/10/11 16:57:52, alexeypa wrote: > nit: indentation. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:32: IDS_TITLE, "Chrome Remote Desktop Host Configurer" On 2012/10/11 01:15:24, sergeyu wrote: > These will need to me moved to string_resources.grd - I've just checked it in: > crrev.com/161247 > Also opened crbug.com/155204 to track localization on windows. This can be addressed in a separate CL, when we've decided how to tackle l10n. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:32: IDS_TITLE, "Chrome Remote Desktop Host Configurer" On 2012/10/11 16:57:52, alexeypa wrote: > On 2012/10/11 01:15:24, sergeyu wrote: > > These will need to me moved to string_resources.grd - I've just checked it in: > > crrev.com/161247 > > Also opened crbug.com/155204 to track localization on windows. > > Let's talk offline about this. There are some Windows-specific stuff I wanted to > go though. This can be addressed in a separate CL, when we've decided how to tackle l10n. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_resource.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_resource.h:1: #define IDD_MAIN 101 On 2012/10/11 01:15:24, sergeyu wrote: > We shouldn't need this file if you move resources to string_resources.grd This can be addressed in a separate CL, when we've decided how to tackle l10n. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:19: int result = ::LoadString(NULL, id, str.get(), kMaxStringSize); On 2012/10/11 16:57:52, alexeypa wrote: > According to MSDN if you pass 0 as the size of the buffer, LoadString will > return a read-only pointer (to the passed buffer). You can use this feature to > avoid copying the string to the buffer. You will not need to allocate a large > buffer on the stack as well. Removed. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:20: str[std::min(result, kMaxStringSize)] = 0; On 2012/10/11 16:57:52, alexeypa wrote: > LoadString NULL terminates the string during truncation, according to MSDN. Removed. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:21: return string16(str.get()); On 2012/10/11 16:57:52, alexeypa wrote: > nit: This will scan the string again to find '\0'. Use the constructor accepting > a pointer and the number of characters. You will not need line #20 then too. Removed. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:34: ExitProcess(0); On 2012/10/11 16:57:52, alexeypa wrote: > ExitProcess is a bad way to exit a process having multiple threads running. To > make it safe you need to make sure that all but main thread are exited. This > means that you have to implement controlled shutdown and then you just return > from main as usual. > > TerminalProcess is more reliable for exiting is the middle of a process. By > using TerminateProcess you also agree that you are intentionally don't allow any > shutdown. > EndDialog works, and seems more appropriate. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:42: SetWindowText(LoadString(IDS_TITLE).c_str()); On 2012/10/11 16:57:52, alexeypa wrote: > Since you are using ATL, CWindow class provides nice wrappers for SetWindowText, > LoadString, EnableWindow, etc. I didn't find anything that could simplify the line above this comment. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:43: ::EnableWindow(GetDlgItem(IDC_CHANGE_PIN), FALSE); On 2012/10/11 16:57:52, alexeypa wrote: > nit you don't need to force the global scope here. I do, because of the nice wrapper for EnableWindow that CWindow provides. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.cc:93: LONG width = rect.right - rect.left; On 2012/10/11 16:57:52, alexeypa wrote: > nit: Does this math work on 1024x768 resolution? Yes. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:13: #include <atluser.h> On 2012/10/11 16:57:52, alexeypa wrote: > nit: swap atlwin and atluser Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:41: // Sets an appropriate initial position for the dialog. On 2012/10/11 16:57:52, alexeypa wrote: > nit: Separate the message handlers from the following code by a blank line. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; On 2012/10/11 16:57:52, alexeypa wrote: > scoped_reptf? Please add a quick comment what this member is for. I don't think scoped_refptr adds anything here. The context getter has to outlive this object, and it will do so. I've added an explanatory comment. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:18: string16 LoadString(int id) { On 2012/10/11 16:57:52, alexeypa wrote: > An identcal function is defined in host_configurer_window.cc True. This shorter function is also defined there. I'll factor it out if you think it's worth it. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:63: MessageBox(LoadString(IDS_SAME_PIN).c_str(), LoadString(IDS_TITLE).c_str(), On 2012/10/11 16:57:52, alexeypa wrote: > MessageBox disrupts the UI flow. Is there a way to avoid it? Let's talk offline. There are many possibilities for the UI flow. Experiments will be helpful, and we can always iterate. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:68: MessageBox(LoadString(IDS_INVALID_PIN).c_str(), On 2012/10/11 16:57:52, alexeypa wrote: > Looking at the way LoadString is used, it's ATL counterpart can be used as well > without annoying c_str(). Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:75: base::Bind(&StartHostWindow::OnAuthCode, base::Unretained(this))); On 2012/10/11 16:57:52, alexeypa wrote: > Now do you make sure that |auth_code_getter_| will not invoke > StartHostWindow::OnAuthCode after StartHostWindow has been destroyed? I've added a WeakPtr. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:95: std::string StartHostWindow::GetDlgItemString(int id) { On 2012/10/11 16:57:52, alexeypa wrote: > I'm pretty sure an ATL equivalent of this function exists. I couldn't find a similar function that didn't involve essentially the same amount of work. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:12: #include <atlwin.h> On 2012/10/11 16:57:52, alexeypa wrote: > nit: swap atlwin and atluser Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:24: class StartHostWindow : public ATL::CDialogImpl<StartHostWindow> { On 2012/10/11 16:57:52, alexeypa wrote: > nit: Add and explanation what this class is for. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:36: StartHostWindow(net::URLRequestContextGetter* url_request_context_getter); On 2012/10/11 16:57:52, alexeypa wrote: > nit: scoped_refptr? I don't think a scoped_refptr adds anything here. The context getter has to outlive this object, and it does so. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:48: remoting::AuthCodeGetter auth_code_getter_; On 2012/10/11 16:57:52, alexeypa wrote: > nit: sort the variables alphabetically. Add comments explaining what they are > for. I don't think there's any general rule that variables should be sorted alphabetically. These are sorted by role, and by visual order within the dialog box, in the case of members that reflect widgets. I've added a comment.
https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_... File remoting/host/setup/win/host_configurer_window.h (right): https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; On 2012/10/11 21:27:35, simonmorris wrote: > I don't think scoped_refptr adds anything here. The context getter has to > outlive this object, and it will do so. It is not apparent from the code. scoped_refptr provides a strong guarantee, while keeping it a raw pointer requires the maintainer of the code to be very careful. https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start... File remoting/host/setup/win/start_host_window.cc (right): https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start... remoting/host/setup/win/start_host_window.cc:95: std::string StartHostWindow::GetDlgItemString(int id) { On 2012/10/11 21:27:35, simonmorris wrote: > On 2012/10/11 16:57:52, alexeypa wrote: > > I'm pretty sure an ATL equivalent of this function exists. > > I couldn't find a similar function that didn't involve essentially the same > amount of work. UINT CWidnowCWindow::GetDlgItemText(int nID, CSimpleString& strText) const https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start... File remoting/host/setup/win/start_host_window.h (right): https://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start... remoting/host/setup/win/start_host_window.h:36: StartHostWindow(net::URLRequestContextGetter* url_request_context_getter); On 2012/10/11 21:27:35, simonmorris wrote: > On 2012/10/11 16:57:52, alexeypa wrote: > > nit: scoped_refptr? > > I don't think a scoped_refptr adds anything here. The context getter has to > outlive this object, and it does so. scoped_refptr provides a strong guarantee. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_va... File remoting/host/setup/pin_validator.cc (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_va... remoting/host/setup/pin_validator.cc:10: if (pin.length() < 6) { nit: drop {} https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_va... remoting/host/setup/pin_validator.cc:15: if (c < '0' || c > '9') { nit: drop {} here as well. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_va... File remoting/host/setup/pin_validator.h (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_va... remoting/host/setup/pin_validator.h:12: // Returns whether a PIN is valid. nit: Returns true whether ... https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer.cc (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer.cc:16: : public ATL::CAtlExeModuleT<HostConfigurerModule> { nit: Include atlbase.h & Co. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer_window.cc (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:41: SetWindowText(LoadString(IDS_TITLE)); This is the only call to LoadString. Calling CString::LoadString directly and checking the error code should be enough. CString title; if (!title.LoadString(IDS_TITLE)) { EndDialog(...); return FALSE; } if (!SetWindowText(title)) { EndDialog(...); return FALSE; } https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:42: ::EnableWindow(GetDlgItem(IDC_CHANGE_PIN), FALSE); if (!GetDlgItem(IDC_CHANGE_PIN).EnableWindow(FALSE)) { EndDialog(...); return FALSE; } https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:43: ::EnableWindow(GetDlgItem(IDC_STOP_HOST), FALSE); Same as above. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:104: SetWindowPos(NULL, x, y, -1, -1, SWP_NOSIZE | SWP_NOZORDER | SWP_NOACTIVATE); nit: I think PositionWindow should return the status of SetWindowPos() call. The caller then can decide whether failure to reposition the window is fatal or not. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/st... File remoting/host/setup/win/start_host_window.cc (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/st... remoting/host/setup/win/start_host_window.cc:21: s.LoadString(id); How do you handle an error returned by LoadString? https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/st... remoting/host/setup/win/start_host_window.cc:46: SetWindowText(LoadString(IDS_TITLE)); Same comments as in the other file: - GetDlgItem returns CWindow - Return values should be checked. https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/st... File remoting/host/setup/win/start_host_window.h (right): https://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/st... remoting/host/setup/win/start_host_window.h:53: // Data read from widgets in the dialog box. nit: Add an empty line before the comment.
I thought patch set 1 fixed the WeakPtr problem, and the ExitProcess problem. I was wrong. This patch set has better fixes, so please take another look at them. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; On 2012/10/12 17:09:35, alexeypa wrote: > On 2012/10/11 21:27:35, simonmorris wrote: > > I don't think scoped_refptr adds anything here. The context getter has to > > outlive this object, and it will do so. > > It is not apparent from the code. scoped_refptr provides a strong guarantee, > while keeping it a raw pointer requires the maintainer of the code to be very > careful. One result of that guarantee is that it becomes impossible to know when the pointee will be destroyed. See willchan@'s rant. But I don't think it's important here, so I've moved to scoped_refptr. Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.cc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.cc:95: std::string StartHostWindow::GetDlgItemString(int id) { On 2012/10/12 17:09:35, alexeypa wrote: > On 2012/10/11 21:27:35, simonmorris wrote: > > On 2012/10/11 16:57:52, alexeypa wrote: > > > I'm pretty sure an ATL equivalent of this function exists. > > > > I couldn't find a similar function that didn't involve essentially the same > > amount of work. > > UINT CWidnowCWindow::GetDlgItemText(int nID, CSimpleString& strText) const Done. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... File remoting/host/setup/win/start_host_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/start_... remoting/host/setup/win/start_host_window.h:36: StartHostWindow(net::URLRequestContextGetter* url_request_context_getter); On 2012/10/12 17:09:35, alexeypa wrote: > On 2012/10/11 21:27:35, simonmorris wrote: > > On 2012/10/11 16:57:52, alexeypa wrote: > > > nit: scoped_refptr? > > > > I don't think a scoped_refptr adds anything here. The context getter has to > > outlive this object, and it does so. > > scoped_refptr provides a strong guarantee. Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_val... File remoting/host/setup/pin_validator.cc (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_val... remoting/host/setup/pin_validator.cc:10: if (pin.length() < 6) { On 2012/10/12 17:09:35, alexeypa wrote: > nit: drop {} Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_val... remoting/host/setup/pin_validator.cc:15: if (c < '0' || c > '9') { On 2012/10/12 17:09:35, alexeypa wrote: > nit: drop {} here as well. Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_val... File remoting/host/setup/pin_validator.h (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/pin_val... remoting/host/setup/pin_validator.h:12: // Returns whether a PIN is valid. On 2012/10/12 17:09:35, alexeypa wrote: > nit: Returns true whether ... Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... remoting/host/setup/win/host_configurer.cc:16: : public ATL::CAtlExeModuleT<HostConfigurerModule> { On 2012/10/12 17:09:35, alexeypa wrote: > nit: Include atlbase.h & Co. Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... File remoting/host/setup/win/host_configurer_window.cc (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... remoting/host/setup/win/host_configurer_window.cc:41: SetWindowText(LoadString(IDS_TITLE)); On 2012/10/12 17:09:35, alexeypa wrote: > This is the only call to LoadString. Calling CString::LoadString directly and > checking the error code should be enough. > > CString title; > if (!title.LoadString(IDS_TITLE)) { > EndDialog(...); > return FALSE; > } > > if (!SetWindowText(title)) { > EndDialog(...); > return FALSE; > } I've moved the code to a separate file, so it can be shared with StartHostWindow. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... remoting/host/setup/win/host_configurer_window.cc:42: ::EnableWindow(GetDlgItem(IDC_CHANGE_PIN), FALSE); On 2012/10/12 17:09:35, alexeypa wrote: > if (!GetDlgItem(IDC_CHANGE_PIN).EnableWindow(FALSE)) { > EndDialog(...); > return FALSE; > } Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... remoting/host/setup/win/host_configurer_window.cc:43: ::EnableWindow(GetDlgItem(IDC_STOP_HOST), FALSE); On 2012/10/12 17:09:35, alexeypa wrote: > Same as above. Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/hos... remoting/host/setup/win/host_configurer_window.cc:104: SetWindowPos(NULL, x, y, -1, -1, SWP_NOSIZE | SWP_NOZORDER | SWP_NOACTIVATE); On 2012/10/12 17:09:35, alexeypa wrote: > nit: I think PositionWindow should return the status of SetWindowPos() call. The > caller then can decide whether failure to reposition the window is fatal or not. Failure to reposition the window should never be fatal. No user prefers an application that will not run, to an application with a sub-optimally positioned window. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/sta... File remoting/host/setup/win/start_host_window.cc (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/sta... remoting/host/setup/win/start_host_window.cc:21: s.LoadString(id); On 2012/10/12 17:09:35, alexeypa wrote: > How do you handle an error returned by LoadString? This code has moved to load_string_from_resource.cc. It now loads a "missing resource XXX" string, if it can't load the real string. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/sta... remoting/host/setup/win/start_host_window.cc:46: SetWindowText(LoadString(IDS_TITLE)); On 2012/10/12 17:09:35, alexeypa wrote: > Same comments as in the other file: > - GetDlgItem returns CWindow > - Return values should be checked. Done. http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/sta... File remoting/host/setup/win/start_host_window.h (right): http://codereview.chromium.org/11090063/diff/2003/remoting/host/setup/win/sta... remoting/host/setup/win/start_host_window.h:53: // Data read from widgets in the dialog box. On 2012/10/12 17:09:35, alexeypa wrote: > nit: Add an empty line before the comment. Done.
lgtm, once my comments are addressed. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; On 2012/10/15 16:51:41, simonmorris wrote: > One result of that guarantee is that it becomes impossible to know when the > pointee will be destroyed. See willchan@'s rant. It is not possible to know without scoped_refptr as well. Having scoped_refptr actually makes the situation more defined - the object will not be killed before the reference is released, while before it potentially could. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer.cc:7: #include <windows.h> nit: no need to include <windows.h> if atlbase.h is there. This will also make the list of includes sorted. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer_window.cc (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:38: GetDlgItem(IDC_CHANGE_PIN).EnableWindow(FALSE); nit: I'm not fun of zillions of UI calls none of them checking the return value. I believe the same rules should apply to UI calls as to any other API calls. The problem is that when the return value is ignored the reader has to read the code as if there are two branches instead of one. The code following the call is executed both in case of success and failure. The reader has to understand why it is safe. An explicit condition statement make the error handling logic more apparent. Here for instance the first question that I have is why is it safe ti ignore the error? It is followed by what does the error mean? Can an attacker exploit this somehow? I believe each case of ignoring the result should be explained in the comments, explicitly stating why it is safe. It is OK to do it in a follow up CL. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:105: DestroyWindow(); DestroyWindow() should be called on the same thread that was used to create the window, i.e. the UI thread. Is it possible that Finish() will be called on a different thread? If so - just to the IU thread first. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:106: ui_task_runner_->PostTask(FROM_HERE, quit_); This function is probably running on the UI thread. Do you have to post |quit_|? Can you call it directly instead? http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:23: class HostConfigurerWindow : public ATL::CDialogImpl<HostConfigurerWindow> { nit: Add a comment explaining that this class is a modeless dialog, otherwise it is not obvious why you use DestroyWindow(). http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:33: HostConfigurerWindow( nit: I suspect all methods of this class should be called on the UI thread. It would be nice to document it and add DCHECKs to validate it in runtime. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:58: base::Closure quit_; nit: You don't have to mention that |quit_| stop the message loop. You can just say that it notifies the caller that the window has been destroyed. It will be up to the caller to decide what to do.
fyi http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer.cc (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer.cc:7: #include <windows.h> On 2012/10/15 18:48:55, alexeypa wrote: > nit: no need to include <windows.h> if atlbase.h is there. This will also make > the list of includes sorted. Done. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer_window.cc (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:38: GetDlgItem(IDC_CHANGE_PIN).EnableWindow(FALSE); On 2012/10/15 18:48:55, alexeypa wrote: > nit: I'm not fun of zillions of UI calls none of them checking the return value. > I believe the same rules should apply to UI calls as to any other API calls. The > problem is that when the return value is ignored the reader has to read the code > as if there are two branches instead of one. The code following the call is > executed both in case of success and failure. The reader has to understand why > it is safe. An explicit condition statement make the error handling logic more > apparent. > > Here for instance the first question that I have is why is it safe ti ignore the > error? It is followed by what does the error mean? Can an attacker exploit this > somehow? > > I believe each case of ignoring the result should be explained in the comments, > explicitly stating why it is safe. > > It is OK to do it in a follow up CL. I've added comments indicating that it's acceptable for these calls to fail. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:105: DestroyWindow(); On 2012/10/15 18:48:55, alexeypa wrote: > DestroyWindow() should be called on the same thread that was used to create the > window, i.e. the UI thread. Is it possible that Finish() will be called on a > different thread? If so - just to the IU thread first. Done. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.cc:106: ui_task_runner_->PostTask(FROM_HERE, quit_); On 2012/10/15 18:48:55, alexeypa wrote: > This function is probably running on the UI thread. Do you have to post |quit_|? > Can you call it directly instead? Done. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:23: class HostConfigurerWindow : public ATL::CDialogImpl<HostConfigurerWindow> { On 2012/10/15 18:48:55, alexeypa wrote: > nit: Add a comment explaining that this class is a modeless dialog, otherwise it > is not obvious why you use DestroyWindow(). Done. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:33: HostConfigurerWindow( On 2012/10/15 18:48:55, alexeypa wrote: > nit: I suspect all methods of this class should be called on the UI thread. It > would be nice to document it and add DCHECKs to validate it in runtime. Done. http://codereview.chromium.org/11090063/diff/15001/remoting/host/setup/win/ho... remoting/host/setup/win/host_configurer_window.h:58: base::Closure quit_; On 2012/10/15 18:48:55, alexeypa wrote: > nit: You don't have to mention that |quit_| stop the message loop. You can just > say that it notifies the caller that the window has been destroyed. It will be > up to the caller to decide what to do. Done.
ping sergeyu@
http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer.rc (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer.rc:9: PUSHBUTTON "Start host" IDC_START_HOST, 65, 8, 50, 14 On 2012/10/11 21:27:35, simonmorris wrote: > On 2012/10/11 16:57:52, alexeypa wrote: > > On 2012/10/11 01:15:24, sergeyu wrote: > > > These strings need to be localizeable. It might be better to create controls > > > manually, without dialog resources > > > > If we are going to do localization in Windows way, we should use dialog > > resources. We might need to adjust dialog layout to accommodate longer > strings. > > We will have to use MUI resources or local resources manually then. > > > > Alternatively the dialog layout should be able to fit the longest translated > > strings. Then we will be able to get away with a single copy of resources and > > patching strings in runtime. > > This can be addressed in a separate CL, when we've decided how to tackle l10n. That's ok, but it's better to think about it sooner rather then later, otherwise we may need to rewrite some of the UI code. http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... File remoting/host/setup/win/host_configurer_window.h (right): http://codereview.chromium.org/11090063/diff/1/remoting/host/setup/win/host_c... remoting/host/setup/win/host_configurer_window.h:44: net::URLRequestContextGetter* url_request_context_getter_; On 2012/10/15 18:48:55, alexeypa wrote: > On 2012/10/15 16:51:41, simonmorris wrote: > > One result of that guarantee is that it becomes impossible to know when the > > pointee will be destroyed. See willchan@'s rant. > > It is not possible to know without scoped_refptr as well. Having scoped_refptr > actually makes the situation more defined - the object will not be killed before > the reference is released, while before it potentially could. +1 on this. Using raw pointer for ref-counted object is wrong. willchan was ranting about using ref-counting at all, not about usage of scoped_refptr for ref-counted objects. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... File remoting/host/setup/host_starter.cc (right): http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:21: std::string MakeHostId() { I think I already asked about it in a previous CL: Why can't we reuse base::GenerateGUID() here? If you don't wan't to rely on GUID generation on windows, you can still reuse RandomDataToGUIDString() to format GUIDs. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:97: const std::string& host_name, const std::string& host_pin, nit: One argument per line please (http://dev.chromium.org/developers/coding-style) http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:110: on_done_runner_ = on_done_runner; Instead of passing on_done_runner here it would be better to remember the thread we are called from (i.e. base::CurrentThreadRunnerHandle::Get()) so that the called expects the callback on the same thread on which it calls StartHost(). Alternatively you can pass the main thread in the constructor, and then DCHECK here that we are called on the right thread. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:120: refresh_token_ = refresh_token; Here and in all other callback methods: please add DCHECKs to verify that we are on the right thread (i.e. main UI thread). http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:157: in_progress_ = false; This method is called on the thread that daemon controller creates internally and it is different from UI thread, so there is a potential race condition when you mutate the object here. It would be better to jump back to the UI thread first and only then do everything else. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/pin_va... File remoting/host/setup/pin_validator.cc (right): http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/pin_va... remoting/host/setup/pin_validator.cc:9: bool IsPinValid(const std::string& pin) { Already reviewed in another CL.
http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... File remoting/host/setup/host_starter.cc (right): http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:21: std::string MakeHostId() { On 2012/10/15 23:44:26, sergeyu wrote: > I think I already asked about it in a previous CL: Why can't we reuse > base::GenerateGUID() here? If you don't wan't to rely on GUID generation on > windows, you can still reuse RandomDataToGUIDString() to format GUIDs. I'll make such a change in a separate CL: it doesn't belong in this one. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:97: const std::string& host_name, const std::string& host_pin, On 2012/10/15 23:44:26, sergeyu wrote: > nit: One argument per line please > (http://dev.chromium.org/developers/coding-style) Done. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:110: on_done_runner_ = on_done_runner; On 2012/10/15 23:44:26, sergeyu wrote: > Instead of passing on_done_runner here it would be better to remember the thread > we are called from (i.e. base::CurrentThreadRunnerHandle::Get()) so that the > called expects the callback on the same thread on which it calls StartHost(). > > Alternatively you can pass the main thread in the constructor, and then DCHECK > here that we are called on the right thread. Done. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:120: refresh_token_ = refresh_token; On 2012/10/15 23:44:26, sergeyu wrote: > Here and in all other callback methods: please add DCHECKs to verify that we are > on the right thread (i.e. main UI thread). Many of the other callback methods are called from the network thread. I've added code to post them onto the UI thread. http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:157: in_progress_ = false; On 2012/10/15 23:44:26, sergeyu wrote: > This method is called on the thread that daemon controller creates internally > and it is different from UI thread, so there is a potential race condition when > you mutate the object here. It would be better to jump back to the UI thread > first and only then do everything else. Done.
On 2012/10/16 17:23:03, simonmorris wrote: > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > File remoting/host/setup/host_starter.cc (right): > > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > remoting/host/setup/host_starter.cc:21: std::string MakeHostId() { > On 2012/10/15 23:44:26, sergeyu wrote: > > I think I already asked about it in a previous CL: Why can't we reuse > > base::GenerateGUID() here? If you don't wan't to rely on GUID generation on > > windows, you can still reuse RandomDataToGUIDString() to format GUIDs. > > I'll make such a change in a separate CL: it doesn't belong in this one. > > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > remoting/host/setup/host_starter.cc:97: const std::string& host_name, const > std::string& host_pin, > On 2012/10/15 23:44:26, sergeyu wrote: > > nit: One argument per line please > > (http://dev.chromium.org/developers/coding-style) > > Done. > > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > remoting/host/setup/host_starter.cc:110: on_done_runner_ = on_done_runner; > On 2012/10/15 23:44:26, sergeyu wrote: > > Instead of passing on_done_runner here it would be better to remember the > thread > > we are called from (i.e. base::CurrentThreadRunnerHandle::Get()) so that the > > called expects the callback on the same thread on which it calls StartHost(). > > > > Alternatively you can pass the main thread in the constructor, and then DCHECK > > here that we are called on the right thread. > > Done. > > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > remoting/host/setup/host_starter.cc:120: refresh_token_ = refresh_token; > On 2012/10/15 23:44:26, sergeyu wrote: > > Here and in all other callback methods: please add DCHECKs to verify that we > are > > on the right thread (i.e. main UI thread). > > Many of the other callback methods are called from the network thread. I've > added code to post them onto the UI thread. > > http://codereview.chromium.org/11090063/diff/28001/remoting/host/setup/host_s... > remoting/host/setup/host_starter.cc:157: in_progress_ = false; > On 2012/10/15 23:44:26, sergeyu wrote: > > This method is called on the thread that daemon controller creates internally > > and it is different from UI thread, so there is a potential race condition > when > > you mutate the object here. It would be better to jump back to the UI thread > > first and only then do everything else. > > Done. ping
LGTM with two nits. One thing I'm not sure about is that you create two separate windows: HostConfigurerWindow and StartHostWindow. It would be better to have just one wizard window that would show different pages depending on state of the host. But it's fine to keep it as is for now - we can rework it later. http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... File remoting/host/setup/host_starter.cc (right): http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:114: main_task_runner_ = base::ThreadTaskRunnerHandle::Get(); Maybe do it in the constructor and add DCHECK here to make sure that object is created and used on the same thread? http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:140: user_email)); nit: Will this fit on the previous line?
On 2012/10/17 18:34:13, sergeyu wrote: > LGTM with two nits. > One thing I'm not sure about is that you create two separate windows: > HostConfigurerWindow and StartHostWindow. It would be better to have just one > wizard window that would show different pages depending on state of the host. > But it's fine to keep it as is for now - we can rework it later. > Yes, this is only intended as a proof-of-concept, and a demonstration that the worker code does what it should do, not as a particularly good UI.
fyi http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... File remoting/host/setup/host_starter.cc (right): http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:114: main_task_runner_ = base::ThreadTaskRunnerHandle::Get(); On 2012/10/17 18:34:14, sergeyu wrote: > Maybe do it in the constructor and add DCHECK here to make sure that object is > created and used on the same thread? Done. http://codereview.chromium.org/11090063/diff/32001/remoting/host/setup/host_s... remoting/host/setup/host_starter.cc:140: user_email)); On 2012/10/17 18:34:14, sergeyu wrote: > nit: Will this fit on the previous line? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11090063/39001
Presubmit check for 11090063-39001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) 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/setup/win/host_configurer_resource.h Presubmit checks took 2.0s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11090063/42002
Retried try job too often for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chrome_frame_net_tests, chrome_frame_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, printing_unittests, remoting_unittests, safe_browsing_tests, sql_unittests, sync_integration_tests, sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11090063/42002
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 500: <html><head> <meta http-equiv="content-type" content="text/html;charset=utf-8"> <title>500 Server Error</title> </head> <body text=#000000 bgcolor=#ffffff> <h1>Error: Server Error</h1> <h2>The server encountered an error and could not complete your request.<p>If the problem persists, please <A HREF="http://code.google.com/appengine/community.html">report</A> your problem and mention this error message and the query that caused it.</h2> <h2></h2> </body></html>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11090063/42002
Change committed as 162600 |
