Bug 69063 – [Chromium] Forward fullscreenchanged event to plugins

• Component: New Bugs
• Status: RESOLVED WONTFIX

[James Kozianski]

Forward fullscreenchanged event to plugins

[James Kozianski]

Created attachment 109125 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109125 [details]
Patch

Attachment 109125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9880864

[James Kozianski]

Please note, this patch depends on the Chromium-side patch here: http://codereview.chromium.org/8065026.

Created attachment 109910 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 109910 [details]
Patch

Attachment 109910 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9969123

[Darin Fisher (:fishd, Google)]

Comment on attachment 109910 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109910&action=review

> Source/WebKit/chromium/public/WebPlugin.h:86
> +    virtual void onFullscreenChange() { }

nit: we normally use the convention of didSomeEvent or willDoSomeEvent instead of onEvent.  that way the before/after relation is made clear.

we already have WebViewClient::{enter,exit}FullscreenForNode.  that is driven from ChromeClient::{enter,exit}FullscreenForNode.
had you considered using that?

Created attachment 110026 [details]
Patch

Comment on attachment 110026 [details]
Patch

Renamed onFullscreenChange to handleFullscreenChangeEvent.

There are already did and willDo versions of fullscreen functions and their purpose is different - performing an action as opposed to notifying about it. Please let me know if the new name is appropriate.

Comment on attachment 110026 [details]
Patch

The existing implementation for entering/exiting fullscreen mode uses WebKit::ChromeClientImpl::{enter,exit}FullscreenForElement. WebKit::ChromeClientImpl::{enter,exit}FullscreenForNode is unused and calls UNIMPLEMENTED code in RenderView.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110026&action=review

> Source/WebKit/chromium/ChangeLog:63
> +>>>>>>> .r96838

oops

> Source/WebKit/chromium/public/WebPlugin.h:86
> +    virtual void handleFullscreenChangeEvent() { }

nit: didChangeFullscreenState would probably be more consistent with the
way we name other notifications of state changes.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:194
> +        m_webPlugin->handleFullscreenChangeEvent();

i'm still not sure that this is the best solution.  can the page intercept this
event and prevent its delivery to the WebPluginContainerImpl?  if it did that,
then perhaps we might miss a notification that we switched into fullscreen mode?
could a page construct a fullscreenchange event and dispatch it manually to the
plugin to confuse the plugin?

this is why i was thinking that we should use the ChromeClient methods as a
signal since those cannot be intercepted or faked.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=230802