Using ClickToFlash's HTML5 player on youtube.com, the system's screen dimming preference is not disabled while playing full screen video. Recent bug fixes have fixed other bugs with CTF's player, including the cursor hiding and controller display. This is a regression from Safari 5.0.5 on Snow Leopard that has occurred for all of the 5.1 builds I've used in Lion. I don't know whether this affects 5.1 on SL.
<rdar://problem/9828140>
Created attachment 105053 [details] Patch
Comment on attachment 105053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105053&action=review > Source/WebKit2/UIProcess/mac/WebFullScreenManagerProxyMac.mm:114 > + [[m_webView fullScreenWindowController] setIsPlaying:(isPlaying ? YES : NO)]; You shouldn’t need to jump through that hoop to convert from bool -> BOOL. Does the compiler complain if you pass isPlaying directly? > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:79 > + DOMWindow* window = m_element->document()->domWindow(); > + if (window) { We’d typically move the declaration of window in to the condition part of the if statement here, and below. > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:94 > + window->addEventListener(eventNames.endedEvent, m_mediaEventListener, true); Why does the call to addEventListener not require .get() like the calls to removeEventListener above?
Comment on attachment 105053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105053&action=review >> Source/WebKit2/UIProcess/mac/WebFullScreenManagerProxyMac.mm:114 >> + [[m_webView fullScreenWindowController] setIsPlaying:(isPlaying ? YES : NO)]; > > You shouldn’t need to jump through that hoop to convert from bool -> BOOL. Does the compiler complain if you pass isPlaying directly? No, this was just me being (needlessly) type-specific. I'll remove it. >> Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:79 >> + if (window) { > > We’d typically move the declaration of window in to the condition part of the if statement here, and below. Yup; will do. >> Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:94 >> + window->addEventListener(eventNames.endedEvent, m_mediaEventListener, true); > > Why does the call to addEventListener not require .get() like the calls to removeEventListener above? Looks like that's because addEventListener() takes a PassRefPtr<> and removeEventListener() takes a bare pointer.
Created attachment 108008 [details] Update patch to address Mark's comments.
Comment on attachment 108008 [details] Update patch to address Mark's comments. Windows build failure is: 4>WebFullScreenManagerProxyMessageReceiver.obj : error LNK2001: unresolved external symbol "private: void __thiscall WebKit::WebFullScreenManagerProxy::setIsPlaying(bool)" (?setIsPlaying@WebFullScreenManagerProxy@WebKit@@AAEX_N@Z)
(In reply to comment #6) > (From update of attachment 108008 [details]) > Windows build failure is: > > > 4>WebFullScreenManagerProxyMessageReceiver.obj : error LNK2001: unresolved external symbol "private: void __thiscall WebKit::WebFullScreenManagerProxy::setIsPlaying(bool)" (?setIsPlaying@WebFullScreenManagerProxy@WebKit@@AAEX_N@Z) I'll address this and resubmit.
Created attachment 108374 [details] Patch Added stubs for setIsPlaying() to each of the ports which implement WebFullScreenManagerProxy.
Comment on attachment 108374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108374&action=review > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:148 > + [self _updatePowerAssertions]; This just scream for KVO (I don't think we use such fancy cocoaisms in WebKit though). :p > Source/WebKit2/UIProcess/mac/WebFullScreenManagerProxyMac.mm:114 > + [[m_webView fullScreenWindowController] setIsPlaying:(isPlaying ? YES : NO)]; As Mark said, I'm surprised you have to do this? Can't you just pass isPlaying directly? > Source/WebKit2/UIProcess/win/WebFullScreenManagerProxyWin.cpp:113 > + // FIXME: Implement notImplemented()? > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:78 > + DOMWindow* window = m_element->document()->domWindow(); Do we need to grab this twice? If not, than these checks could be if (m_element && window) and we lose an indent level. > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:87 > + setIsPlaying(isAnyMoviePlaying()); isAnyMoviePlaying() sounds scary... Do hidden movies count as "playing"? Ones which are display: none, etc? (certainly I can produce cases where movies play and are not visible to the user) will that interact poorly with this code? > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:201 > + nextNode = nextNode->traverseNextNode(m_element.get()); This feels like a for loop, but it might be clearer written in this while style? I trust your judgemnet.
(In reply to comment #9) > (From update of attachment 108374 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108374&action=review > Eric, thanks for looking at this patch. Unfortunately, it has been largely (if not entirely) made irrelevant by the fix for: https://bugs.webkit.org/show_bug.cgi?id=73730. I'll clear the review bit and close this bug. Sorry for wasting your time.
*** This bug has been marked as a duplicate of bug 73730 ***