Chromium plumbing for webkitRequestFullScreen
Created attachment 103577 [details] Patch
Comment on attachment 103577 [details] Patch Attachment 103577 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9358062
Comment on attachment 103577 [details] Patch Attachment 103577 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9352079
Comment on attachment 103577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103577&action=review > Source/WebKit/chromium/public/WebView.h:366 > add another new line here > Source/WebKit/chromium/public/WebView.h:367 > + // Fullscreen ----------------------------------------------------------- nit: new line after the section break > Source/WebKit/chromium/public/WebView.h:369 > + though folks have not been entirely consistent, we have a convention of putting two new lines at the end of a section. so, add another new line here. > Source/WebKit/chromium/public/WebViewClient.h:218 > + virtual void enterFullscreenForElement() { } shouldn't we pass a WebElement to these functions? if not, then the functions should probably be renamed to not have the ForElement suffix. what's the difference between these functions and the ForNode variants? why do we need both? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:914 > + // be restructured to wait for an ACK from the browser that we did enter we normally try to avoid leaving comments in this code that reference the multi-process architecture details of Chromium. they can easily bit-rot since people on the Chromium side might be unlikely to fix up comments here. maybe you should just talk in terms of "the embedder" needing to notify you asynchronously, etc. > Source/WebKit/chromium/src/WebViewImpl.cpp:2680 > + Element* fullscreenElement = page()->mainFrame()->document()->webkitCurrentFullScreenElement(); it might be nice to cache a pointer to the m_page->mainFrame()->document() in a local variable to help readability since you reuse that expression a lot. > Source/WebKit/chromium/src/WebViewImpl.cpp:2681 > + if (fullscreenElement) shouldn't this be a "if (!fullscreenElement)"? i would think that if we have a full screen element that we would want to exit full screen mode. > Source/WebKit/chromium/src/WebViewImpl.h:375 > + virtual void exitFullscreen(); there is actually a section in WebViewImpl.h that is dedicated for listing the methods of WebView in the same order as they appear in the header file. unfortunately, the folks who authored setVisibilityState and graphicsContext3D did not follow that rule. please don't add to that mess :-)
Comment on attachment 103577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103577&action=review >> Source/WebKit/chromium/public/WebView.h:366 > > add another new line here Done. >> Source/WebKit/chromium/public/WebView.h:369 >> + > > though folks have not been entirely consistent, we have a convention of putting two new > lines at the end of a section. so, add another new line here. Done. >> Source/WebKit/chromium/public/WebViewClient.h:218 >> + virtual void enterFullscreenForElement() { } > > shouldn't we pass a WebElement to these functions? if not, then the functions > should probably be renamed to not have the ForElement suffix. > > what's the difference between these functions and the ForNode variants? why > do we need both? The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by jer.noble@apple.com that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?) The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s. I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:914 >> + // be restructured to wait for an ACK from the browser that we did enter > > we normally try to avoid leaving comments in this code that reference the > multi-process architecture details of Chromium. they can easily bit-rot > since people on the Chromium side might be unlikely to fix up comments > here. maybe you should just talk in terms of "the embedder" needing to > notify you asynchronously, etc. Okay. I've updated the comment. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2680 >> + Element* fullscreenElement = page()->mainFrame()->document()->webkitCurrentFullScreenElement(); > > it might be nice to cache a pointer to the m_page->mainFrame()->document() in a local variable > to help readability since you reuse that expression a lot. Done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2681 >> + if (fullscreenElement) > > shouldn't this be a "if (!fullscreenElement)"? i would think that if we have a full screen element that we would want to exit full screen mode. Yep, that's right. Sorry, this was amongst some local changes I'd made but forgot to upload. >> Source/WebKit/chromium/src/WebViewImpl.h:375 >> + virtual void exitFullscreen(); > > there is actually a section in WebViewImpl.h that is dedicated for listing > the methods of WebView in the same order as they appear in the header file. > unfortunately, the folks who authored setVisibilityState and graphicsContext3D > did not follow that rule. please don't add to that mess :-) Done.
Created attachment 104002 [details] Patch
(In reply to comment #5) > The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by jer.noble@apple.com that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?) > > The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s. > > I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen. Ah! Makes sense now. The ForNode methods are probably the leftovers from the work that scherkus' intern did to try to implement fullscreen mode for video. I agree that it should be deleted in place of these new methods. However, I still don't see the point in calling these ForElement. From the point of view of the browser, the only significant difference between F11 and webkitRequestFullscreen is the latter is content-initiated. That means the browser may need to present different UI to the user. The browser won't otherwise care at all about what is focused in the page. I also think it is consistent to drop the ForElement suffix since that normally implies that there will be a corresponding parameter of type WebElement, but you don't pass such a parameter.
Comment on attachment 104002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104002&action=review By the way, if you think that ForElement is the best suffix, then just pass a WebElement parameter like the ChromeClient methods do. > Source/WebKit/chromium/public/WebView.h:367 > + // Fullscreen ----------------------------------------------------------- sorry to be a nut about whitespace, but i had intended for there to be two blank lines above each section heading. apparently, a good number of people haven't been following that convention, but if you scroll up far enough, you'll see that convention in place. sigh :(
(In reply to comment #7) > (In reply to comment #5) > > The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by jer.noble@apple.com that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?) > > > > The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s. > > > > I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen. > > Ah! Makes sense now. The ForNode methods are probably the leftovers from the work that scherkus' intern did to try to implement fullscreen mode for video. I agree that it should be deleted in place of these new methods. > > However, I still don't see the point in calling these ForElement. From the point of view of the browser, the only significant difference between F11 and webkitRequestFullscreen is the latter is content-initiated. That means the browser may need to present different UI to the user. The browser won't otherwise care at all about what is focused in the page. > > I also think it is consistent to drop the ForElement suffix since that normally implies that there will be a corresponding parameter of type WebElement, but you don't pass such a parameter. Yep, that makes sense. I've dropped the suffix. >> Source/WebKit/chromium/public/WebView.h:367 > sorry to be a nut about whitespace, but i had intended for there to be two blank > lines above each section heading. apparently, a good number of people haven't > been following that convention, but if you scroll up far enough, you'll see that > convention in place. sigh :( Sorry, my bad! Done.
Created attachment 104281 [details] Patch
Created attachment 104301 [details] Patch
Created attachment 104302 [details] Patch for landing
Comment on attachment 104302 [details] Patch for landing Rejecting attachment 104302 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: Kit2/ChangeLog Failed to merge in the changes. Patch failed at 0001 Dump-as-markup conversion: editing/pasteboard/paste-text-010.html and editing/pasteboard/paste-text-015.html When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 146. Full output: http://queues.webkit.org/results/9421630
Created attachment 104429 [details] Patch for landing
Comment on attachment 104429 [details] Patch for landing Clearing flags on attachment: 104429 Committed r93381: <http://trac.webkit.org/changeset/93381>
All reviewed patches have been landed. Closing bug.