Expose setFullscreen method on WebPluginContainer
Created attachment 104791 [details] Patch
Comment on attachment 104791 [details] Patch Attachment 104791 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9482076
Created attachment 104792 [details] Patch
API changes sound like a fishd review.
Comment on attachment 104792 [details] Patch I thought the new fullscreen goodness allowed you to make any element fullscreen?
(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 :)
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.
(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.
Created attachment 104953 [details] Patch
(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.
Comment on attachment 104953 [details] Patch Clearing flags on attachment: 104953 Committed r93714: <http://trac.webkit.org/changeset/93714>
All reviewed patches have been landed. Closing bug.