Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9953002:
The me2me host is now configurable from the web UI on Windows. (Closed)
|
Created:
8 years, 8 months ago by alexeypa (please no reviews) 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. |
DescriptionChromoting: The me2me host is now configurable from the web UI on Windows.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129999
Patch Set 1 #Patch Set 2 : Moving the Windows service name to a common location. #
Total comments: 38
Patch Set 3 : CR feedback #
Total comments: 120
Patch Set 4 : Addressed one or two nits. :-) #
Messages
Total messages: 13 (0 generated)
PTAL. Jamie & Sergey - JS and the daemon controller. Wez - the rest.
https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... File remoting/host/branding.h (right): https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... remoting/host/branding.h:13: extern const char kWindowsServiceName[]; Should this be ifdef'd for Windows only? https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... File remoting/host/plugin/daemon_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:39: // A simple wrapper arounf base::Thread making sure that COM is initialized on Nit: s/arounf/around/ https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:41: class CoThread: public base::Thread { Nit: spacing around colon. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:108: State forbidden_state_; This is a confusing name. If it's doing what I think it is, this is a "termination" state that signifies an error (ie, once we reach this state, something has gone wrong, and it's not going to recover). If so, perhaps it should be called error_termination_state_? https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:130: HRESULT hr = E_FAIL; Couldn't you initialize this to S_OK and skip the else clause below? https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:152: << std::hex << hr << std::dec << ")."; Is the explicit reversion to std::dec necessary? https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:188: if (forbidden_state_ != new_state || FAILED(last_error_)) { Why the second part of the OR? It's overruled by the next line. Also, the first part of the OR feels like a test for success, (we're not in the forbidden state) but the second feels like a test for failure. I can't parse what's going on here. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:208: state_ = STATE_NOT_INSTALLED; Should return NOT_IMPLEMENTED for now, because we can't install, but the JS doesn't know that.
some nits, but LGTM otherwise. http://codereview.chromium.org/9953002/diff/2001/remoting/host/branding.cc File remoting/host/branding.cc (right): http://codereview.chromium.org/9953002/diff/2001/remoting/host/branding.cc#ne... remoting/host/branding.cc:37: const char kWindowsServiceName[] = "chromoting"; I thin we should use something else (e.g. chrome_remote_desktop) for official builds http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... File remoting/host/plugin/daemon_controller_win.cc (right): http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:25: #include <elevated_controller.h> nit: I think this should be in quotes, it's not a system header. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:41: class CoThread: public base::Thread { should it be called ComThread? http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:108: State forbidden_state_; Alternatively you can have flags like start_pending_ and stop_pending_ to indicated when start/stop task has been posted to CoThread, but hasn't been processed. And then you adjust the value returned from GetState() depending on these flags. But that's optional for this CL - we will need to cleanup this interface anyway. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:108: State forbidden_state_; On 2012/03/30 01:11:47, Jamie wrote: > This is a confusing name. If it's doing what I think it is, this is a > "termination" state that signifies an error (ie, once we reach this state, > something has gone wrong, and it's not going to recover). If so, perhaps it > should be called error_termination_state_? It's not really termination state. The problem is that the webapp expects GetState() to return STARTING after calling Start(), but that is hard to satisfy when the service state machine is controlled by the OS, and we need to call the OS on a separate thread. We should add completion callbacks to Start() and Stop() methods to avoid hacks like this (and that way we won't need state polling in the webapp). http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:140: hr = ::CoGetObject(ASCIIToUTF16(kElevationMoniker).c_str(), Are you sure that the result of ASCIIToUTF16() is not destroyed before CoGetObject() is called? If not maybe better to allocate variable for that string. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:143: (void**)&control_); C style casts are not allowed by code style: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... use reinterpret_cast<> http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:152: << std::hex << hr << std::dec << ")."; May be cleaner to use StringPrintf() for format hex numbers - iostream interface is ugly. Feel free to disagree. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:163: nit: don't need this empty line. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:310: base::JSONReader::Read(UTF16ToUTF8(file_content), true)); We should probably change this interface to pass the config as string. Doesn't look like DictionaryValue is useful here - HostScriptObject serializes it back to string anyway... http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:312: if (config.get() == NULL || !config->IsType(base::Value::TYPE_DICTIONARY)) { It would be cleaner to use GetAsDictionary() method instead of IsType() and static_cast below. http://codereview.chromium.org/9953002/diff/2001/remoting/host/plugin/daemon_... remoting/host/plugin/daemon_controller_win.cc:379: remoting::DaemonController::State DaemonControllerWin::ConvertToDaemonState( nit: doesn't look like this need to be a class member. Maybe move make it a static function.
PTAL. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... File remoting/host/branding.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... remoting/host/branding.cc:37: const char kWindowsServiceName[] = "chromoting"; On 2012/03/30 07:42:35, sergeyu wrote: > I thin we should use something else (e.g. chrome_remote_desktop) for official > builds We will if we decide to make the official and non-official builds completely separate. This will required changing GUID in the installation script and, maybe, come other changes (event log name, ...). For now we should be fine with sharing non user-facing names. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... File remoting/host/branding.h (right): https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/brandi... remoting/host/branding.h:13: extern const char kWindowsServiceName[]; On 2012/03/30 01:11:47, Jamie wrote: > Should this be ifdef'd for Windows only? Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... File remoting/host/plugin/daemon_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:25: #include <elevated_controller.h> On 2012/03/30 07:42:35, sergeyu wrote: > nit: I think this should be in quotes, it's not a system header. Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:39: // A simple wrapper arounf base::Thread making sure that COM is initialized on On 2012/03/30 01:11:47, Jamie wrote: > Nit: s/arounf/around/ Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:41: class CoThread: public base::Thread { On 2012/03/30 01:11:47, Jamie wrote: > Nit: spacing around colon. Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:41: class CoThread: public base::Thread { On 2012/03/30 07:42:35, sergeyu wrote: > should it be called ComThread? Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:108: State forbidden_state_; On 2012/03/30 07:42:35, sergeyu wrote: > We should add completion callbacks to Start() and Stop() methods to avoid hacks > like this (and that way we won't need state polling in the webapp). Exactly. I added a TODO. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:130: HRESULT hr = E_FAIL; On 2012/03/30 01:11:47, Jamie wrote: > Couldn't you initialize this to S_OK and skip the else clause below? Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:140: hr = ::CoGetObject(ASCIIToUTF16(kElevationMoniker).c_str(), On 2012/03/30 07:42:35, sergeyu wrote: > Are you sure that the result of ASCIIToUTF16() is not destroyed before > CoGetObject() is called? If not maybe better to allocate variable for that > string. I'm pretty sure. It is guaranteed by the C++ standard. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:143: (void**)&control_); On 2012/03/30 07:42:35, sergeyu wrote: > C style casts are not allowed by code style: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... > > use reinterpret_cast<> Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:152: << std::hex << hr << std::dec << ")."; On 2012/03/30 01:11:47, Jamie wrote: > Is the explicit reversion to std::dec necessary? Yes. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:152: << std::hex << hr << std::dec << ")."; On 2012/03/30 07:42:35, sergeyu wrote: > May be cleaner to use StringPrintf() for format hex numbers - iostream interface > is ugly. Feel free to disagree. I tried using StringPrintf. The mixture of streams and printf formatting looks weird and it is not a bit shorter. I'll keep this code as it is. There are other examples of using manipulators in the code. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:163: On 2012/03/30 07:42:35, sergeyu wrote: > nit: don't need this empty line. Without the empty line it looks cluttered. I added a comment explaining why UI message loop is needed instead of the empty line. :-) https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:188: if (forbidden_state_ != new_state || FAILED(last_error_)) { On 2012/03/30 01:11:47, Jamie wrote: > Why the second part of the OR? It's overruled by the next line. There are two hacks in there. First we don't report the state we started an operation in (i.e. "stop" for starting and "running" for stopping) unless an error occurs. In the latter case there will be no further processing even if the state didn't change. I.e. for starting: stop -> stop (with error). The 2nd hack is because we don't have a good way to report an error. We have start_failed state but what is there is an error when stopping the daemon? I prefer to keep two hack separate and remove them one by one as we are refactoring the JS interface. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:208: state_ = STATE_NOT_INSTALLED; On 2012/03/30 01:11:47, Jamie wrote: > Should return NOT_IMPLEMENTED for now, because we can't install, but the JS > doesn't know that. Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:310: base::JSONReader::Read(UTF16ToUTF8(file_content), true)); On 2012/03/30 07:42:35, sergeyu wrote: > We should probably change this interface to pass the config as string. Doesn't > look like DictionaryValue is useful here - HostScriptObject serializes it back > to string anyway... It would definitely make my life easier. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:312: if (config.get() == NULL || !config->IsType(base::Value::TYPE_DICTIONARY)) { On 2012/03/30 07:42:35, sergeyu wrote: > It would be cleaner to use GetAsDictionary() method instead of IsType() and > static_cast below. Done. https://chromiumcodereview.appspot.com/9953002/diff/2001/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:379: remoting::DaemonController::State DaemonControllerWin::ConvertToDaemonState( On 2012/03/30 07:42:35, sergeyu wrote: > nit: doesn't look like this need to be a class member. Maybe move make it a > static function. It is a static member. It does not have to be a member but it an easy way to say that the function knows something private to DaemonControllerWin. DaemonControllerWin is not known outside of this module so we are not polluting the headers either.
lgtm
Wez, ping?
On 2012/03/30 21:40:36, alexeypa wrote: > Wez, ping? Working on it. :)
Basically LGTM, but with one or two nits. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/brandi... File remoting/host/branding.h (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/brandi... remoting/host/branding.h:14: extern const char kWindowsServiceName[]; Although this is currently Windows-specific, would it make sense for this to be a cross-platform define used for the Mac launchd service name(s) and potentially the Linux init.d service name? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:24: const size_t kMaxConfigFileSize = 0x10000; nit: Specifying the max file size in hex makes it less obvious to the casual reader what the limit is! https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:27: const char kXmppLogin[] = "xmpp_login"; nit: Add a comment to explain what these constants are for. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:30: const FilePath::CharType kAuthConfig[] = FILE_PATH_LITERAL("auth.json"); nit: These should really be kAuthConfig -> kAuthConfigFilename. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:33: // TODO(alexeypa): remove the hardcoded undocimented paths and store typos: remove -> Remove, undocimented. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:43: // Reads and parses a JSON configuration file (up to 64KB in size). nit: The "up to 64KB in size" comment goes with the definition of kMaxConfigFileSize. If kMaxConfigFileSize only applies to this function, consider defining it local to the function. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:72: // Parse JSON data. nit: "Parse the JSON configuration, expecting it to contain a dictionary." https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:83: config_out->reset(dictionary); Gah. Gross. Can't see a better way to do it, off the top of my head, though, short of adding TakeAsDictionary() to base::Value. :( https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:105: // Build the host configuration. nit: Isn't this reading it, rather than "building" it? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:113: // Build the filtered config. Why? What is the purpose of the filtering? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:121: nit: No need for this blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:126: // Convert the filtered config back to string and return it to the caller. typo: ... back to a string ... https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:139: FilePath system_profile; nit: Comment before these lines e.g. "Determine the config directory path and create it if necessary". May want to comment on the security guarantees this has, in terms of who can read the config. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:141: nit: No need for this blank line? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:150: int written = file_util::WriteFile( Should we be doing the write-temp-file-then-delete-old-file-then-rename-temp-to-be-new-config dance here? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:159: // TODO(alexeypa): make it a single file. nit: I think you mean "Store the authentication and host configurations in a single file"? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:166: return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED); nit: That's not strictly the right error, and in fact access-denied would lead me to believe that the config file wasn't touch, whereas it will have been written truncated data? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:196: return hr; Do you actually want to return potentially arbitrary return-codes from the service control manager from these methods? It might be cleaner to continue to log the error with LOG_GETLASTERROR() as you do below but return something better defined to the caller. I assume you're converting Win32->HRESULT to avoid GetLastError() getting trampled during logging. One slight oddity of this arrangement is that APIs returning an invalid value but leaving GetLastError() set to zero will be treated as having succeeded. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:227: ::OpenServiceA(scmanager, kWindowsServiceName, nit: Why are we using OpenServiceA but OpenSCManagerW? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.h (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.h:39: HRESULT OpenService(ScopedScHandle* service_out); base::ScopedScHandle? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/host_s... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/host_s... remoting/host/host_service_win.cc:105: service_name_(ASCIIToUTF16(kWindowsServiceName)), nit: Someone could conceivably put UTF-8 in kWindowsServiceName; will UTF8ToUTF16 not work here? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... File remoting/host/plugin/daemon_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:32: // The COM elevation moniker for the elevated controller. nit: Consider adding a sentence to explain what this is actually used for. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:36: // Name of the worker thread. nit: Similarly, what worker thread? What does the thread actually do? Why is the constant not called kDaemonControllerThreadName? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:40: // the owner thread. nit: I think you mean "A base::Thread implementation that initializes COM on the new thread"? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:45: // Activates an elevated instance of elevated controller and returns nit: ... elevated instance of the controller ... or ... elevated instance of ElevatedController https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:46: // the pointer to the control interface in |control_out|. This routine keeps nit: The routine can't keep ownership; do you mean the ComThread does? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:55: IDaemonControl* control_; nit: Perhaps we should have a ScopedIPtr... ;) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:73: // Activates an elevated instance of elevated controller and returns nit: See above. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:77: // Opens the controlled service handle. nit: What is the controlled service handle? You mean it opens a handle to the Daemon service? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:80: // Worker functions called in the context of the worker thread. nit: Double use of the term "worker" makes this comment less helpful! https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:86: // SERVICE_RUNNING, SERVICE_STOPPED) to the daemon state. nit: "Converts a Windows service status code to a Daemon state." https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:89: // Service status change notification callback. nit: Suggest rewording e.g. "Status change callback used to monitor start/stop progress". https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:101: // Cached daemon state. nit: This is the state as of the most recent status-change callback? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:107: // "stopped" as a failure to start the service. This comment is very confusing, as is the name |forbidden_state_|. The idea is that the actual state of the service doesn't necessarily reflect transitions accurately, so we only report it to callers in case of errors, and otherwise maintain an "official" state the rest of the time? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:108: // TODO(alexeypa): remove this variable once JS interafce supports callbacks. typo: interafce https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:125: nit: Personally I'd lose the {} on the if, and this blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:131: nit: No need for blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:139: IDaemonControl* control = NULL; This doesn't seem to be used? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:160: // N.B. The UI message loop is required for the single threaded COM apartment. typo: The UI ... is required for ... -> COM must be run on a UI message loop ... https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:162: if (!worker_thread_.StartWithOptions(thread_options)) { Since ComThread doesn't make sense unless started with a UI message loop, can we make it default to one instead of passing options here? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:175: // that can receive APC callbacks. nit: Suggest "Make the thread alertable, so we can switch to APC notifications rather than polling, "? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:186: // supports callbacks. nit: remove ... -> Remove ... https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:191: // TODO(alexeypa): remove this hack once JS nicely reports errors. nit: It's not immediately obvious what the hack is here. :) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:227: // TODO(alexeypa): implement on-demand installation. typo: implement ... -> Implement ... https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:227: // TODO(alexeypa): implement on-demand installation. nit: We generally prefer to create TODOs with an associated bug, if possible, and refer to the bug-Id in the TODO. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:248: state_ == STATE_STOPPING) { These are cached states; isn't there a risk that something has started the service since we read them? Is there any danger in posting a stop control when the service isn't in one of these states? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:249: nit: Errant blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:259: DWORD DaemonControllerWin::OpenService(ScopedScHandle* service_out) { nit: This function looks oddly familiar from ElevatedController... ;) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:296: BSTR host_config = NULL; Is there no managed container for BSTR? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:318: callback.Run(scoped_ptr<base::DictionaryValue>(dictionary)); Yup. Still gross ... :P https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:323: nit: Errant blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:328: last_error_ = hr; Why do we lock when setting |last_error_|? Surely if someone is querying asynchronously then there's going to be a race in the states they see? https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:332: // Set the configuration file Suggest "Store the configuration." https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:345: nit: No need for blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:354: nit: No need for blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:371: nit: Blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:381: nit: Blank line. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:385: nit: No need for these blank lines. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:389: break; nit: Nor for this break, or the ones below.
FYI. Addressed one or two Wez's nits. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/brandi... File remoting/host/branding.h (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/brandi... remoting/host/branding.h:14: extern const char kWindowsServiceName[]; On 2012/03/30 22:11:01, Wez wrote: > Although this is currently Windows-specific, would it make sense for this to be > a cross-platform define used for the Mac launchd service name(s) and potentially > the Linux init.d service name? The service name on MAC is "org.chromium.chromoting". I assume that the prefix is mandatory while on Windows we assume "chromoting" is unique enough. I guess we could change it eventually to be "org.chromium.chromoting". https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:24: const size_t kMaxConfigFileSize = 0x10000; On 2012/03/30 22:11:01, Wez wrote: > nit: Specifying the max file size in hex makes it less obvious to the casual > reader what the limit is! Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:27: const char kXmppLogin[] = "xmpp_login"; On 2012/03/30 22:11:01, Wez wrote: > nit: Add a comment to explain what these constants are for. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:30: const FilePath::CharType kAuthConfig[] = FILE_PATH_LITERAL("auth.json"); On 2012/03/30 22:11:01, Wez wrote: > nit: These should really be kAuthConfig -> kAuthConfigFilename. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:33: // TODO(alexeypa): remove the hardcoded undocimented paths and store On 2012/03/30 22:11:01, Wez wrote: > typos: remove -> Remove, undocimented. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:43: // Reads and parses a JSON configuration file (up to 64KB in size). On 2012/03/30 22:11:01, Wez wrote: > nit: The "up to 64KB in size" comment goes with the definition of > kMaxConfigFileSize. If kMaxConfigFileSize only applies to this function, > consider defining it local to the function. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:72: // Parse JSON data. On 2012/03/30 22:11:01, Wez wrote: > nit: "Parse the JSON configuration, expecting it to contain a dictionary." Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:105: // Build the host configuration. On 2012/03/30 22:11:01, Wez wrote: > nit: Isn't this reading it, rather than "building" it? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:113: // Build the filtered config. On 2012/03/30 22:11:01, Wez wrote: > Why? What is the purpose of the filtering? This matched Mac implementation. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:121: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for this blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:126: // Convert the filtered config back to string and return it to the caller. On 2012/03/30 22:11:01, Wez wrote: > typo: ... back to a string ... Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:139: FilePath system_profile; On 2012/03/30 22:11:01, Wez wrote: > nit: Comment before these lines e.g. "Determine the config directory path and > create it if necessary". May want to comment on the security guarantees this > has, in terms of who can read the config. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:141: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for this blank line? Removed. My eyes hurt now! :-) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:150: int written = file_util::WriteFile( On 2012/03/30 22:11:01, Wez wrote: > Should we be doing the > write-temp-file-then-delete-old-file-then-rename-temp-to-be-new-config dance > here? We could. However the whole configuration flow is so not transactional that at this point it does not matter if we fail half way into the process. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:159: // TODO(alexeypa): make it a single file. On 2012/03/30 22:11:01, Wez wrote: > nit: I think you mean "Store the authentication and host configurations in a > single file"? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:166: return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED); On 2012/03/30 22:11:01, Wez wrote: > it will have been written truncated data? Nope. I bet something else bad happened. I replaced it E_FAIL which means exactly that. Broadly. :-) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:196: return hr; On 2012/03/30 22:11:01, Wez wrote: > Do you actually want to return potentially arbitrary return-codes from the > service control manager from these methods? It might be cleaner to continue to > log the error with LOG_GETLASTERROR() as you do below but return something > better defined to the caller. I don't think so. Something better defined is "operation failed"/"succeeded". OS codes deliver this message and provide a possibility to display a more meaningful error message to the user (potentially). > I assume you're converting Win32->HRESULT to avoid GetLastError() getting > trampled during logging. No. COM methods should return HRESULT error by convention. > One slight oddity of this arrangement is that APIs > returning an invalid value but leaving GetLastError() set to zero will be > treated as having succeeded. GetLastError() is purely Win32 concept. It usually does not apply to intra-application interfaces. It also typically applies only to functions returned a boolean instead of error code. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:227: ::OpenServiceA(scmanager, kWindowsServiceName, On 2012/03/30 22:11:01, Wez wrote: > nit: Why are we using OpenServiceA but OpenSCManagerW? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/host_s... File remoting/host/host_service_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/host_s... remoting/host/host_service_win.cc:105: service_name_(ASCIIToUTF16(kWindowsServiceName)), On 2012/03/30 22:11:01, Wez wrote: > nit: Someone could conceivably put UTF-8 in kWindowsServiceName; will > UTF8ToUTF16 not work here? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... File remoting/host/plugin/daemon_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:32: // The COM elevation moniker for the elevated controller. On 2012/03/30 22:11:01, Wez wrote: > nit: Consider adding a sentence to explain what this is actually used for. This is typical RTFM. I.e. one sentence is too little. More text is overkill. "The COM elevation moniker" is a unique phase interested folks can use to read about it. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:36: // Name of the worker thread. On 2012/03/30 22:11:01, Wez wrote: > nit: Similarly, what worker thread? What does the thread actually do? Why is > the constant not called kDaemonControllerThreadName? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:40: // the owner thread. On 2012/03/30 22:11:01, Wez wrote: > nit: I think you mean "A base::Thread implementation that initializes COM on the > new thread"? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:45: // Activates an elevated instance of elevated controller and returns On 2012/03/30 22:11:01, Wez wrote: > nit: ... elevated instance of the controller ... > > or ... elevated instance of ElevatedController Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:46: // the pointer to the control interface in |control_out|. This routine keeps On 2012/03/30 22:11:01, Wez wrote: > nit: The routine can't keep ownership; do you mean the ComThread does? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:55: IDaemonControl* control_; On 2012/03/30 22:11:01, Wez wrote: > nit: Perhaps we should have a ScopedIPtr... ;) It is not going to help in this case. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:73: // Activates an elevated instance of elevated controller and returns On 2012/03/30 22:11:01, Wez wrote: > nit: See above. Stale code. Removed. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:77: // Opens the controlled service handle. On 2012/03/30 22:11:01, Wez wrote: > nit: What is the controlled service handle? You mean it opens a handle to the > Daemon service? Reworded. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:80: // Worker functions called in the context of the worker thread. On 2012/03/30 22:11:01, Wez wrote: > nit: Double use of the term "worker" makes this comment less helpful! Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:86: // SERVICE_RUNNING, SERVICE_STOPPED) to the daemon state. On 2012/03/30 22:11:01, Wez wrote: > nit: "Converts a Windows service status code to a Daemon state." Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:89: // Service status change notification callback. On 2012/03/30 22:11:01, Wez wrote: > nit: Suggest rewording e.g. "Status change callback used to monitor start/stop > progress". Stale code. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:101: // Cached daemon state. On 2012/03/30 22:11:01, Wez wrote: > nit: This is the state as of the most recent status-change callback? No, it is not. Reworded. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:107: // "stopped" as a failure to start the service. On 2012/03/30 22:11:01, Wez wrote: > This comment is very confusing, as is the name |forbidden_state_|. forbidden_state_ is a hack and it is confusing because it is a hack! > The idea is that the actual state of the service doesn't necessarily reflect > transitions accurately, so we only report it to callers in case of errors, and > otherwise maintain an "official" state the rest of the time? No. The state of the service reflects transitions accurately. It is just JS code expectation are not synchronized with the service status changes. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:108: // TODO(alexeypa): remove this variable once JS interafce supports callbacks. On 2012/03/30 22:11:01, Wez wrote: > typo: interafce Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:125: On 2012/03/30 22:11:01, Wez wrote: > nit: Personally I'd lose the {} on the if, and this blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:131: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for blank line. I replaced it with a comment. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:139: IDaemonControl* control = NULL; On 2012/03/30 22:11:01, Wez wrote: > This doesn't seem to be used? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:160: // N.B. The UI message loop is required for the single threaded COM apartment. On 2012/03/30 22:11:01, Wez wrote: > typo: The UI ... is required for ... -> COM must be run on a UI message loop > ... Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:162: if (!worker_thread_.StartWithOptions(thread_options)) { On 2012/03/30 22:11:01, Wez wrote: > Since ComThread doesn't make sense unless started with a UI message loop, can we > make it default to one instead of passing options here? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:175: // that can receive APC callbacks. On 2012/03/30 22:11:01, Wez wrote: > nit: Suggest "Make the thread alertable, so we can switch to APC notifications > rather than polling, "? Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:186: // supports callbacks. On 2012/03/30 22:11:01, Wez wrote: > nit: remove ... -> Remove ... Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:191: // TODO(alexeypa): remove this hack once JS nicely reports errors. On 2012/03/30 22:11:01, Wez wrote: > nit: It's not immediately obvious what the hack is here. :) I'm sorry. It is there. Look more closely. :-) https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:227: // TODO(alexeypa): implement on-demand installation. On 2012/03/30 22:11:01, Wez wrote: > typo: implement ... -> Implement ... Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:227: // TODO(alexeypa): implement on-demand installation. On 2012/03/30 22:11:01, Wez wrote: > nit: We generally prefer to create TODOs with an associated bug, if possible, > and refer to the bug-Id in the TODO. While being a good practice (I presume) I haven't seen many TODO comments mentioning any bug number. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:248: state_ == STATE_STOPPING) { On 2012/03/30 22:11:01, Wez wrote: > These are cached states; isn't there a risk that something has started the > service since we read them? Is there any danger in posting a stop control when > the service isn't in one of these states? There is no danger. The state is inherently out-on-sync. This is just a precaution making JS logic a little bit more reliable. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:249: On 2012/03/30 22:11:01, Wez wrote: > nit: Errant blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:259: DWORD DaemonControllerWin::OpenService(ScopedScHandle* service_out) { On 2012/03/30 22:11:01, Wez wrote: > nit: This function looks oddly familiar from ElevatedController... ;) There is little benefit in sharing the same implementation of the function between those two places at the moment. I don't really want to introduce a whole file for them. Not until the code is a little bit more stable. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:296: BSTR host_config = NULL; On 2012/03/30 22:11:01, Wez wrote: > Is there no managed container for BSTR? There is. It drags ATL. We don't want it here. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:323: On 2012/03/30 22:11:01, Wez wrote: > nit: Errant blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:328: last_error_ = hr; On 2012/03/30 22:11:01, Wez wrote: > Why do we lock when setting |last_error_|? Surely if someone is querying > asynchronously then there's going to be a race in the states they see? Race is fine. Corruption (state_ is not synched with last_error_ for instance) is not fine. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:332: // Set the configuration file On 2012/03/30 22:11:01, Wez wrote: > Suggest "Store the configuration." Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:345: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:354: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:371: On 2012/03/30 22:11:01, Wez wrote: > nit: Blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:381: On 2012/03/30 22:11:01, Wez wrote: > nit: Blank line. Done. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:385: On 2012/03/30 22:11:01, Wez wrote: > nit: No need for these blank lines. My eyes hurt. It is unreadable. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/plugin... remoting/host/plugin/daemon_controller_win.cc:389: break; On 2012/03/30 22:11:01, Wez wrote: > nit: Nor for this break, or the ones below. See above.
https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:113: // Build the filtered config. On 2012/03/30 23:47:09, alexeypa wrote: > On 2012/03/30 22:11:01, Wez wrote: > > Why? What is the purpose of the filtering? > > This matched Mac implementation. GetConfig() is documented to return only certain keys, and not to include private information. That's the purpose of the filtering, if that helps.
On 2012/03/31 00:48:34, Lambros wrote: > https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... > File remoting/host/elevated_controller_win.cc (right): > > https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... > remoting/host/elevated_controller_win.cc:113: // Build the filtered config. > On 2012/03/30 23:47:09, alexeypa wrote: > > On 2012/03/30 22:11:01, Wez wrote: > > > Why? What is the purpose of the filtering? > > > > This matched Mac implementation. > GetConfig() is documented to return only certain keys, and not to include > private information. That's the purpose of the filtering, if that helps. Yes, GetConfig() should only return certain fields of the config. This is useful in two cases: - prevent user A from reading auth token used by user B, - prevent malicious apps accessing the auth tokens they shouldn't be allowed to.
Thanks for dealing with [most of] my comments! https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:113: // Build the filtered config. On 2012/03/30 23:47:09, alexeypa wrote: > On 2012/03/30 22:11:01, Wez wrote: > > Why? What is the purpose of the filtering? > > This matched Mac implementation. Right; my point is that the comment should clarify that e.g. "GetConfig is defined to return only a sub-set of the configuration." or "Filter the configuration (see GetConfig definition)." https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:196: return hr; On 2012/03/30 23:47:09, alexeypa wrote: > On 2012/03/30 22:11:01, Wez wrote: > > Do you actually want to return potentially arbitrary return-codes from the > > service control manager from these methods? It might be cleaner to continue to > > log the error with LOG_GETLASTERROR() as you do below but return something > > better defined to the caller. > > I don't think so. Something better defined is "operation failed"/"succeeded". OS > codes deliver this message and provide a possibility to display a more > meaningful error message to the user (potentially). > > > I assume you're converting Win32->HRESULT to avoid GetLastError() getting > > trampled during logging. > > No. COM methods should return HRESULT error by convention. The word "here" was missing from my sentence; I thought it might be cleaner to do the conversion later, except for the GetLastError() trampling issue. > > One slight oddity of this arrangement is that APIs > > returning an invalid value but leaving GetLastError() set to zero will be > > treated as having succeeded. > > GetLastError() is purely Win32 concept. It usually does not apply to > intra-application interfaces. It also typically applies only to functions > returned a boolean instead of error code. Yes; my point is that an API could return FALSE (i.e. error) but GetLastError() be ERROR_SUCCESS, in which case this code will behave as if the call had succeeded. Of course APIs shouldn't fail in that way...
FYI. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... File remoting/host/elevated_controller_win.cc (right): https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:113: // Build the filtered config. On 2012/04/01 01:25:27, Wez wrote: > Right; my point is that the comment should clarify that e.g. "GetConfig is > defined to return only a sub-set of the configuration." or "Filter the > configuration (see GetConfig definition)." Oh, well. I'll try to address this in one of the follow up CL. https://chromiumcodereview.appspot.com/9953002/diff/6005/remoting/host/elevat... remoting/host/elevated_controller_win.cc:196: return hr; On 2012/04/01 01:25:27, Wez wrote: > The word "here" was missing from my sentence; I thought it might be cleaner to > do the conversion later, except for the GetLastError() trampling issue. That is not the only reason. I don't think that using GetLastError() for anything that is not Win32 API is a good idea. There should be a single source of the error status to avoid issues like the one you are describing below. > Yes; my point is that an API could return FALSE (i.e. error) but GetLastError() > be ERROR_SUCCESS, in which case this code will behave as if the call had > succeeded. Of course APIs shouldn't fail in that way... Well, this can definitely happen in practice but it happens so rare that it can be fixed on case-by-case basic. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
