Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10833060:
Update for OS X 10.6 SDK. (Closed)
|
Created:
8 years, 4 months ago by Jamie Modified:
8 years, 4 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate for OS X 10.6 SDK.
BUG=138999, 139274
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149106
Patch Set 1 #
Total comments: 3
Patch Set 2 : "Added TODO to clean up the use of private APIs." #
Total comments: 4
Patch Set 3 : Updated comments. #
Messages
Total messages: 16 (0 generated)
Lambros, can you please test this on a Lion installation--I don't have one. You'll also need to LGTM once everyone is happy. Daniel, a lot of the changes are to code you know better than I do. PTAL. Robert, I've had to suppress most of the warnings, rather than actually updating the APIs. I think the comments are sufficient to explain why, but PTAL. https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... remoting/host/video_frame_capturer_mac.mm:657: LOG(INFO) << "Using CgBlitPreLion."; These messages seem worth making more prominent. They shouldn't appear more than once per session unless the screen mode changes.
https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... remoting/host/video_frame_capturer_mac.mm:315: IOPMAssertionCreateWithName(CFSTR("UserIsActive"), How is this valid? The valid assertion types are kIOPMAssertionTypePreventUserIdleSystemSleep, kIOPMAssertionTypePreventUserIdleDisplaySleep, and kIOPMAssertionTypePreventSystemSleep. What are you trying to do here?
Note that calling IOPMAssertionCreateWithName with an invalid type will return an error (see the implementation of IOPMAssertionCreateWithName in http://www.opensource.apple.com/source/IOKitUser/IOKitUser-514.16.31/pwr_mgt.... . Why are you not checking for errors? If nothing else, log. See my implementation of PowerSaveBlocker in content/browser. I'd be happy to move it higher in the food chain if that meant you could use it.
https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/1/remoting/host/video_fr... remoting/host/video_frame_capturer_mac.mm:315: IOPMAssertionCreateWithName(CFSTR("UserIsActive"), On 2012/07/27 23:16:07, Avi wrote: > How is this valid? The valid assertion types are > kIOPMAssertionTypePreventUserIdleSystemSleep, > kIOPMAssertionTypePreventUserIdleDisplaySleep, and > kIOPMAssertionTypePreventSystemSleep. What are you trying to do here? The intent is to wake up the display if it is already asleep on connection. kIOPMAssertionTypeNoDisplaySleep doesn't do this, and there is not documented constant that does. dcaiafa@ found this one by digging through the source code so he can probably provide more details. Regarding error checking, I think we expect this to fail on systems prior to 10.8 (perhaps the source code you cite is from an older system?) On older systems, we don't need the assertion because the input injection APIs awaken the display. I guess we could log an error conditionally based on the OS version. I'll look into replacing this with PowerSaveBlocker for a future CL.
> On 2012/07/27 23:16:07, Avi wrote: > How is this valid? The valid assertion types are > kIOPMAssertionTypePreventUserIdleSystemSleep, > kIOPMAssertionTypePreventUserIdleDisplaySleep, and > kIOPMAssertionTypePreventSystemSleep. What are you trying to do here? kIOPMAssertionTypePreventUserIdleDisplaySleep used to work until 10.7.3. Apple made a change in 10.7.4 where none of the documented assertions would wake up an already sleeping display. I found the "UserIsActive" assertion by running Apple's Remote Desktop (which would wake the display as we wanted), and using pmset to inspect which assertions it created.
On 2012/07/28 00:43:30, Jamie wrote: > The intent is to wake up the display if it is already asleep on connection. > kIOPMAssertionTypeNoDisplaySleep doesn't do this, and there is not documented > constant that does. dcaiafa@ found this one by digging through the source code > so he can probably provide more details. This is something that you need to document in the source code. Relying on private interfaces like this is tricky enough, and while I don't have a problem doing it when we don't have a choice, at least we need to be clear. > Regarding error checking, I think we expect this to fail on systems prior to > 10.8 (perhaps the source code you cite is from an older system?) > I guess we could log an error conditionally based on the OS version. If we're conditionally executing code based on the OS version, we shouldn't make a system call that we know will error out rather than ignore the result. > kIOPMAssertionTypePreventUserIdleDisplaySleep used to work until 10.7.3. Apple > made a change in 10.7.4 where none of the documented assertions would wake up an > already sleeping display. In 10.7.3 they introduced IOPMAssertionDeclareUserActivity for exactly this purpose. Can we use kIOPMAssertionTypePreventUserIdleDisplaySleep through 10.7.2, and use the new API from 10.7.3 on?
On 2012/07/28 02:07:11, Avi wrote: > On 2012/07/28 00:43:30, Jamie wrote: > > The intent is to wake up the display if it is already asleep on connection. > > kIOPMAssertionTypeNoDisplaySleep doesn't do this, and there is not documented > > constant that does. dcaiafa@ found this one by digging through the source code > > so he can probably provide more details. > > This is something that you need to document in the source code. Relying on > private interfaces like this is tricky enough, and while I don't have a problem > doing it when we don't have a choice, at least we need to be clear. > > > Regarding error checking, I think we expect this to fail on systems prior to > > 10.8 (perhaps the source code you cite is from an older system?) > > > I guess we could log an error conditionally based on the OS version. > > If we're conditionally executing code based on the OS version, we shouldn't make > a system call that we know will error out rather than ignore the result. > > > kIOPMAssertionTypePreventUserIdleDisplaySleep used to work until 10.7.3. Apple > > made a change in 10.7.4 where none of the documented assertions would wake up > an > > already sleeping display. > > In 10.7.3 they introduced IOPMAssertionDeclareUserActivity for exactly this > purpose. Can we use kIOPMAssertionTypePreventUserIdleDisplaySleep through > 10.7.2, and use the new API from 10.7.3 on? I've added a TODO to move to this API. I like your suggestion of reusing PowerSaveBlocker (we weren't aware of it when writing this), but given the need to land this for M22 in order to unblock the 10.6 SDK effort, I think we should land this CL as an interim fix for now.
https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... remoting/host/video_frame_capturer_mac.mm:316: IOPMAssertionCreateWithName(CFSTR("UserIsActive"), Can you please put in a brief note as to where you got this constant?
Also reference BUG=139274 in your description. https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... remoting/host/video_frame_capturer_mac.mm:652: LOG(INFO) << "Using CgBlitPostLion."; This will be spammy, along with the other LOG statements.
https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... remoting/host/video_frame_capturer_mac.mm:652: LOG(INFO) << "Using CgBlitPostLion."; On 2012/07/30 21:50:37, rsesek wrote: > This will be spammy, along with the other LOG statements. It should only be logged once per Chromoting session, unless the screen mode changes.
LGTM, but wait for Avi, too
LGTM when you add a comment explaining what the constant does and where you got it.
lgtm
fyi https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... File remoting/host/video_frame_capturer_mac.mm (right): https://chromiumcodereview.appspot.com/10833060/diff/7001/remoting/host/video... remoting/host/video_frame_capturer_mac.mm:316: IOPMAssertionCreateWithName(CFSTR("UserIsActive"), On 2012/07/30 21:04:36, Avi wrote: > Can you please put in a brief note as to where you got this constant? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10833060/11002
Change committed as 149106 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
