Bug 66746 – Expose setFullscreen method on WebPluginContainer

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Apthorp]

Expose setFullscreen method on WebPluginContainer

[Jeremy Apthorp]

Created attachment 104791 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104791 [details]
Patch

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

[Jeremy Apthorp]

Created attachment 104792 [details]
Patch

[Eric Seidel (no email)]

API changes sound like a fishd review.

[Eric Seidel (no email)]

Comment on attachment 104792 [details]
Patch

I thought the new fullscreen goodness allowed you to make any element fullscreen?

[Jeremy Apthorp]

(In reply to comment #5)
> (From update of attachment 104792 [details])
> I thought the new fullscreen goodness allowed you to make any element fullscreen?

They do. The reason I put the code in WebPluginContainer instead of WebElement is that WebPluginContainer has a direct reference to a WebCore::HTMLPlugInElement, and it seemed silly to wrap it in WebElement only to unwrap it and call a method on it immediately afterwards.

I think if I moved the function to WebElement, I would mirror the WebKit API, and have:

webelement->requestFullScreen(flags);

and

webelement->document()->cancelFullScreen();

which would necessitate adding 3 methods:
- void WebElement::requestFullScreen(flags)
- WebDocument WebElement::document()
- void WebDocument::cancelFullScreen()


If that seems cleaner/better to someone with a clearer understanding of the WebFoo wrapper APIs than me, I'm happy to reimplement it that way :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 104792 [details]
Patch

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

> Source/WebKit/chromium/public/WebPluginContainer.h:93
> +    virtual void setFullscreen(bool) = 0;

Since this is implemented as webkitRequestFullScreen, it could fail.  In other
words, this method should probably have a name indicative of the fact that this
could fail.

Another idea here, is that we could just expose a requestFullScreen function on
WebElement.  WebPluginContainer provides a way to get the WebElement containing
the plugin.  We could also add a cancelFullScreen method on WebElement.  This
might be the better way to expose this functionality to the embedder of WebKit.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 104792 [details] [details])
> > I thought the new fullscreen goodness allowed you to make any element fullscreen?
> 
> They do. The reason I put the code in WebPluginContainer instead of WebElement is that WebPluginContainer has a direct reference to a WebCore::HTMLPlugInElement, and it seemed silly to wrap it in WebElement only to unwrap it and call a method on it immediately afterwards.
> 
> I think if I moved the function to WebElement, I would mirror the WebKit API, and have:
> 
> webelement->requestFullScreen(flags);
> 
> and
> 
> webelement->document()->cancelFullScreen();
> 
> which would necessitate adding 3 methods:
> - void WebElement::requestFullScreen(flags)
> - WebDocument WebElement::document()
> - void WebDocument::cancelFullScreen()
> 
> 
> If that seems cleaner/better to someone with a clearer understanding of the WebFoo wrapper APIs than me, I'm happy to reimplement it that way :)

Heh, sorry... I only looked at the patch before commenting.  I think it is better to add the more generic APIs.  We've found that to be the better way in the past.

I have to note though that if this is for Pepper, we may have a bit of an issue to worry about.  Assuming we want to require a user gesture before accepting a request to make an element fullscreen, we might have trouble here.  You see with pepper, events are dispatched asynchronously.  This means that when the plugin sees the event, webkit has already cleared its user gesture flag.

[Jeremy Apthorp]

Created attachment 104953 [details]
Patch

[Jeremy Apthorp]

(In reply to comment #7)
> (From update of attachment 104792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104792&action=review
> 
> > Source/WebKit/chromium/public/WebPluginContainer.h:93
> > +    virtual void setFullscreen(bool) = 0;
> 
> Since this is implemented as webkitRequestFullScreen, it could fail.  In other
> words, this method should probably have a name indicative of the fact that this
> could fail.
> 
> Another idea here, is that we could just expose a requestFullScreen function on
> WebElement.  WebPluginContainer provides a way to get the WebElement containing
> the plugin.  We could also add a cancelFullScreen method on WebElement.  This
> might be the better way to expose this functionality to the embedder of WebKit.

I've rearranged this to reflect the arrangement of the underlying WebKit implementation.

[WebKit Review Bot]

Comment on attachment 104953 [details]
Patch

Clearing flags on attachment: 104953

Committed r93714: <http://trac.webkit.org/changeset/93714>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.