I noticed this on Mac, not sure if it happens on other platforms.

Dupe of bug 517870? Or is the password screen on the mac different?

I don't really know. Calling this a dupe and CCing smichaud to yell at me if the password screen on Mac is activated using another mechanism.

Sounds reasonable to me. Only you, Ehsan, can tell us whether or not what you saw was the screensaver asking you for a password :-) If you did, then you're right.

I have screen saver disabled, so my mac goes directly to the password screen after 30 minutes.

> I have screen saver disabled, so my mac goes directly to the > password screen after 30 minutes. So you have your computer set to log you out after 30 minutes of inactivity? (This can be done in System Preferences : Security, and is off by default.) If so then this bug isn't a dup of bug 517870. (Though it's conceivable you could "fix" both bugs by faking "activity".) And by the way, what you saw wasn't the "password screen", it was the "login screen".

(In reply to comment #5) > > I have screen saver disabled, so my mac goes directly to the > > password screen after 30 minutes. > > So you have your computer set to log you out after 30 minutes of > inactivity? > > (This can be done in System Preferences : Security, and is off by > default.) > > If so then this bug isn't a dup of bug 517870. (Though it's > conceivable you could "fix" both bugs by faking "activity".) > > And by the way, what you saw wasn't the "password screen", it was the > "login screen". Hmm, OK seems like I've got everything wrong! Here's the settings I have: In Security, I have "Require password for sleep and screen saver" checked. In "Desktop and Screen Saver", I do have a screen saver set up (sorry for misdirecting you on that). In Energy Saver, I have set up "Display sleep" to a value which I think might be lower than the screen saver timeout, so I don't see the screen saver before my display goes to sleep.

So, when you're in fullscreen mode your display goes to sleep, and then when you move the mouse you see the "password for sleep and screensaver" screen?

(In reply to comment #7) > So, when you're in fullscreen mode your display goes to sleep, and then when > you move the mouse you see the "password for sleep and screensaver" screen? Correct.

I think the bug here is that your display shouldn't go to sleep when you're in "fullscreen mode". Which makes this bug different from bug 517870 (though still related to it). By the way, I assume the "fullscreen mode" you're talking about is a plugin's fullscreen mode, and not the browser's fullscreen mode.

(In reply to comment #9) > I think the bug here is that your display shouldn't go to sleep when you're in > "fullscreen mode". Which makes this bug different from bug 517870 (though > still related to it). So should we reopen this? > By the way, I assume the "fullscreen mode" you're talking about is a plugin's > fullscreen mode, and not the browser's fullscreen mode. It's the new fancy 10.7 full-screen thing, whatever it's called. This is the video in question, fwiw: <http://assange.rt.com/ibrahim-episode-eleven/>

> So should we reopen this? Yes, let's do that. I need to dig into this further to figure out its full implications.

Not sure if this is different code wise, but we should keep the display from dimming, too. With default settings, that happens before the actual sleep and screen savers kick in .

Firefox on Android has the same problem (bug 786893). It would be nice for the core video code notify the platform widget code when to disallow/allow screen savers.

To disable the screensaver during <video> playback and WebRTC calls all we need to do is implement a WakeLockListener for each platform, and the existing code with cause the screensaver disable as appropriate. The WakeLockListener is already implemented for Windows, B2G, and Android. We need them implemented on other platforms. I'll contort this bug to be implementing the MacOSX specific WakeLockListener. See bug 968603 for an example of how to do this (on Windows).

Chris, as you identified, this bug has already been fixed on Windows, B2G, and Android. Can we prioritize this for MacOS and Linux (bug 811261) for platform consistency?

I'm happy to look at this, but I've been asked to finish bug 941296 first.

https://developer.apple.com/library/mac/qa/qa1340/_index.html explains how to do this. See also: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/power_save_blocker_mac.cc

And here's VLC's code. It seems like the api for doing this might be a bit of disaster: http://git.videolan.org/?p=vlc.git;a=blob;f=modules/gui/macosx/intf.m;h=005390a01bca62ccad094c5d88c9ac79cba73bd3;hb=HEAD#l1442

Do you steel work on this bug ? Do you want us to continue what you have done ?

I won't get to it for a few weeks yet, so if you want to take it, please go ahead.

Is it possible to be assigned ? Can you review the patch ?

Comment on attachment 8436316 [details] [diff] [review] bug-772347-fix.patch Steven, would you mind having a look at this?

I should be able to get to this in a few days. Ping me if I forget :-)

I have made some improvements compared to the first patch. If you haven't already look at the first patch you should ignore it, otherwise I can tell you what differs.

Comment on attachment 8437506 [details] [diff] [review] bug-772347.patch This patch has one fairly big problem: + NS_ConvertUTF16toUTF8 s(aTopic); + CFStringRef cf_topic = (CFStringRef) &s; This *shouldn't* work, even if it actually does. Instead you should do something like this: CFStringRef cf_topic = ::CFStringCreateWithCharacters(kCFAllocatorDefault, reinterpret_cast<const UniChar*> (aTopic.get()), aTopic.length()); Then you'll need to call CFRelease(cf_topic) after the call to IOPMAssertionCreateWithName(). And there's a style nit I'd like you to fix: + AddScreenWakeLockListener(); NS_OBJC_TRY_ABORT([NSApp run]); - + RemoveScreenWakeLockListener(); return NS_OK; Please leave an empty line above and below the calls to AddScreenWakeLockListener() and RemoveScreenWakeLockListener(). Otherwise this patch looks fine to me. Please make these changes and ask me again to review them. By the way, I notice that you've mostly just copied the Windows implementation. This is exactly the right way to go about it! And it's what I would have done in your place. The less you write from scratch, the fewer mistakes you'll make :-)

(In reply to comment #14) Chris: I'd like to test Lucas's patch, but I don't know where to find examples to test with. Could you list a few?

Comment on attachment 8437506 [details] [diff] [review] bug-772347.patch Another style nit I forgot to mention: Please ensure that there's a "::" before every call to an external, OS-provided API. For example, IOPMAssertionCreateWithName(), IOPMAssertionRelease(), CFStringCreateWithCharacters() and CFRelease().

(In reply to Steven Michaud from comment #27) > Chris: I'd like to test Lucas's patch, but I don't know where to find > examples to test with. Could you list a few? The issue here is just that the screen goes to sleep (or activates the screensaver) while a video is playing. I run into it all the time with the youtube html player. Set your screensaver to activate in 1 minute, or a similar short time, and then leave an html5 video playing for longer than that without touching the keyboard or mouse. If the screensaver doesn't come on, the patch is working. Here's any example video you can use: https://people.xiph.org/~giles/2013/02-Digital_Show_and_Tell.html Then make sure it goes to sleep again if you stop the video. There may be more subtle things things, like whether autoplay for script invocations trigger the wakelock. I haven't tested those.

What Ralph said; just play an HTML video, and wait for the screensaver to kick in. If the patch works, the screen saver should not kick in.

Thanks for your review Steven. I tried to change the patch as you suggested. I hope it's ok now. I just want to add that I don't want to take all the credit for this patch. We are a french student team composed of Corentin Cos, Benjamin Michel, Nicolas Vignes, myself and last but not least Othman Darraz ;)

I should be able to get to this tomorrow, Lucas. Sorry for the delay.

No problem, I understand ;)

Comment on attachment 8438392 [details] [diff] [review] bug-772347.patch This looks fine to me (save for one more style nit). And my manual tests worked perfectly. I separately set the "Start screen saver" and "Display sleep" to their lowest possible values, then played the HTML5 video from comment #29 (in a separate tab). The screen saver and display sleep didn't kick in when they normally would have. But when I closed the tab (and therefore stopped the video), they did kick in normally. Here's the style nit: + + CFStringRef cf_topic = + ::CFStringCreateWithCharacters(kCFAllocatorDefault, + reinterpret_cast<const UniChar*> + (aTopic.Data()), + aTopic.Length()); + Please remove the blank lines above and below this function call, and attach a new patch (so that someone else can land it). Thanks for your work on this!

I've done the small change you wanted me to do. Thanks you very much Steven for the reviews. We are proud to help Mozilla, as small as our contribution is ! :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d437a1b97e7c Further nits: * Your patch had no commit message. I wrote a minimal one, but please do something similar for your future patches. * Your patch added trailing whitespace. I removed that for you, but please check for this as well before submitting. 'git' and 'hg export' will highlight whitespace issues in red. 'git' will also warn you on commit.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/064a0120db2d This version doesn't compile: /builds/slave/m-in-osx64_g-00000000000000000/build/widget/cocoa/nsAppShell.mm:209:1: error: no template named 'StaticRefPtr'; did you mean 'mozilla::StaticRefPtr'? I've attached the version I committed with the fixed commit message so you can continue from there.

> This version doesn't compile: Odd. It compiled fine for me, earlier today.

> Odd. It compiled fine for me, earlier today. But I landed the patch against m-c code current as of yesterday. Maybe the patch has bitrotted since then.

This one should work. Adding "--disable-unified-compilation" in the mozconfig worked to reproduce the compilation error. There wasn't any error expect the namespace for |StaticRefPtr|.

Comment on attachment 8440108 [details] [diff] [review] bug-772347.patch This looks good to me. I made sure that it applies to current trunk code and builds successfully with or without --disable-unified-compilation. I even tested it again -- it works fine.

Pushing to try for good measure: https://tbpl.mozilla.org/?tree=Try&rev=ac8029e08a62

I'm not really familiar with Trys... Everything is green, sounds good ?

Lucas: green is good! :) Because you added the checkin-needed keyword, someone will land your patch on the mozilla-inbound branch within a day or so.

Cool, Thanks Chris :)

Is this relnote-worthy?

We didn't note it when the feature landed for Windows in ff 30. But does seem a reasonable, if minor, thing to mention.

I've verified this bug using: FF 33 Aurora Build Id: 20140723004001 OS: Mac Os X 10.8.5 And I've encountered the following issue: I've played https://people.xiph.org/~giles/2013/02-Digital_Show_and_Tell.html in fullscreen and waited the video to finish. After the video finished the screensaver will not start after the set time (in my case 2 min). This issue is not reproducible when the video is played in window mode.

Thanks for verifying. I'm not sure if that's a feature or a bug, but I've also noticed occasional log messages about not being able to release the wakelock. I filed bug 1043489 for the new issue.

The bug was verified using: FF 32.09 Beta Build Id: 20141002185629 OS: Mac Os X 10.9.5

Back out the feature for Firefox 33 due to bug 104389. Approval Request Comment [Feature/regressing bug #]: feature 772347, problem bug 104389 [User impact if declined]: Machine will not sleep after video playback finishes in fullscreen mode, consuming unnecessary power. [Describe test coverage new/current, TBPL]: Manual test. [Risks and why]: Risk is moderate. Backout of the feature is straightforward, but there's no time for testing. [String/UUID change made/needed]: None Already landed to make rc deadline: https://hg.mozilla.org/releases/mozilla-release/rev/df37248fafcb

Target milestone tracks trunk landing. Status flags track doing things on release branches :)

Thanks for the correction. :)

I've checked the FF 33 RC and the back-out was successfully done.

Release Note Request (optional, but appreciated) [Why is this notable]: [Suggested wording]: Fullscreen video on Mac disables display sleep, and dimming, during playback. [Links (documentation, blog post, etc)]: I don't have any particular documentation for this. I thought it was annoying enough that users might like to hear that it's been fixed.

Verified as fixed using the following environment: FF 34.0b8 Build Id:20141110195804 OS: Mac Os X 10.9.5