Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10545076:
Implement PowerSaveBlocker2 for Linux. Much simpler than the original! (Closed)
|
Created:
8 years, 6 months ago by Mike Mammarella Modified:
8 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org, Avi (use Gerrit), rvargas (doing something else) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement PowerSaveBlocker2 for Linux. Much simpler than the original!
After this CL is committed, I'll remove all the old code and switch over the
callers. I'll also port the Android version, which is just a stub with a
NOTIMPLEMENTED() in it anyway.
BUG=126591
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141311
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 26
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Messages
Total messages: 11 (0 generated)
Satoru for DBus library use. Scott as content/browser owner. Avi, Ricardo FYI - here's the Linux rewrite.
https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... File content/browser/power_save_blocker.h (right): https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker.h:98: // ~Delegate() {} True dat.
https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:510: #define PowerSaveBlocker PowerSaveBlocker2 This looks ugly. Why do you need to keep the old implementation? https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:521: void RemoveBlock(); function comments are missing. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:545: static const char kFreeDesktopAPIObjectPath[]; nit: would be simpler to define these constants in anonymous namespace than in the class. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:550: // If DPMS is not enabled, then we don't want to try to disable power saving, what does DPMS stand for? Please add some comment about it. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:555: // Returns an appropriate DBus API to use based on the desktop environment. nit: DBus -> D-Bus https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:604: // We must provide a valid-looking interface. We'll change it below. This looks lie a hack. Rather than replacing it later, what about doing something like: scoped_ptr<dbus::MethodCall> method_call; ... case kGnomeAPI: ... method_call.reset(new MethodCall(kGnomeAPIInterfaceName, "Inhibit")); https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:625: message_writer.AppendString(reason_.c_str()); no need to use .c_str() https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:678: // We must provide a valid-looking interface. We'll change it below. see my comment above. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, FILE thread is now deprecated. Please use BrowserThread::PostBlockingPoolTask()
https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:510: #define PowerSaveBlocker PowerSaveBlocker2 On 2012/06/08 14:59:41, satorux1 wrote: > This looks ugly. Why do you need to keep the old implementation? We don't, strictly speaking. We had to do this for the other two platforms because there were three of us doing one platform each, and we can't switch over until all three (well four, including Android, but it's just a stub) have finished the new versions. I wanted to do this CL in the same style as the others (see power_save_blocker_win.cc for instance that also does this), and then do the cleanup CL next to switch over. I will remove this line in literally my very next CL. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:521: void RemoveBlock(); On 2012/06/08 14:59:41, satorux1 wrote: > function comments are missing. I actually had them in a previous patch set, but I removed them because I thought they were superfluous. They basically added nothing to the function names. Is there really any confusion about what these functions do? https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:545: static const char kFreeDesktopAPIObjectPath[]; On 2012/06/08 14:59:41, satorux1 wrote: > nit: would be simpler to define these constants in anonymous namespace than in > the class. Done. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:550: // If DPMS is not enabled, then we don't want to try to disable power saving, On 2012/06/08 14:59:41, satorux1 wrote: > what does DPMS stand for? Please add some comment about it. Improved the comment. It's not particularly relevant what DPMS actually stands for (I had to look it up myself just now), but it's worth saying what it is. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:555: // Returns an appropriate DBus API to use based on the desktop environment. On 2012/06/08 14:59:41, satorux1 wrote: > nit: DBus -> D-Bus Done. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:604: // We must provide a valid-looking interface. We'll change it below. On 2012/06/08 14:59:41, satorux1 wrote: > This looks lie a hack. Rather than replacing it later, what about doing > something like: > > scoped_ptr<dbus::MethodCall> method_call; > ... > > case kGnomeAPI: > ... > method_call.reset(new MethodCall(kGnomeAPIInterfaceName, "Inhibit")); I considered that but thought this seemed a little nicer. Since you prefer the other way I can change it, though how would you feel about adding an if statement to MethodCall::MethodCall() to only call SetInterface() on a non-empty string? https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:625: message_writer.AppendString(reason_.c_str()); On 2012/06/08 14:59:41, satorux1 wrote: > no need to use .c_str() Done. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/08 14:59:41, satorux1 wrote: > FILE thread is now deprecated. Please use BrowserThread::PostBlockingPoolTask() Hmm. As the comment indicates, I need the same thread to be used below in the destructor. If I use PostBlockingPoolSequencedTask() with some random string I make up, will that happen? It's not clear that the same thread will run the tasks, only that they'll run in sequence.
http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... File content/browser/power_save_blocker_linux.cc (right): http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... content/browser/power_save_blocker_linux.cc:521: void RemoveBlock(); On 2012/06/08 18:33:36, Mike Mammarella wrote: > On 2012/06/08 14:59:41, satorux1 wrote: > > function comments are missing. > > I actually had them in a previous patch set, but I removed them because I > thought they were superfluous. They basically added nothing to the function > names. Is there really any confusion about what these functions do? From function name alone, it's hard to tell what ApplyBlock() does. It's probably obvious for the implementer who knows the context, but would be nice to explain what does the function do. Besides, this is from the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Every function declaration should have comments immediately preceding it that describe what the function does and how to use it. http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... content/browser/power_save_blocker_linux.cc:604: // We must provide a valid-looking interface. We'll change it below. On 2012/06/08 18:33:36, Mike Mammarella wrote: > On 2012/06/08 14:59:41, satorux1 wrote: > > This looks lie a hack. Rather than replacing it later, what about doing > > something like: > > > > scoped_ptr<dbus::MethodCall> method_call; > > ... > > > > case kGnomeAPI: > > ... > > method_call.reset(new MethodCall(kGnomeAPIInterfaceName, "Inhibit")); > > I considered that but thought this seemed a little nicer. Since you prefer the > other way I can change it I think using a dummy place holder does not look great. I'd prefer the scoped ptr solution. > though how would you feel about adding an if > statement to MethodCall::MethodCall() to only call SetInterface() on a non-empty > string? Let's not change MethodCall. It takes the two parameters as these are mandatory. http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/08 18:33:36, Mike Mammarella wrote: > On 2012/06/08 14:59:41, satorux1 wrote: > > FILE thread is now deprecated. Please use > BrowserThread::PostBlockingPoolTask() > > Hmm. As the comment indicates, I need the same thread to be used below in the > destructor. If I use PostBlockingPoolSequencedTask() with some random string I > make up, will that happen? It's not clear that the same thread will run the > tasks, only that they'll run in sequence. Why do they need to run on the same thread? As long as it's run in sequence, I thought it'd be ok.
https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:521: void RemoveBlock(); On 2012/06/08 19:31:50, satorux1 wrote: > On 2012/06/08 18:33:36, Mike Mammarella wrote: > > On 2012/06/08 14:59:41, satorux1 wrote: > > > function comments are missing. > > > > I actually had them in a previous patch set, but I removed them because I > > thought they were superfluous. They basically added nothing to the function > > names. Is there really any confusion about what these functions do? > > From function name alone, it's hard to tell what ApplyBlock() does. It's > probably obvious for the implementer who knows the context, but would be nice to > explain what does the function do. > > Besides, this is from the style guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > Every function declaration should have comments immediately preceding it that > describe what the function does and how to use it. OK, I added comments. (The last patch set had them back, but I added more.) https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:604: // We must provide a valid-looking interface. We'll change it below. On 2012/06/08 19:31:50, satorux1 wrote: > On 2012/06/08 18:33:36, Mike Mammarella wrote: > > On 2012/06/08 14:59:41, satorux1 wrote: > > > This looks lie a hack. Rather than replacing it later, what about doing > > > something like: > > > > > > scoped_ptr<dbus::MethodCall> method_call; > > > ... > > > > > > case kGnomeAPI: > > > ... > > > method_call.reset(new MethodCall(kGnomeAPIInterfaceName, "Inhibit")); > > > > I considered that but thought this seemed a little nicer. Since you prefer the > > other way I can change it > > I think using a dummy place holder does not look great. I'd prefer the scoped > ptr solution. > > > > though how would you feel about adding an if > > statement to MethodCall::MethodCall() to only call SetInterface() on a > non-empty > > string? > > Let's not change MethodCall. It takes the two parameters as these are mandatory. Done. Personally I'm not a fan of the way this cascades into having to change message_writer as well. It seems to me that since you can change the interface with SetInterface() after construction it's not really important that it be mandatory in the constructor. https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/08 19:31:50, satorux1 wrote: > On 2012/06/08 18:33:36, Mike Mammarella wrote: > > On 2012/06/08 14:59:41, satorux1 wrote: > > > FILE thread is now deprecated. Please use > > BrowserThread::PostBlockingPoolTask() > > > > Hmm. As the comment indicates, I need the same thread to be used below in the > > destructor. If I use PostBlockingPoolSequencedTask() with some random string I > > make up, will that happen? It's not clear that the same thread will run the > > tasks, only that they'll run in sequence. > > Why do they need to run on the same thread? As long as it's run in sequence, I > thought it'd be ok. They need to be the same because the D-Bus library asserts it. :) This thread becomes both the origin and D-Bus thread.
LGTM http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... File content/browser/power_save_blocker_linux.cc (right): http://codereview.chromium.org/10545076/diff/10001/content/browser/power_save... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/08 19:50:17, Mike Mammarella wrote: > On 2012/06/08 19:31:50, satorux1 wrote: > > On 2012/06/08 18:33:36, Mike Mammarella wrote: > > > On 2012/06/08 14:59:41, satorux1 wrote: > > > > FILE thread is now deprecated. Please use > > > BrowserThread::PostBlockingPoolTask() > > > > > > Hmm. As the comment indicates, I need the same thread to be used below in > the > > > destructor. If I use PostBlockingPoolSequencedTask() with some random string > I > > > make up, will that happen? It's not clear that the same thread will run the > > > tasks, only that they'll run in sequence. > > > > Why do they need to run on the same thread? As long as it's run in sequence, I > > thought it'd be ok. > > They need to be the same because the D-Bus library asserts it. :) > > This thread becomes both the origin and D-Bus thread. Oh, I that explains. You might want to put it as a comment here, as it was rather subtle.
sky: owner LGTM? https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/10001/content/browser/po... content/browser/power_save_blocker_linux.cc:752: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/08 20:00:14, satorux1 wrote: > On 2012/06/08 19:50:17, Mike Mammarella wrote: > > On 2012/06/08 19:31:50, satorux1 wrote: > > > On 2012/06/08 18:33:36, Mike Mammarella wrote: > > > > On 2012/06/08 14:59:41, satorux1 wrote: > > > > > FILE thread is now deprecated. Please use > > > > BrowserThread::PostBlockingPoolTask() > > > > > > > > Hmm. As the comment indicates, I need the same thread to be used below in > > the > > > > destructor. If I use PostBlockingPoolSequencedTask() with some random > string > > I > > > > make up, will that happen? It's not clear that the same thread will run > the > > > > tasks, only that they'll run in sequence. > > > > > > Why do they need to run on the same thread? As long as it's run in sequence, > I > > > thought it'd be ok. > > > > They need to be the same because the D-Bus library asserts it. :) > > > > This thread becomes both the origin and D-Bus thread. > > Oh, I that explains. You might want to put it as a comment here, as it was > rather subtle. Done.
https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... content/browser/power_save_blocker_linux.cc:515: kNoAPI, // Disable. No supported API available. enum style in chrome is ALL_CAPS. https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... content/browser/power_save_blocker_linux.cc:584: : type_(type), reason_(reason), api_(SelectAPI()) { each on its own line, and initialize inhibit_cookie_.
https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... File content/browser/power_save_blocker_linux.cc (right): https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... content/browser/power_save_blocker_linux.cc:515: kNoAPI, // Disable. No supported API available. On 2012/06/08 20:44:41, sky wrote: > enum style in chrome is ALL_CAPS. Ah yes, I always forget this difference between the Google style guide and the Chromium style guide. (In my defense the code I'm replacing did it this way, so I followed suit.) https://chromiumcodereview.appspot.com/10545076/diff/6/content/browser/power_... content/browser/power_save_blocker_linux.cc:584: : type_(type), reason_(reason), api_(SelectAPI()) { On 2012/06/08 20:44:41, sky wrote: > each on its own line, and initialize inhibit_cookie_. Done. (Another difference in the style guides!)
LGTM |
