Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6660001:
Getting service process on Mac to handle having things moved/changed underneath it. (Closed)
|
Created:
9 years, 9 months ago by dmac Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGetting service process on Mac to handle having things moved/changed underneath it.
BUG=74983
TEST=See http://code.google.com/p/chromium/issues/detail?id=74983#c16
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78967
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixed up phajdan's comments, got things working properly #
Total comments: 63
Patch Set 3 : Move over to FilePathWatcher #
Total comments: 66
Patch Set 4 : Revised based on comments #Patch Set 5 : missed a comment #Patch Set 6 : fix up bad instance reset #
Total comments: 16
Patch Set 7 : Fix up last of the nits #Patch Set 8 : polishing the chrome #
Messages
Total messages: 19 (0 generated)
Work in progress (should have it done today). Just wanted to let you guys know where I was.
Drive-by with testing comments. Please let me do another review after fixing those. http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... File base/file_path_component_watcher_unittest.cc (right): http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... base/file_path_component_watcher_unittest.cc:70: scoped_ptr<ScopedTempDir> temp_dir_; nit: No need for scoped_ptr, really. The entire text fixture is created fresh and destroyed for each test. http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... base/file_path_component_watcher_unittest.cc:85: class ChangeAttr : public TestTask { nit: It's going to be simpler to use NewRunnableFunction instead of those classes. I've tried it in a very similar code.
Thanks Pawel, I think I addressed all your comments. On 2011/03/10 16:02:04, Paweł Hajdan Jr. wrote: > Drive-by with testing comments. Please let me do another review after fixing > those. > > http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... > File base/file_path_component_watcher_unittest.cc (right): > > http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... > base/file_path_component_watcher_unittest.cc:70: scoped_ptr<ScopedTempDir> > temp_dir_; > nit: No need for scoped_ptr, really. The entire text fixture is created fresh > and destroyed for each test. > > http://codereview.chromium.org/6660001/diff/1/base/file_path_component_watche... > base/file_path_component_watcher_unittest.cc:85: class ChangeAttr : public > TestTask { > nit: It's going to be simpler to use NewRunnableFunction instead of those > classes. I've tried it in a very similar code.
Scott, Sanjeev: I think this is ready for review. Mark/TVL: I would appreciate you guys giving the base/mac stuff a going over if you have a chance. Also, if you would look at the service_process_mac_util stuff I would appreciate that as well. There's some reasonably tricky launchd/file system interactions going on there.
Thanks again for doing this. The logic looks fine to me. I have a few comments but they are nits. Of course, I am not qualified to review the Mac-specific stuff. And I will write a Windows implementation for FilePathComponentWatcher one of these days. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher_linux.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_linux.cc:18: return false; NOTIMPLEMENTED() here? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher_win.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_win.cc:18: return false; NOTIMPLEMENTED() http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:197: bool ServiceProcessState::SignalReadyPlatformSpecific() { It seems kind of cheating to use the SignalReady to do something that has nothing to do with signalling ready. Perhaps call this Initialize() or something like that?
Code I commented in the drive-by LGTM, thanks.
http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher.h (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:5: // This module provides a way to monitor a filepath for changes in the path. chrome/browser/file_path_watcher/file_path_watcher.h...
Might also want to check with brett on putting this in base vs. some place higher. I believe there is some effort to stop the tide of stuff going into base.
Are you imminently planning on implementing the file watcher on the other platforms? If not, I would just make it a mac specific thing and put it in your directory, I'd prefer not to have long-lived code that looks like it's cross platform but doesn't do anything.
Actually having looked at FilePathWatcher, and realizing what it is being used for, I am considering attempting to merge the two classes. That being said, I think we will eventually need a linux implementation at the very least if we are considering moving linux over to DBUS. I'll let Sanjeev speak to a windows impl. On Fri, Mar 11, 2011 at 10:31 AM, <brettw@chromium.org> wrote: > Are you imminently planning on implementing the file watcher on the other > platforms? If not, I would just make it a mac specific thing and put it in > your > directory, I'd prefer not to have long-lived code that looks like it's cross > platform but doesn't do anything. > > http://codereview.chromium.org/6660001/ >
http://codereview.chromium.org/6660001/diff/8001/base/base.gyp File base/base.gyp (right): http://codereview.chromium.org/6660001/diff/8001/base/base.gyp#newcode87 base/base.gyp:87: 'file_path_component_watcher_unittest.cc', I agree with what Brett said about this. I’d make this test Mac-only too (and get rid of the #ifdefs from it). http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher.h (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. What year is it? Throughout the entire change, the added files should say 2011. I don’t care if you don’t touch modified files. In fact, I prefer if you don’t. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:5: // This module provides a way to monitor a filepath for changes in the path. TVL wrote: > chrome/browser/file_path_watcher/file_path_watcher.h... I think dmac’s comment below covers that. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:5: // This module provides a way to monitor a filepath for changes in the path. filepath is two words. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:19: // FilePathWatcher, in that FilePathWatcher watches a single file, and it's "single file, and it's" -> "single file and its" http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:26: // corresponding FileWatcher object to prevent a reference cycle. What’s a FileWatcher object? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:33: // Called when platform specific code detected an error. The watcher will Blank line before. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:43: bool Watch(const FilePath& path, Delegate* delegate) WARN_UNUSED_RESULT; Are you supposed to be able to call Watch twice on the same object? Probably not, but nothing in the code seems to check this or care. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:45: // Used internally to encapsulate different members on different platforms. I’m going to hold off on a thorough review of the interface-implementation separation until you combine things into a single implementation-only thing as Brett mentioned, or explain why you’d prefer not to do so. No point in reviewing this aspect now if it’s going to change. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:47: : public base::RefCountedThreadSafe<PlatformDelegate> { What I just said notwithstanding… Why is the platform delegate reference-counted? And is the Cancel thing really necessary? (Maybe it’s not if you didn’t have this be reference-counted.) http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:55: // Stop watching. This is called from FilePathComponentWatcher's dtor Spell things out. Destructor, not dtor. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher.h:67: } // namespace } // namespace base http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher_linux.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_linux.cc:22: } // namespace } // namespace (two spaces between } and //.) http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher_mac.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:11: #include <vector> Blank line before. C++ system headers are in a separate section than C system headers. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:20: // Mac-specific file watcher implementation based on kqueue. This sounds like it’s going to eat up a lot of file descriptors. :) http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:24: FilePathComponentWatcherImpl(FilePathComponentWatcher* watcher) explicit http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:26: virtual ~FilePathComponentWatcherImpl() {} Not needed? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:46: if(close(fd) != 0) { if<space>(close… http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:51: void CloseFileDescriptors(std::vector<int>& descriptors) { It’s uncommon (and therefore unexpected) to pass non-const references. This’ll work with a pointer instead. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:56: bool OpenFileDescriptorsForPath(FilePath path, std::vector<int>* descriptors) { Start this function with CloseFileDescriptors(*descriptors); descriptors->clear(); to ensure you’re working with clean state. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:80: } else { No else needed after a return. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:88: CFOptionFlags callBackTypes, Match our style: callback_types. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:97: void FilePathComponentWatcherImpl::OnFilePathChanged(CFFileDescriptorRef fd) { I would find this class implementation easier to read if OnFilePathChanged followed Watch. I know that this will make the method definition order not match the order that things were declared in, but I think it’s a net win. (If you wind up making this Mac-specific and combine things, you’ll get rid of the platform delegate, and then you can even have things in the right temporal order in both the declaration and implementations.) http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:102: FilePath new_path; Don’t declare this ’til you use it, which looks like it’d be right before the fcntl. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:108: } else if (delegate_) { When will there not be a delegate? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:109: char path[MAXPATHLEN]; Anything involving MAXPATHLEN is a hack. I think we’ve discussed this before. This doesn’t make me happy, but evidently it’s the way to do this. Bad, bad interface. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:110: int path_fd = file_descriptors_[file_descriptors_.size() - 1]; You can just use file_descriptors_.back(). http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:114: delegate_->OnFilePathComponentsChanged(watcher_, path_, new_path); new_path may be empty here, which I’m assuming is your signal that it’s been deleted. You should document that in the delegate declaration in the header. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:128: LOG(ERROR) << "FileDescriptorsForPath: " << path.value(); OpenFileDescriptorsForPath? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:134: scoped_ptr<struct kevent> changes(reinterpret_cast<struct kevent*>( scoped_ptr is intended to be used with objects allocated by |operator new|, and frees them with |delete|. calloc ≠ new. You should use std::vector<struct kevent> changes(count). When you need to call EV_SET, use &changes[i]. When you need to pass the whole thing to kevent, use &changes[0]. (Ensure that count > 0, obviously.) http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:137: for(size_t i = 0; i != count; ++i) { for<space>(size_t… http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:143: if (HANDLE_EINTR(kevent(kq, changes.get(), count, NULL, 0, NULL)) == -1) { This confused me for a second, because I saw the NULL last argument, which usually means “wait forever,” and I was going to ask when you expected this to return. It wasn’t until I realized that you were asking for 0 changes that I realized it would return immediately, and you’re just using this to get your changes into the kqueue but not actually wait. Might be worth a comment. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:149: memset(&context, 0, sizeof(context)); Or just CFFileDescriptorContext context = {0}; http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:158: CFRunLoopAddSource(CFRunLoopGetMain(), The run loop source leaks because nobody ever removes it from the run loop, even after the kqueue it’s monitoring is gone. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:158: CFRunLoopAddSource(CFRunLoopGetMain(), I usually use CFRunLoopGetCurrent. Do you have a reason to prefer one over another in this case? http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:159: source, This didn’t all fit on one line? http://codereview.chromium.org/6660001/diff/8001/base/file_util.h File base/file_util.h (right): http://codereview.chromium.org/6660001/diff/8001/base/file_util.h#newcode305 base/file_util.h:305: // Moves the file to the trash. You’ve only used this in a test. Just define the function in the test. Don’t bloat up the application for something we don’t need to have there. http://codereview.chromium.org/6660001/diff/8001/base/file_util_posix.cc File base/file_util_posix.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_util_posix.cc#newc... base/file_util_posix.cc:538: bool GetInodeAndDevice(const FilePath& path, ino_t* inode, dev_t* dev) { Nobody was using this function? http://codereview.chromium.org/6660001/diff/8001/base/file_util_posix.cc#newc... base/file_util_posix.cc:538: bool GetInodeAndDevice(const FilePath& path, ino_t* inode, dev_t* dev) { GetDeviceAndInode would make more sense to me. http://codereview.chromium.org/6660001/diff/8001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_util_unittest.cc#n... base/file_util_unittest.cc:1847: } Don’t leave it in the trash. Delete it. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:9: #include <syslog.h> Why? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:117: socket_path = base::SysNSStringToUTF8(ns_socket_path); Uh-oh, seems that the existing code isn’t using -[NSString fileSystemRepresentation] where it should be. File a follow-up bug? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:203: FilePath executable_path = FilePath(base::SysCFStringRefToUTF8(exe_path)); This is wrong. Use CFStringGetFileSystemRepresentation or -[NSString fileSystemRepresentation]. The NSString method is less cumbersome and looks like it’d be easy to use here. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:357: // Can we execute it? Remember, no “we.” This is better: // Is it executable? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:367: needs_plist_rewrite = true; Judging from this logic, it seems that needs shutdown == needs_plist_deletion and needs_restart == needs_plist_rewrite True? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:369: // Nothing exciting happened. Let's continue to ask for events. s/Let's c/C/ http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:370: if (!watcher->Watch(new_path, this)) { OK, so it *is* possible to call Watch on an existing object, but only after it’s called out? ? Is it a better design (potentially losing fewer events) if the watcher is allowed to keep watching until cancelled? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:377: = [NSMutableDictionary dictionaryWithContentsOfURL:plist_url]; Put the = on the preceding line, and indent this one properly for a continuation line. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:379: NSString* ns_new_path = base::SysUTF8ToNSString(new_path.value()); Why not just +[NSString stringWithUTF8String:]? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:398: std::string plist_path(base::SysNSStringToUTF8([plist_url path])); No, use [plist_url fileSystemRepresentation]. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:408: } else if (needs_restart) { This can just be |else|, look at the enclosing conditional. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:409: VLOG(1) << "Restarting using launchctl"; I assume that as soon as you run launchctl unload, this process will be killed? Worth a comment. I assume that’s also why you’re using a one-liner shell script to do this and starting it in a new process group? Worth a comment. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:410: argv.push_back("/bin/sh"); /bin/bash if you’re using --noprofile. http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:414: "/bin/launchctl unload -S %s '%s';/bin/launchctl load -S %s '%s';", The only way I’ll let you pass arbitrary arguments to a shell script is if you properly sanitize them. Using '%s' protects you against most nasties, but what if plist_path winds up with a ' in it? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:426: } What are you expecting to happen here? Be killed? What do you do? Return to the run loop? Do you have any cleanup that needs to be done? Can you just exit? Can you sleep forever expecting to be killed? http://codereview.chromium.org/6660001/diff/8001/chrome/common/service_proces... chrome/common/service_process_util_mac.mm:429: Blank line at EOF. http://codereview.chromium.org/6660001/diff/8001/chrome/service/service_proce... File chrome/service/service_process.cc (right): http://codereview.chromium.org/6660001/diff/8001/chrome/service/service_proce... chrome/service/service_process.cc:8: #include <syslog.h> ?
http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... File base/file_path_component_watcher_mac.cc (right): http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:26: virtual ~FilePathComponentWatcherImpl() {} What about closing the file descriptors on exit, if they're still open? Otherwise, a failure setting up the kernel event watchers could leak FDs. http://codereview.chromium.org/6660001/diff/8001/base/file_path_component_wat... base/file_path_component_watcher_mac.cc:108: } else if (delegate_) { If delegate_ is NULL, I would think we'd want to DCHECK (CHECK?) in Watch, since that effectively turns this into a no-op - save the FDs.
OK, I think I've dealt with all the issues brought up above. I have moved FilePathWatcher from chrome/browser/common to content/common, and modified it so I can use it in a separate CL (http://codereview.chromium.org/6670081/). Major new modifications in this CL: Creation of launchd class which abstracts out all the interactions with launchd so that we can unit test them somewhat. Changed ServiceProcessState from being a singleton to being injected. If somebody could take a look at this ASAP I would appreciate it as http://crbug.com/74983 is marked as a beta blocker.
http://codereview.chromium.org/6660001/diff/18004/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): http://codereview.chromium.org/6660001/diff/18004/chrome/chrome_common.gypi#n... chrome/chrome_common.gypi:201: 'common/launchd_mac.h', Sort: h < mm http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h File chrome/common/launchd_mac.h (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:13: class Launchd { I don’t like this as a class name, especially without a namespace (which we’re suposed to generally be using these days). This is more of a LaunchdInterface, perhaps? http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:22: // to be included. In order to get away with this, you need to have a COMPILE_ASSERT in the .mm file that ensures that your own constants are equal to Foundation’s. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:24: User = 1, // ~/Library/Launch* To make it obvious that these are bits, I would write: User = 1 << 0, Local = 1 << 1, Network = 1 << 2, System = 1 << 3 http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:51: // Used by a process controlled by launchd to kill oneself. itself http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:55: // send us SIGTERM. This call will return, but a SIGTERM should occur shortly. send us SIGTERM -> send this process a SIGTERM http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:55: // send us SIGTERM. This call will return, but a SIGTERM should occur shortly. occur -> be received http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:65: // send us SIGTERM and then reload us. This call will return, but a SIGTERM Revise comment as above: itself, send this process a, be received. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:74: virtual CFMutableDictionaryRef ReadLaunchdPlist(Domain domain, Your CFDictionaryRef-returning functions above were named with “Copy” in the name, so they didn’t need any special ownership documentation. There’s no Copy or Create or Get here, and there’s no comment telling me who owns the dictionary. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:86: virtual bool DeleteLaunchDPlist(Domain domain, Type type, CFStringRef name); You have ReadLaunchdPlist and WriteLaunchdPlist (lowercase D), and a lot of other things that refer to launchd with a lowercase d that aren’t related to plists, but then you have DeleteLaunchDPlist (capital D). Be consistent. I prefer the lowercase D. Given the class name, you probably don’t even need to put Launchd in each method’s name. You’ll probably have something like LaunchdInterface* launchd_interface = LaunchdInterface::GetInstance(); launchd_interface->DeletePlist(...), which is more than sufficient. I doubt launchd_interface->DeleteLaunchdPlist(...) would be any better. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:93: ScopedInstance(Launchd* instance) { explicit http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:97: Launchd::SetInstance(NULL); This doesn’t reset the initial state. That’s probably fine for what you’re doing here. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.mm File chrome/common/launchd_mac.mm (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:22: CFURLCopyFileSystemPath(url, kCFURLPOSIXPathStyle)); Might path be NULL? If url is bad in some way, like not being a file URL, perhaps? http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:24: // TODO(dmaclach): Turn this into a more general utility This comment applies to the entire function, right? Put it at the top of the “meat” inside the {braces}. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:26: CFIndex max = CFStringGetMaximumSizeOfFileSystemRepresentation(path); I’d just use -[NSString fileSystemRepresentation] here so you don’t need to worry about your own buffer. return std::string([(NSString*)path.get() fileSystemRepresentation]); You may need a ScopedNSAutoreleasePool to get away with this depending on where it runs. (The fact that we already have a one-line way to do this provided by Foundation is why I figured your TODO applied to the entire CFURL-to-string function and not just the CFString-to-string portion, but I see your bug is titled for the CFString-to-string conversion, so I think it’d be invalid/wontfix as-is. If you want a CFURL-to-string utility function in something like mac_util, that might be a different story.) http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:32: CFURLRef CopyPListURL(Launchd::Domain domain, CopyPlistURL, lowercase L, as you’ve done elsewhere. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:38: domain, Come up with a classier way to break these lines up. How about: NSArray* library_paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, domain, YES); Note the use of YES in a Foundation call instead of true. It makes no difference here in the object code, but you should be in the habit of using YES when the prototype says BOOL. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:46: [library_path stringByAppendingPathComponent:launch_dir_name]; 4-space the continuation line. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:57: NSString* plist_file_path = goes on this line, not the next. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:60: NSURL *url = [[NSURL alloc] initFileURLWithPath:plist_file_path You’ve been putting the *s on the type and not the variable name, stick with that for consistency. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:65: } } // namespace http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:68: bool Launchd::g_set_to_singleton_ = false; I don’t see why you need this. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:86: return GTMCopyLaunchdExports(); I haven’t read this code or its API documentation, I’m assuming it’s correct. Same through RemoveLaunchdJob. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:106: std::string path(FileSystemPathFromCFURL(cf_url)); Isn’t it kind of obnoxious to use CopyPlistURL, which deals with CFStrings and then turns it into a CFURL at the last moment, and then have to get a string back from the CFURL by calling FileSystemPathFromCFURL? http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:135: session_type.c_str(), path.c_str(), Didn’t I mention doing proper sanitization and escaping on shell arguments in a previous review? You need to do that for all arguments here. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:152: NSMutableDictionary* plist = goes on this line. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:153: = [[NSMutableDictionary alloc] initWithContentsOfURL:ns_url]; Continuation line uses a 4-space indent. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:153: = [[NSMutableDictionary alloc] initWithContentsOfURL:ns_url]; OK, I see, the caller owns the returned dictionary. Rename the function or document the ownership in the header (as I mentioned in the comment I made in the header). http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:165: dict)); dict fits on the previous line for sure. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:166: Boolean good = false; Just bool, not Boolean. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:189: Dump the blank line at EOF. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:316: CFIndex max = CFStringGetMaximumSizeOfFileSystemRepresentation(exe_path); This is an .mm file, just use NSString as discussed before. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:325: shutdown_task->Run(); Are the error cases here really the right place to run (and delete) the shutdown_task? http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:355: 0, Just put all of these on one line. No sense in making them so vertical when you’re not even going to use the vertical space to say what the null arguments are supposed to do. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:408: if (needs_shutdown || needs_restart) { This is a long function. Even if you can’t or don’t want to break it up into smaller functions, you should at least put some blank lines (and maybe comments if needed—you might not in this function) between portions that do different groups of major work. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:467: Blank line at EOF. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... File chrome/common/service_process_util_unittest.cc (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:225: #include <CoreFoundation/CoreFoundation.h> I didn’t yet read the test.
I think I caught all Mark's nits. http://codereview.chromium.org/6660001/diff/18004/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): http://codereview.chromium.org/6660001/diff/18004/chrome/chrome_common.gypi#n... chrome/chrome_common.gypi:201: 'common/launchd_mac.h', On 2011/03/21 16:50:49, Mark Mentovai wrote: > Sort: h < mm Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h File chrome/common/launchd_mac.h (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:13: class Launchd { On 2011/03/21 16:50:49, Mark Mentovai wrote: > I don’t like this as a class name, especially without a namespace (which we’re > suposed to generally be using these days). > > This is more of a LaunchdInterface, perhaps? Will fix up in a later CL. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:22: // to be included. On 2011/03/21 16:50:49, Mark Mentovai wrote: > In order to get away with this, you need to have a COMPILE_ASSERT in the .mm > file that ensures that your own constants are equal to Foundation’s. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:24: User = 1, // ~/Library/Launch* On 2011/03/21 16:50:49, Mark Mentovai wrote: > To make it obvious that these are bits, I would write: > > User = 1 << 0, > Local = 1 << 1, > Network = 1 << 2, > System = 1 << 3 Foundation definitions have them mapped out as 1,2,4,8 so I'm leaving them to match up. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:51: // Used by a process controlled by launchd to kill oneself. On 2011/03/21 16:50:49, Mark Mentovai wrote: > itself Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:55: // send us SIGTERM. This call will return, but a SIGTERM should occur shortly. On 2011/03/21 16:50:49, Mark Mentovai wrote: > occur -> be received Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:55: // send us SIGTERM. This call will return, but a SIGTERM should occur shortly. On 2011/03/21 16:50:49, Mark Mentovai wrote: > send us SIGTERM -> send this process a SIGTERM Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:65: // send us SIGTERM and then reload us. This call will return, but a SIGTERM On 2011/03/21 16:50:49, Mark Mentovai wrote: > Revise comment as above: itself, send this process a, be received. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:86: virtual bool DeleteLaunchDPlist(Domain domain, Type type, CFStringRef name); On 2011/03/21 16:50:49, Mark Mentovai wrote: > You have ReadLaunchdPlist and WriteLaunchdPlist (lowercase D), and a lot of > other things that refer to launchd with a lowercase d that aren’t related to > plists, but then you have DeleteLaunchDPlist (capital D). Be consistent. I > prefer the lowercase D. > > Given the class name, you probably don’t even need to put Launchd in each > method’s name. You’ll probably have something like LaunchdInterface* > launchd_interface = LaunchdInterface::GetInstance(); > launchd_interface->DeletePlist(...), which is more than sufficient. I doubt > launchd_interface->DeleteLaunchdPlist(...) would be any better. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:93: ScopedInstance(Launchd* instance) { On 2011/03/21 16:50:49, Mark Mentovai wrote: > explicit Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.h... chrome/common/launchd_mac.h:97: Launchd::SetInstance(NULL); On 2011/03/21 16:50:49, Mark Mentovai wrote: > This doesn’t reset the initial state. That’s probably fine for what you’re doing > here. Actually it's exactly what I want ;-) http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.mm File chrome/common/launchd_mac.mm (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:22: CFURLCopyFileSystemPath(url, kCFURLPOSIXPathStyle)); On 2011/03/21 16:50:49, Mark Mentovai wrote: > Might path be NULL? If url is bad in some way, like not being a file URL, > perhaps? Obsolete. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:24: // TODO(dmaclach): Turn this into a more general utility On 2011/03/21 16:50:49, Mark Mentovai wrote: > This comment applies to the entire function, right? Put it at the top of the > “meat” inside the {braces}. Obsolete. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:26: CFIndex max = CFStringGetMaximumSizeOfFileSystemRepresentation(path); On 2011/03/21 16:50:49, Mark Mentovai wrote: > I’d just use -[NSString fileSystemRepresentation] here so you don’t need to > worry about your own buffer. > > return std::string([(NSString*)path.get() fileSystemRepresentation]); > > You may need a ScopedNSAutoreleasePool to get away with this depending on where > it runs. > > (The fact that we already have a one-line way to do this provided by Foundation > is why I figured your TODO applied to the entire CFURL-to-string function and > not just the CFString-to-string portion, but I see your bug is titled for the > CFString-to-string conversion, so I think it’d be invalid/wontfix as-is. If you > want a CFURL-to-string utility function in something like mac_util, that might > be a different story.) Obsolete. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:32: CFURLRef CopyPListURL(Launchd::Domain domain, On 2011/03/21 16:50:49, Mark Mentovai wrote: > CopyPlistURL, lowercase L, as you’ve done elsewhere. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:38: domain, On 2011/03/21 16:50:49, Mark Mentovai wrote: > Come up with a classier way to break these lines up. How about: > > NSArray* library_paths = > NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, domain, YES); > > Note the use of YES in a Foundation call instead of true. It makes no difference > here in the object code, but you should be in the habit of using YES when the > prototype says BOOL. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:46: [library_path stringByAppendingPathComponent:launch_dir_name]; On 2011/03/21 16:50:49, Mark Mentovai wrote: > 4-space the continuation line. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:57: NSString* plist_file_path On 2011/03/21 16:50:49, Mark Mentovai wrote: > = goes on this line, not the next. I hate that. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:60: NSURL *url = [[NSURL alloc] initFileURLWithPath:plist_file_path On 2011/03/21 16:50:49, Mark Mentovai wrote: > You’ve been putting the *s on the type and not the variable name, stick with > that for consistency. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:65: } On 2011/03/21 16:50:49, Mark Mentovai wrote: > } // namespace Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:68: bool Launchd::g_set_to_singleton_ = false; On 2011/03/21 16:50:49, Mark Mentovai wrote: > I don’t see why you need this. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:86: return GTMCopyLaunchdExports(); On 2011/03/21 16:50:49, Mark Mentovai wrote: > I haven’t read this code or its API documentation, I’m assuming it’s correct. > Same through RemoveLaunchdJob. OK.. it's been working for a while now. http://codereview.chromium.org/6660001/diff/18004/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:106: std::string path(FileSystemPathFromCFURL(cf_url)); On 2011/03/21 16:50:49, Mark Mentovai wrote: > Isn’t it kind of obnoxious to use CopyPlistURL, which deals with CFStrings and > then turns it into a CFURL at the last moment, and then have to get a string > back from the CFURL by calling FileSystemPathFromCFURL? Done. It made sense in the earlier CL because some places it was used as a CFURLRef and others as a NSURL. Recent changes tipped the balance back to NSURL. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:316: CFIndex max = CFStringGetMaximumSizeOfFileSystemRepresentation(exe_path); On 2011/03/21 16:50:49, Mark Mentovai wrote: > This is an .mm file, just use NSString as discussed before. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:325: shutdown_task->Run(); On 2011/03/21 16:50:49, Mark Mentovai wrote: > Are the error cases here really the right place to run (and delete) the > shutdown_task? I completely removed the error case for now. I will log a bug to cleanup, but the wiring is too complicated to put it in now. http://code.google.com/p/chromium/issues/detail?id=76987 http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:355: 0, On 2011/03/21 16:50:49, Mark Mentovai wrote: > Just put all of these on one line. No sense in making them so vertical when > you’re not even going to use the vertical space to say what the null arguments > are supposed to do. Functionized http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:408: if (needs_shutdown || needs_restart) { On 2011/03/21 16:50:49, Mark Mentovai wrote: > This is a long function. > > Even if you can’t or don’t want to break it up into smaller functions, you > should at least put some blank lines (and maybe comments if needed—you might not > in this function) between portions that do different groups of major work. It's been broken down a bit, and I feel it's pretty readable at this point. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:467: On 2011/03/21 16:50:49, Mark Mentovai wrote: > Blank line at EOF. Done. http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... File chrome/common/service_process_util_unittest.cc (right): http://codereview.chromium.org/6660001/diff/18004/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:225: #include <CoreFoundation/CoreFoundation.h> On 2011/03/21 16:50:49, Mark Mentovai wrote: > I didn’t yet read the test. ACK.
The code LGTM from a launchd and plist perspective (you should get at least one more OK); what's up with the trybot hang?
LGTM. I really like the test menu in your TEST= line. All of the remaining requested changes are minor and not controversial, and you can make them and check in. The two comments I had about using private: for data in the classes in the unit test can be addressed another time. I trust you’re good for it, and since you plan on circling back to take care of other rough edges (especially with the testing interface), I don’t think you need to make those changes now either. Nice work, Dave. http://codereview.chromium.org/6660001/diff/26005/chrome/common/launchd_mac.mm File chrome/common/launchd_mac.mm (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:60: static_cast<int>(NSUserDomainMask), foundation_value_changed); You should use a different msg (second parameter) for each of the four COMPILE_ASSERTs. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:81: private: Indent by 1. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_unittest.cc (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:254: delete_called_(false) Nit: { goes on this line. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:330: FilePath file_; OK for now, but the data should really be in a private: section and you should provide accessors as needed. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:380: const char *data = "#! testbundle"; Put a \n at the end of this string. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:390: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\r" Your lines should end with \n throughout this string, not \r. This isn’t the Classic OS anymore! http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:425: ScopedTempDir temp_dir_; Make private when able, as in the class above. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_win.cc (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_win.cc:131: scoped_ptr<Task> scoped_shutdown_task(shutdown_task); Good catch.
http://codereview.chromium.org/6660001/diff/26005/chrome/common/launchd_mac.mm File chrome/common/launchd_mac.mm (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/launchd_mac.m... chrome/common/launchd_mac.mm:60: static_cast<int>(NSUserDomainMask), foundation_value_changed); On 2011/03/22 00:48:24, Mark Mentovai wrote: > You should use a different msg (second parameter) for each of the four > COMPILE_ASSERTs. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_mac.mm (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_mac.mm:81: private: On 2011/03/22 00:48:24, Mark Mentovai wrote: > Indent by 1. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_unittest.cc (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:254: delete_called_(false) On 2011/03/22 00:48:24, Mark Mentovai wrote: > Nit: { goes on this line. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:330: FilePath file_; On 2011/03/22 00:48:24, Mark Mentovai wrote: > OK for now, but the data should really be in a private: section and you should > provide accessors as needed. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:380: const char *data = "#! testbundle"; On 2011/03/22 00:48:24, Mark Mentovai wrote: > Put a \n at the end of this string. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:390: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\r" On 2011/03/22 00:48:24, Mark Mentovai wrote: > Your lines should end with \n throughout this string, not \r. This isn’t the > Classic OS anymore! I'm a classy kind of guy. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_unittest.cc:425: ScopedTempDir temp_dir_; On 2011/03/22 00:48:24, Mark Mentovai wrote: > Make private when able, as in the class above. Done. http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... File chrome/common/service_process_util_win.cc (right): http://codereview.chromium.org/6660001/diff/26005/chrome/common/service_proce... chrome/common/service_process_util_win.cc:131: scoped_ptr<Task> scoped_shutdown_task(shutdown_task); On 2011/03/22 00:48:24, Mark Mentovai wrote: > Good catch. thx
LGTM again for what it’s worth. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
