Bug 65061 – REGRESSION: ClickToFlash HTML5 player doesn't disable screen dimming when full screen

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Media

[Jon]

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.

[Alexey Proskuryakov]

<rdar://problem/9828140>

[Jer Noble]

Created attachment 105053 [details]
Patch

[Mark Rowe (bdash)]

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?

[Jer Noble]

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.

[Jer Noble]

Created attachment 108008 [details]
Update patch to address Mark's comments.

[Darin Adler]

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)

[Jer Noble]

(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.

[Jer Noble]

Created attachment 108374 [details]
Patch

Added stubs for setIsPlaying() to each of the ports which implement WebFullScreenManagerProxy.

[Eric Seidel (no email)]

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.

[Jer Noble]

(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.

[Jer Noble]

*** This bug has been marked as a duplicate of bug 73730 ***