Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5970015:
Add multi-process notification class. (Closed)
|
Created:
9 years, 11 months ago by dmac Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd multi-process notification class.
This is a platform abstraction for a notification that can be sent between processes.
Currently only implemented on Mac. Windows and Linux will be done in a future CL.
BUG=NONE
TEST=BUILD
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70629
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71418
Patch Set 1 #
Total comments: 42
Patch Set 2 : git status #Patch Set 3 : Added missing file #
Total comments: 22
Patch Set 4 : Added domain to listener callback #Patch Set 5 : Added documentation to domains #Patch Set 6 : more comments added #Patch Set 7 : Responded to Mark's comments #
Total comments: 1
Patch Set 8 : Fixed up mac side so that it works on 10.5 as well. #
Total comments: 22
Patch Set 9 : Turn off force leopard flag #Patch Set 10 : Now receives notifications on the thread that calls Start on the listener #
Total comments: 8
Patch Set 11 : Disabled Leopard Thread on 10.6, added comment #Patch Set 12 : fixed up with the end of mark's comments #Patch Set 13 : small comment fix #Patch Set 14 : moved to chrome/browser from chrome/common #Patch Set 15 : Fixed up bad tabs #Messages
Total messages: 20 (0 generated)
If you could take a look, I'd appreciate it.
Drive-by with minor test comments. Glad to see TestTimeouts being used. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_unittest.cc (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:42: scoped_ptr<MessageLoop> io_loop; Why not just stack-allocate the loop? You can RunAllPending in TearDown if you wish. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:69: new QuitTask()); Is there any reason for not using MessageLoop::QuitTask here? IIRC, Quit can only be called on the current message loop, so the MessageLoopForIO code in the custom QuitTask should make no difference. If the above is not true, it at least deserves a comment.
http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification.h (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:19: // platform (so on MacOSX a "Happy" notification will become Mac OS X is generally spelled with three spaces. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:34: virtual void OnNotificationReceived(const std::string& name) = 0; Wanna declare a virtual destructor? http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:50: public: Bogus indentation throughout this class. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:51: explicit PerformTaskOnNotification(Task* task) : task_(task) { Should this class really define its implementation in this header? Should there be a multi_process_notification.cc where the implementation could go? Maybe talk to erg, who took a lot of code out of headers. I don’t know exactly what his threshold criteria were, but this may be borderline. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_linux.cc (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:11: NOTIMPLEMENTED(); #include "base/logging.h" http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:19: virtual ~ListenerImpl(); Q. Why is this virtual? A. Because Start() is virtual. Q. Why is Start() virtual? A. ? For the purposes of this class as it stands now, I don’t think you need to declare any destructor at all (or define any destructor, lines 33-34). http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:42: Listener::Listener(const std::string& name, Listener::Delegate* delegate) Wait a sec. This is how you’re doing your factory? Then does Listener actually need to have anything declared virtual? I get the sense that you wrote this with the platform split being handled differently, and then changed it up once or twice along the way. No biggie, but let’s make sure the split is handled well, and get rid of the leftover stuff from the rewrites. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:46: Listener::~Listener() { } Put the } on its own line. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. What will you use this for? How is it supposed to operate with multiple running Chromes? Consider two Chromes running out of different profiles in the same user account, or two Chromes running in different user accounts under fast user switching. Keep in mind that I’m not going to use your answers to block you from checking in an incomplete design, I just want to get an idea of how this will be used, what the constraints are, how it currently deals with those constraints, and how it might ultimately be able to deal. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:7: #include <notify.h> <notify.h> and <Foundation/Foundation.h> can be in the same group, no need for a blank line separating them. Once they’re in the same group, F sorts before n. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:83: return io_loop->WatchFileDescriptor(fd_, true, MessageLoopForIO::WATCH_READ, Do you ever need to undo this? watcher_ will disappear when ListenerImpl is destroyed, but I don’t know whether something else makes io_loop aware of that or if you need to do it yourself. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:90: if (read(fd_, &token, sizeof(token)) >= 0) { HANDLE_EINTR from base/eintr_wrapper.h. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:91: // Have to swap to native endianness <rdar://problem/5352778>. Link to the problem: good. Link to the problem that I can’t see: bad. Open Radar and Google Search aren’t fruitful. I’d appreciate a better description of the problem. Is this something that will be fixed in some future OS release? If it is, will ntohl stop being the right thing to do? If so, is there some sort of test we can write so that we’ll be notified pretty quickly if this workaround (if that’s what it is) becomes obsolete? http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:109: Listener::~Listener() { } {} on separate lines, along with whatever other questions and comments I made in the Linux file that apply to this one too. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_unittest.cc (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:5: #if defined(OS_MACOSX) This is ineffective because you don’t have build/build_config.h yet. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:22: static const char kStartedNotificationName[]; These can be file-static or in an anonymous namespace in this file. There’s no reason for them to be public class members, right? (I get that you couldn’t do private class members because of MultiProcessNotificationMain.) http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:39: Extra blank line. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:42: scoped_ptr<MessageLoop> io_loop; This should be io_loop_. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:110: Blank line at EOF.
Some significant changes here. Thanks for the comments. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification.h (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:19: // platform (so on MacOSX a "Happy" notification will become On 2011/01/04 18:19:33, Mark Mentovai wrote: > Mac OS X is generally spelled with three spaces. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:34: virtual void OnNotificationReceived(const std::string& name) = 0; On 2011/01/04 18:19:33, Mark Mentovai wrote: > Wanna declare a virtual destructor? Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:50: public: On 2011/01/04 18:19:33, Mark Mentovai wrote: > Bogus indentation throughout this class. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification.h:51: explicit PerformTaskOnNotification(Task* task) : task_(task) { On 2011/01/04 18:19:33, Mark Mentovai wrote: > Should this class really define its implementation in this header? Should there > be a multi_process_notification.cc where the implementation could go? Maybe talk > to erg, who took a lot of code out of headers. I don’t know exactly what his > threshold criteria were, but this may be borderline. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_linux.cc (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:11: NOTIMPLEMENTED(); On 2011/01/04 18:19:33, Mark Mentovai wrote: > #include "base/logging.h" Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:19: virtual ~ListenerImpl(); On 2011/01/04 18:19:33, Mark Mentovai wrote: > Q. Why is this virtual? > > A. Because Start() is virtual. > > Q. Why is Start() virtual? > > A. ? > > For the purposes of this class as it stands now, I don’t think you need to > declare any destructor at all (or define any destructor, lines 33-34). Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:42: Listener::Listener(const std::string& name, Listener::Delegate* delegate) On 2011/01/04 18:19:33, Mark Mentovai wrote: > Wait a sec. This is how you’re doing your factory? Then does Listener actually > need to have anything declared virtual? > > I get the sense that you wrote this with the platform split being handled > differently, and then changed it up once or twice along the way. No biggie, but > let’s make sure the split is handled well, and get rid of the leftover stuff > from the rewrites. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_linux.cc:46: Listener::~Listener() { } On 2011/01/04 18:19:33, Mark Mentovai wrote: > Put the } on its own line. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/01/04 18:19:33, Mark Mentovai wrote: > What will you use this for? Sending notifications to the service process. > How is it supposed to operate with multiple running Chromes? Consider two > Chromes running out of different profiles in the same user account, or two > Chromes running in different user accounts under fast user switching. I have changed the API so the caller and listener can handle those cases. > Keep in mind that I’m not going to use your answers to block you from checking > in an incomplete design, I just want to get an idea of how this will be used, > what the constraints are, how it currently deals with those constraints, and how > it might ultimately be able to deal. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:7: #include <notify.h> On 2011/01/04 18:19:33, Mark Mentovai wrote: > <notify.h> and <Foundation/Foundation.h> can be in the same group, no need for a > blank line separating them. Once they’re in the same group, F sorts before n. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:83: return io_loop->WatchFileDescriptor(fd_, true, MessageLoopForIO::WATCH_READ, On 2011/01/04 18:19:33, Mark Mentovai wrote: > Do you ever need to undo this? watcher_ will disappear when ListenerImpl is > destroyed, but I don’t know whether something else makes io_loop aware of that > or if you need to do it yourself. when watcher_ is destroyed it handles taking care of unregistering itself. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:90: if (read(fd_, &token, sizeof(token)) >= 0) { On 2011/01/04 18:19:33, Mark Mentovai wrote: > HANDLE_EINTR from base/eintr_wrapper.h. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:91: // Have to swap to native endianness <rdar://problem/5352778>. On 2011/01/04 18:19:33, Mark Mentovai wrote: > Link to the problem: good. > > Link to the problem that I can’t see: bad. > > Open Radar and Google Search aren’t fruitful. I’d appreciate a better > description of the problem. Is this something that will be fixed in some future > OS release? If it is, will ntohl stop being the right thing to do? If so, is > there some sort of test we can write so that we’ll be notified pretty quickly if > this workaround (if that’s what it is) becomes obsolete? I copied the rdar link from some Apple source code. I have filed a separate bug and referenced it, and copied it into openradar. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_mac.mm:109: Listener::~Listener() { } On 2011/01/04 18:19:33, Mark Mentovai wrote: > {} on separate lines, along with whatever other questions and comments I made in > the Linux file that apply to this one too. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... File chrome/common/multi_process_notification_unittest.cc (right): http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:5: #if defined(OS_MACOSX) On 2011/01/04 18:19:33, Mark Mentovai wrote: > This is ineffective because you don’t have build/build_config.h yet. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:22: static const char kStartedNotificationName[]; On 2011/01/04 18:19:33, Mark Mentovai wrote: > These can be file-static or in an anonymous namespace in this file. There’s no > reason for them to be public class members, right? > > (I get that you couldn’t do private class members because of > MultiProcessNotificationMain.) Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:39: On 2011/01/04 18:19:33, Mark Mentovai wrote: > Extra blank line. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:42: scoped_ptr<MessageLoop> io_loop; On 2011/01/04 18:19:33, Mark Mentovai wrote: > This should be io_loop_. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:42: scoped_ptr<MessageLoop> io_loop; On 2011/01/04 15:54:09, Paweł Hajdan Jr. wrote: > Why not just stack-allocate the loop? You can RunAllPending in TearDown if you > wish. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:69: new QuitTask()); On 2011/01/04 15:54:09, Paweł Hajdan Jr. wrote: > Is there any reason for not using MessageLoop::QuitTask here? IIRC, Quit can > only be called on the current message loop, so the MessageLoopForIO code in the > custom QuitTask should make no difference. > > If the above is not true, it at least deserves a comment. Done. http://codereview.chromium.org/5970015/diff/1/chrome/common/multi_process_not... chrome/common/multi_process_notification_unittest.cc:110: On 2011/01/04 18:19:33, Mark Mentovai wrote: > Blank line at EOF. Done.
Code I commented in the drive-by LGTM. Thank you.
Nice job. These comments are relatively minor. http://codereview.chromium.org/5970015/diff/11001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): http://codereview.chromium.org/5970015/diff/11001/chrome/chrome_common.gypi#n... chrome/chrome_common.gypi:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. CL description: should be “Add multi-process notification class.” This becomes the checkin message, think about how it’ll look in the revision history. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification.cc (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.cc:26: Nuke the blank line at EOF. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification.h (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:15: class Task; This forward declaration isn’t necessary, as you’ve #included "base/task.h". It’s possible that with this forward declaration, you don’t actually need base/task.h here? http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:26: enum Domain { Good solution. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:48: // Dtor required for scoped_ptr to compile. A few more characters would yield a complete sentence. → A destructor is required for scoped_ptr to compile. But is this statement even true? http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:67: virtual void OnNotificationReceived(const std::string& name); Do you want to use that goofy OVERRIDE macro from "base/compiler_specific.h" here? http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:68: bool WasNotificationCalled(); I think that WasNotificationReceived() is a more appropriate name. Users of the API will probably be thinking more about whether a notification was received than whether a specific function was called—a function being called is an implementation detail. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:27: std::string PrependDomain(const std::string& name, The meaning of domain is overloaded in this function, which is confusing. The function name itself seems to mean “domain” as in “the hierarchical domain that will be prepended to name, based on the bundle ID and perhaps other stuff.” But you’ve also got “domain” meaning “multi_process_notification::Domain.” http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:52: return domain_string + bundle_id + "." + name; I think that these should always start with bundle_id, then have an identifier denoting the domain, then have the domain-specific data (if needed), and finally the name. That’s how I think of these hierarchically, anyway. Also, I think it’s the best way to avoid inadvertent (or intentional) collisions now that you’re effectively sticking partially-in-the-user’s-control data into the string (DIR_USER_DATA). domain_string + ".profile." + sha1(user_data_dir) + "." + name domain_string + ".uid." + getuid() + "." + name domain_string + ".system." + name sha1(user_data_dir) — or some other hash — to avoid nasty-character badness. Escaping nasty characters would be another alternative. If you’re confident that nasty characters can’t cause badness, we can get by without the hashification or escaposity. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:114: << notification << "' Status:" << status; Need a space after Status:? http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification_unittest.cc (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_unittest.cc:27: // Post a quit task so that this loop eventually ends and we don't hang What’s with this indentation? Bring it back to be flush where it belongs, but put a blank line before this comment to break it up from the preceding line.
Note that I reviewed set 3, which was current when I began reading, but I see you’re up to 6 now.
Thanks Mark. I hope you know that I have a dream that one day I will run a decent sized code review past you and that you will have no comments/improvements/grammar corrections at all. A man can dare to dream can't he? I do truly appreciate the level of detail you do in these reviews. http://codereview.chromium.org/5970015/diff/11001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): http://codereview.chromium.org/5970015/diff/11001/chrome/chrome_common.gypi#n... chrome/chrome_common.gypi:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/01/06 17:59:09, Mark Mentovai wrote: > CL description: should be “Add multi-process notification class.” This becomes > the checkin message, think about how it’ll look in the revision history. Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification.cc (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.cc:26: On 2011/01/06 17:59:09, Mark Mentovai wrote: > Nuke the blank line at EOF. Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification.h (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:15: class Task; On 2011/01/06 17:59:09, Mark Mentovai wrote: > This forward declaration isn’t necessary, as you’ve #included "base/task.h". > It’s possible that with this forward declaration, you don’t actually need > base/task.h here? Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:26: enum Domain { On 2011/01/06 17:59:09, Mark Mentovai wrote: > Good solution. Thanks http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:48: // Dtor required for scoped_ptr to compile. On 2011/01/06 17:59:09, Mark Mentovai wrote: > A few more characters would yield a complete sentence. > > → A destructor is required for scoped_ptr to compile. > > But is this statement even true? Done. It is as far as I can tell. Without this d'tor (and it can't be an inline d'tor) this file won't compile with problems in the scoped_ptr template. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:67: virtual void OnNotificationReceived(const std::string& name); On 2011/01/06 17:59:09, Mark Mentovai wrote: > Do you want to use that goofy OVERRIDE macro from "base/compiler_specific.h" > here? Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification.h:68: bool WasNotificationCalled(); On 2011/01/06 17:59:09, Mark Mentovai wrote: > I think that WasNotificationReceived() is a more appropriate name. Users of the > API will probably be thinking more about whether a notification was received > than whether a specific function was called—a function being called is an > implementation detail. Good call. Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:27: std::string PrependDomain(const std::string& name, On 2011/01/06 17:59:09, Mark Mentovai wrote: > The meaning of domain is overloaded in this function, which is confusing. The > function name itself seems to mean “domain” as in “the hierarchical domain that > will be prepended to name, based on the bundle ID and perhaps other stuff.” But > you’ve also got “domain” meaning “multi_process_notification::Domain.” Renamed. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:52: return domain_string + bundle_id + "." + name; On 2011/01/06 17:59:09, Mark Mentovai wrote: > I think that these should always start with bundle_id, then have an identifier > denoting the domain, then have the domain-specific data (if needed), and finally > the name. That’s how I think of these hierarchically, anyway. Also, I think it’s > the best way to avoid inadvertent (or intentional) collisions now that you’re > effectively sticking partially-in-the-user’s-control data into the string > (DIR_USER_DATA). > > domain_string + ".profile." + sha1(user_data_dir) + "." + name > domain_string + ".uid." + getuid() + "." + name > domain_string + ".system." + name > > sha1(user_data_dir) — or some other hash — to avoid nasty-character badness. > Escaping nasty characters would be another alternative. If you’re confident that > nasty characters can’t cause badness, we can get by without the hashification or > escaposity. See man 3 notify for why I did it a little ass-backwards. having the uid.getuid() out front is important for notify. I don't think I need to escape the string. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:114: << notification << "' Status:" << status; On 2011/01/06 17:59:09, Mark Mentovai wrote: > Need a space after Status:? Done. http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... File chrome/common/multi_process_notification_unittest.cc (right): http://codereview.chromium.org/5970015/diff/11001/chrome/common/multi_process... chrome/common/multi_process_notification_unittest.cc:27: // Post a quit task so that this loop eventually ends and we don't hang On 2011/01/06 17:59:09, Mark Mentovai wrote: > What’s with this indentation? Bring it back to be flush where it belongs, but > put a blank line before this comment to break it up from the preceding line. Stupid Xcode.
Great, LGTM. http://codereview.chromium.org/5970015/diff/28001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/28001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:40: domain_string = StringPrintf("user.uid.%u.%s.", Put a comment here referencing “man 3 notify” as you did in your response to my question. Since this format is significant, you don’t want anyone coming through here and thinking that it’s totally free-form and up to us to decide what to do.
Mark, can you take a look at the mac implementation now with 10.5 support?
The approach is good. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:31: #define USE_LEOPARD_SWITCHBOARD_THREAD 1 You don’t want to check this in, do you? http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:72: return major_version <= 10 && minor_version <= 5; This comparison is wrong. It’s probably not wrong in a way that would affect anything, but it’s so easy to write it correctly. return major_version < 10 || (major_version == 10 && minor_version <= 5); http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:102: // internal_fd_: which communicates from the thread to external threads This is confusing because it introduces “external threads” without explaining what they are, and because it doesn’t say what “thread” means. I’m assuming that as you’ve used “thread” here, you mean the switchboard thread. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:129: bool finished() { return finished_; } bool finished() const { return finished_; } http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:142: // Describe entries in our entries list for matching tokens to notifications “Describe entries in our entries list?” I don’t understand why you said “describe” or why you repeated “entries” or, as always, why you even needed “our.” I think “Used to match tokens with notifications and vice-versa.” is enough of a comment. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:144: typedef struct { “typedef struct” is so C-ish. This can just be “struct SwitchboardEntry”. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:151: kKillThreadMessage = 0xdecea5e I like this constant. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:208: uint32_t status = notify_cancel(notify_fd_token_); Is this safe if notify_register_file_descriptor in Init failed? http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:265: // Send the appropriate message to end our thread, and then wait for it our -> the http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:274: int nfds = (internal_fd_ > notify_fd_ ? internal_fd_ : notify_fd_) + 1; std::max from <algorithm>? http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:279: if (count == 0) continue; I wonder if you actually expect this to happen without a timeout. In any case, I’d put the error handling first (if (count < 0)) and then handle the unconventional case (if (count == 0)) immediately afterwards. And I’d break the continue out onto its own line. This is already a pretty tall function, one more line won’t kill it. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:281: PLOG(ERROR) << "Exiting LeopardSwitchboardThread. Select error."; The messages in this function are very verbose. All of this cstring data needs to ride along with the executable. Bloatish. Can you trim them down a bit? Keep in mind that the LOG family will already include a filename and line number which helps contribute to the bloat, but which will also help determine what’s going on if you get a log snippet. This might be OK as PLOG(ERROR) << "select"; http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:298: LOG(ERROR) << "Exiting LeopardSwitchboardThread. Invalid token sent: " This LOG doesn’t seem to reflect what actually happens. The thread doesn’t actually exit. Causing the thread to exit seems excessively punitive, anyway. I think I’m OK with this being a LOG(WARNING) and then proceeding to write to internal_fd_. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:303: PLOG(ERROR) << "write"; You gave flowery descriptions of the rest of your errors, including a note that LeopardSwitchboardThread was exiting. This one just says “write”? (Per the above comment, I kind of prefer the short messages, but I’m pointing out the inconsistency in case you really really really really really like the long ones and want to keep them.) http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:306: LOG(ERROR) << "Exiting LeopardSwitchboardThread. External_fd_ closed."; Don’t capitalize variable names just because they begin sentences. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:310: << "Write from notify wrong size: " << status; “Read from notify” made sense, because I read it as “read from notify_fd_.” What does “write from notify” mean? http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:315: int value = -1; No need to initialize this. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:320: LOG(ERROR) << "Exiting LeopardSwitchboardThread. Internal_fd_ closed."; Don’t capitalize variable names just because they begin new sentences. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:340: return -1; Are tokens just opaque junk? If so, is -1 a possible valid token value? And if so, find another way to do this: return a bool and pass the token back in an out parameter? http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:380: if (HANDLE_EINTR(read(external_fd_, &token, sizeof(token))) >= 0) { Shouldn’t happen, but this should really check |== sizeof(token)|. |>= 0| will catch a short read (which would be really odd) and EOF (which is less odd and might be triggered by other bugs). http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:428: if (!g_switchboard_thread_ || g_switchboard_thread_->finished()) { This is a little ugly. Maybe the switchboard class should have a static function like GetSwitchboardThread, whose job it is to create and start the thread if it doesn’t already exist and if it isn’t already running. http://codereview.chromium.org/5970015/diff/33001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:431: delete g_switchboard_thread_; I haven’t thought too much about this, but I’ll ask you to: Is it possible to delete this while someone else might still be using it? Can finished_ be true but the object still exist, taking notifications like OnFileCanReadWithoutBlocking? That might get racy.
On 2011/01/12 22:42:35, Mark Mentovai wrote: I think all comments above have been addressed, and race conditions dealt with. Hopefully I haven't introduced too many stylistic mistakes, but I'm getting this to you quickly in hopes that you may catch any stupid architectural issues I have introduced.
I think this is LGTM pending the results of your “is -1 an invalid token for sure?” investigation. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:35: #define USE_LEOPARD_SWITCHBOARD_THREAD 1 Turn this back off. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:147: // User to match tokens to notifications and vice-versa. “User” should be “Used”. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:258: // the thread can be exited cleanly. the thread can be stopped cleanly. “Exit” sounds to me more like something that an object would do itself, in which case you’d say “so that the thread can exit cleanly,” but that’s not what happens. The observer will ask the thread to stop. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:546: Dump the blank line at EOF.
Changed from depending on tokens to fds to alleviate concerns. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... File chrome/common/multi_process_notification_mac.mm (right): http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:35: #define USE_LEOPARD_SWITCHBOARD_THREAD 1 On 2011/01/13 23:10:34, Mark Mentovai wrote: > Turn this back off. Done. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:147: // User to match tokens to notifications and vice-versa. On 2011/01/13 23:10:34, Mark Mentovai wrote: > “User” should be “Used”. Done. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:258: // the thread can be exited cleanly. On 2011/01/13 23:10:34, Mark Mentovai wrote: > the thread can be stopped cleanly. “Exit” sounds to me more like something that > an object would do itself, in which case you’d say “so that the thread can exit > cleanly,” but that’s not what happens. The observer will ask the thread to stop. Done. http://codereview.chromium.org/5970015/diff/46001/chrome/common/multi_process... chrome/common/multi_process_notification_mac.mm:546: On 2011/01/13 23:10:34, Mark Mentovai wrote: > Dump the blank line at EOF. Done.
LGTM
sorry for the late drive-by, but I just saw this code. What will this be used for?
Turns out that it isn't going to be used, and is going to be removed very shortly. On Mon, Jan 31, 2011 at 12:37 PM, <jam@chromium.org> wrote: > sorry for the late drive-by, but I just saw this code. What will this be > used > for? > > http://codereview.chromium.org/5970015/ >
just a friendly ping, can this be removed if it's not needed? I'm trying to scope out what needs to stay in src\chrome and what will move to src\contents, so the less code that's there the easier the task is On Tue, Feb 1, 2011 at 2:43 PM, David Maclachlan <dmaclach@google.com>wrote: > Turns out that it isn't going to be used, and is going to be removed > very shortly. > > On Mon, Jan 31, 2011 at 12:37 PM, <jam@chromium.org> wrote: > > sorry for the late drive-by, but I just saw this code. What will this be > > used > > for? > > > > http://codereview.chromium.org/5970015/ > > >
I'll be removing it in the next couple of days. Cheers, Dave On Tue, Feb 15, 2011 at 1:32 PM, John Abd-El-Malek <jam@chromium.org> wrote: > just a friendly ping, can this be removed if it's not needed? I'm trying to > scope out what needs to stay in src\chrome and what will move to > src\contents, so the less code that's there the easier the task is > > On Tue, Feb 1, 2011 at 2:43 PM, David Maclachlan <dmaclach@google.com> > wrote: >> >> Turns out that it isn't going to be used, and is going to be removed >> very shortly. >> >> On Mon, Jan 31, 2011 at 12:37 PM, <jam@chromium.org> wrote: >> > sorry for the late drive-by, but I just saw this code. What will this >> > be >> > used >> > for? >> > >> > http://codereview.chromium.org/5970015/ >> > > >
great, thanks! On Tue, Feb 15, 2011 at 2:04 PM, David Maclachlan <dmaclach@google.com>wrote: > I'll be removing it in the next couple of days. > > Cheers, > Dave > > On Tue, Feb 15, 2011 at 1:32 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > > just a friendly ping, can this be removed if it's not needed? I'm trying > to > > scope out what needs to stay in src\chrome and what will move to > > src\contents, so the less code that's there the easier the task is > > > > On Tue, Feb 1, 2011 at 2:43 PM, David Maclachlan <dmaclach@google.com> > > wrote: > >> > >> Turns out that it isn't going to be used, and is going to be removed > >> very shortly. > >> > >> On Mon, Jan 31, 2011 at 12:37 PM, <jam@chromium.org> wrote: > >> > sorry for the late drive-by, but I just saw this code. What will this > >> > be > >> > used > >> > for? > >> > > >> > http://codereview.chromium.org/5970015/ > >> > > > > > > |
