Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10543009:
Content: Implement PowerSaveBlocker2 for the Mac. (Closed)
|
Created:
8 years, 6 months ago by Avi (use Gerrit) Modified:
8 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionContent: Implement PowerSaveBlocker2 for the Mac.
BUG=126591
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140664
Patch Set 1 #
Total comments: 12
Patch Set 2 : ch-ch-changes #Messages
Total messages: 18 (0 generated)
Nico: for review mdm, rvargas: FYI Nico, note the new interface; we're building this new version of the class, much simpler, and then swapping it in.
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:76: // ====== ^^ the deprecated way ^^ vv the way that doesn't work yet vv ====== Why not wait with committing until it works? https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 This is confusing to me. Why not remove this define and call the new interface PSB2 until PSB is gone? That's how we usually do this I believe.
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:76: // ====== ^^ the deprecated way ^^ vv the way that doesn't work yet vv ====== On 2012/06/05 20:30:11, Nico wrote: > Why not wait with committing until it works? It would work, but only on OS X. There are three of us working on the different platforms, and we can't switch over the callers of this API until we have all the platforms in place. https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 On 2012/06/05 20:30:11, Nico wrote: > This is confusing to me. Why not remove this define and call the new interface > PSB2 until PSB is gone? That's how we usually do this I believe. This makes the eventual patch smaller since we just delete this one line, rather than changing all the instances and potentially needing to reformat the code. The timescale here is a few days at most so it seems reasonable to do it this way.
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:76: // ====== ^^ the deprecated way ^^ vv the way that doesn't work yet vv ====== On 2012/06/05 20:30:11, Nico wrote: > Why not wait with committing until it works? This is a joke; the new code works perfectly. https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 On 2012/06/05 20:30:11, Nico wrote: > This is confusing to me. Why not remove this define and call the new interface > PSB2 until PSB is gone? That's how we usually do this I believe. The new interface _is_ called PSB2. This is used in the implementation file to save renaming everything. See https://chromiumcodereview.appspot.com/10538013/
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 On 2012/06/05 20:40:24, Avi wrote: > On 2012/06/05 20:30:11, Nico wrote: > > This is confusing to me. Why not remove this define and call the new interface > > PSB2 until PSB is gone? That's how we usually do this I believe. > > The new interface _is_ called PSB2. This is used in the implementation file to > save renaming everything. See https://chromiumcodereview.appspot.com/10538013/ With "Everything" just being the 77 lines below this define, right?
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 Yes, that is the definition of "everything" we're working with here.
Other than the nits below I think the CL is ok. https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:76: // ====== ^^ the deprecated way ^^ vv the way that doesn't work yet vv ====== On 2012/06/05 20:40:24, Avi wrote: > On 2012/06/05 20:30:11, Nico wrote: > > Why not wait with committing until it works? > > This is a joke; the new code works perfectly. Reword the comment to "// TODO: The below will replace the above code soon, http://crbug.com/..." https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:90: thread->Start(); It's also surprising to me that g_power_thread2->Pointer() implicitly starts a thread -- can't you keep the thread creation technique used in the class above? That's more explicit, and from what I understand the "Start()" code path executed in just one function anyway (PSB() constructor) (?) https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:101: #define PowerSaveBlocker PowerSaveBlocker2 On 2012/06/05 20:58:30, Avi wrote: > Yes, that is the definition of "everything" we're working with here. Changing that takes about 2s in a text editor. I don't think having this define is worth it.
https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... File content/browser/power_save_blocker_mac.cc (right): https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_... content/browser/power_save_blocker_mac.cc:90: thread->Start(); On 2012/06/05 21:08:45, Nico wrote: > can't you keep the thread creation technique used in the class above? That worked in the code above because PowerSaveBlocker::ApplyBlock was guaranteed to always be on the UI thread. Therefore we didn't have to worry about threading. Here, there's no longer any guarantees about what thread PowerSaveBlocker2 will be instantiated on, so we need protection against races.
On Tue, Jun 5, 2012 at 2:11 PM, <avi@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10543009/diff/1/** > content/browser/power_save_**blocker_mac.cc<https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_save_blocker_mac.cc> > File content/browser/power_save_**blocker_mac.cc (right): > > https://chromiumcodereview.**appspot.com/10543009/diff/1/** > content/browser/power_save_**blocker_mac.cc#newcode90<https://chromiumcodereview.appspot.com/10543009/diff/1/content/browser/power_save_blocker_mac.cc#newcode90> > content/browser/power_save_**blocker_mac.cc:90: thread->Start(); > On 2012/06/05 21:08:45, Nico wrote: > >> can't you keep the thread creation technique used in the class above? >> > > That worked in the code above because PowerSaveBlocker::ApplyBlock was > guaranteed to always be on the UI thread. Therefore we didn't have to > worry about threading. Here, there's no longer any guarantees about what > thread PowerSaveBlocker2 will be instantiated on, so we need protection > against races. > Ah, that makes sense. Thinking about this, could you use the blocking pool for this function? Then you wouldn't have to spin up a thread (that's never joined) yourself. > > https://chromiumcodereview.**appspot.com/10543009/<https://chromiumcodereview... >
On 2012/06/05 21:15:29, Nico wrote: > Thinking about this, could you use the blocking pool for this function? Is it OK to throw things in the blocking pool that hang forever? When the power functions work, they return immediately. When they don't work, they _really_ don't work.
On Tue, Jun 5, 2012 at 2:28 PM, <avi@chromium.org> wrote: > On 2012/06/05 21:15:29, Nico wrote: > >> Thinking about this, could you use the blocking pool for this function? >> > > Is it OK to throw things in the blocking pool that hang forever? When the > power > functions work, they return immediately. When they don't work, they > _really_ > don't work. Hm, don't you need a thread sniper in any case then? Or do you think it's fine to have a thread that stays around forever waiting for the system while it's message queue slowly but steadily grows?
For machines that are that broken, I'm only concerned with behaving in a vaguely sane way, and letting a thread slowly grow is saner than jamming the blocking pool. This isn't relevant to the 99.9999% of machines that behave correctly.
On Tue, Jun 5, 2012 at 2:39 PM, <avi@chromium.org> wrote: > For machines that are that broken, I'm only concerned with behaving in a > vaguely > sane way, and letting a thread slowly grow is saner than jamming the > blocking > pool. This isn't relevant to the 99.9999% of machines that behave > correctly. > Where does the 99.9999% figure come from? :-)
otoh, it's what the code did before and we don't have reports of it causing problems, so let's done change that for now.
On 2012/06/05 21:45:23, Nico wrote: > otoh, it's what the code did before and we don't have reports of it causing > problems, so let's done change that for now. Then ptal at the latest patchset.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi@chromium.org/10543009/3003
Change committed as 140664 |
