Summary: | WebKit2: Plumb through the FULLSCREEN_API Chrome client calls | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||
Component: | WebKit2 | Assignee: | Jer Noble <jer.noble> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, sam, webkit-ews, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 55261 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Jer Noble
2011-02-25 15:45:54 PST
Created attachment 83901 [details]
Patch
Attachment 83901 [details] did not build on qt: Build output: http://queues.webkit.org/results/8043559 Attachment 83901 [details] did not build on mac: Build output: http://queues.webkit.org/results/8031612 Created attachment 84077 [details]
Patch
Created attachment 84083 [details]
Patch
Same patch as above, but rebased against current HEAD.
Attachment 84083 [details] did not build on win: Build output: http://queues.webkit.org/results/8075346 Created attachment 84087 [details]
Patch
Updated patch with feedback from Sam Weinig.
Looks like I need to add the new files to the Windows project (and probably the qt one as well). Created attachment 84101 [details]
Patch
Added build commands for new files to the win/ and qt/ ports.
Comment on attachment 84101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84101&action=review > Source/WebKit2/UIProcess/WebFullScreenManagerProxy.cpp:66 > + WebProcess::shared().connection()->send(Messages::WebFullScreenManager::WillEnterFullScreen(), 0); This won't reach the correct (or any?) WebFullScreenManager. It needs to be routed through the WebPage. > Source/WebKit2/UIProcess/WebPageProxy.cpp:1257 > + if (WebFullScreenManagerProxy* manager = this->fullScreenManager()) > + manager->didReceiveMessage(connection, messageID, arguments); > + return; This should probably just call a function like ensureFullScreenManager(), and then you can use the fullScreenManager directly m_fullScreenManager.get(). > Source/WebKit2/UIProcess/WebPageProxy.cpp:1279 > + if (messageID.is<CoreIPC::MessageClassWebFullScreenManagerProxy>()) { > + if (WebFullScreenManagerProxy* manager = this->fullScreenManager()) > + manager->didReceiveSyncMessage(connection, messageID, arguments, reply); > + return; > + } Same here. > Source/WebKit2/UIProcess/WebPageProxy.cpp:1991 > + if (isClosed() || !isValid()) > + return 0; You can remove this. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1390 > + if (m_isClosed) > + return 0; This should not be necessary. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1771 > + if (WebFullScreenManager* fullScreenManager = this->fullScreenManager()) > + fullScreenManager->didReceiveMessage(connection, messageID, arguments); The fullScreenManager should always be non-null here, since only JS can start a fullscreen. I think you can probably just loose the lazy initialization stuff, but that is up to you. (In reply to comment #10) > (From update of attachment 84101 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84101&action=review > > > Source/WebKit2/UIProcess/WebFullScreenManagerProxy.cpp:66 > > + WebProcess::shared().connection()->send(Messages::WebFullScreenManager::WillEnterFullScreen(), 0); > > This won't reach the correct (or any?) WebFullScreenManager. It needs to be routed through the WebPage. Indeed! > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1257 > > + if (WebFullScreenManagerProxy* manager = this->fullScreenManager()) > > + manager->didReceiveMessage(connection, messageID, arguments); > > + return; > > This should probably just call a function like ensureFullScreenManager(), and then you can use the fullScreenManager directly m_fullScreenManager.get(). With the two changes below (removing the isClosed and isValid checks), the fullScreenManager() and fullScreenManagerProxy() functions will always return a non-NULL value, thus making these if checks extraneous. What if I leave the lazy instantiation in, but remove all these unnecessary ifs? > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1279 > > + if (messageID.is<CoreIPC::MessageClassWebFullScreenManagerProxy>()) { > > + if (WebFullScreenManagerProxy* manager = this->fullScreenManager()) > > + manager->didReceiveSyncMessage(connection, messageID, arguments, reply); > > + return; > > + } > > Same here. > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1991 > > + if (isClosed() || !isValid()) > > + return 0; > > You can remove this. Removed. > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1390 > > + if (m_isClosed) > > + return 0; > > This should not be necessary. Removed. > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1771 > > + if (WebFullScreenManager* fullScreenManager = this->fullScreenManager()) > > + fullScreenManager->didReceiveMessage(connection, messageID, arguments); > > The fullScreenManager should always be non-null here, since only JS can start a fullscreen. I think you can probably just loose the lazy initialization stuff, but that is up to you. Same as above. Attachment 84101 [details] did not build on qt: Build output: http://queues.webkit.org/results/8073457 Attachment 84101 [details] did not build on win: Build output: http://queues.webkit.org/results/8073465 Attachment 84101 [details] did not build on win: Build output: http://queues.webkit.org/results/8074425 Created attachment 84242 [details]
Patch
This patch should fix the build issues on platforms which do not enable FULLSCREEN_API, as well as address (some of) Sam's review.
Committed r80619: <http://trac.webkit.org/changeset/80619> |