This patch enables the new document-based Fullscreen API. The ENABLE(FULLSCREEN_API) is already enabled in the BlackBerry WebKit build. My patch turns fullscreen on in the page settings by default and implements the missing ChromeClientBlackBerry::{supports,enter,exit}FullScreenForElement() member functions. The intent of this patch is to support certain web sites such as: http://m.youtube.com/?tsp=1&layout=tablet&player=desktop http://videojs.com These web sites use Javascript to call webkitRequestFullScreen() on a div element. It is also the intent of this patch to allow HTML5 video elements to use the Fullscreen API when webkitEnterFullScreen() is called on them and there is no native fullscreen widget. I am maintaining backward compatibility with the existing native fullscreen mechanism where the Browser chrome implements the entire fullscreen video widget. In order to be able to check this at runtime, I am plumbing through the existing "fullScreenVideoCapable" that QNXStageWebView has always been sending.
Created attachment 141247 [details] Patch
Attachment 141247 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:54: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 141247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141247&action=review > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:724 > +bool ChromeClientBlackBerry::supportsFullScreenForElement(const WebCore::Element* element, bool withKeyboard) > +{ > + return !withKeyboard; > +} Keep in mind that, with the latest W3C specification and the related APIs, keyboard access is always requested. It is of course your port's prerogative, but with this in place this function will always return false when someone calls element->webkitRequestFullscreen(). (As opposed to element->webkitRequestFullScreen().)
(In reply to comment #3) > Keep in mind that, with the latest W3C specification and the related APIs, keyboard access is always requested. It is of course your port's prerogative, but with this in place this function will always return false when someone calls element->webkitRequestFullscreen(). (As opposed to element->webkitRequestFullScreen().) I simply took that line from the Gtk port.
(In reply to comment #2) > Attachment 141247 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > Source/WebCore/ChangeLog:54: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] > Total errors found: 1 in 8 files Apparently in explaining why there are no new tests I'm not allowed to use the phrase "no new tests". So instead of saying "There are no new tests because..." I will say "There are no new dog kennels because...". I had no idea Mr. Lambert was a WebKit contributor.
Created attachment 141265 [details] Patch
(In reply to comment #4) > (In reply to comment #3) > > Keep in mind that, with the latest W3C specification and the related APIs, keyboard access is always requested. It is of course your port's prerogative, but with this in place this function will always return false when someone calls element->webkitRequestFullscreen(). (As opposed to element->webkitRequestFullScreen().) > > I simply took that line from the Gtk port. The GTK port probably implemented that function before the new W3C APIs were added. I'm not saying you should change your port's behavior, but I just wanted to make you aware of it. :)
(In reply to comment #5) > Apparently in explaining why there are no new tests I'm not allowed to use the phrase "no new tests". So instead of saying "There are no new tests because..." I will say "There are no new dog kennels because...". I had no idea Mr. Lambert was a WebKit contributor. Or you could lose the snark and file a bug against the style checker.
(In reply to comment #8) > Or you could lose the snark and file a bug against the style checker. Sorry. My attempt at humour. Won't make sense to people not familiar with Monty Python. Look up "Monty Python mattress" on YouTube :) The style checker is fine, by the way. My first patch picked up the wrong Changelogs.
Comment on attachment 141265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141265&action=review Looks good, can be improved some more. > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:128 > + element->exitFullscreen(); You could combine: if (HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient())) element->exitFullscreen(); > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:129 > } Is it possible to have a test for this or is an existing test covering it? > Source/WebKit/blackberry/Api/WebPage.cpp:6017 > + coreSettings->setFullScreenEnabled(true); Do we want to turn it off at any time? Is it setting dependent? > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:740 > + // This is where we would hide the browser's chrome if we wanted to Lacks period at end. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:749 > + // The Browser chrome has its own fullscreen video widget Ditto. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:753 > + // if hidden above Ditto.
(In reply to comment #10) > You could combine: > if (HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient())) > element->exitFullscreen(); I took your suggestion and made this minor change for my next patch. > > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:129 > > } > > Is it possible to have a test for this or is an existing test covering it? This is just a null pointer check to avoid a seg fault upon Browser shutdown. I don't think most null pointer checks need a Layout test. > > Source/WebKit/blackberry/Api/WebPage.cpp:6017 > > + coreSettings->setFullScreenEnabled(true); > > Do we want to turn it off at any time? Is it setting dependent? It is not setting dependent. I can't think of a reason to ever disable fullscreen elements in a document. The web page can disallow fullscreen per frame/iframe, as per the Fullscreen API spec. > Lacks period at end. I fixed the missing periods at the end of my single sentence comments. Will be in my next patch.
Created attachment 141483 [details] Patch
> > Is it possible to have a test for this or is an existing test covering it? > > This is just a null pointer check to avoid a seg fault upon Browser shutdown. I don't think most null pointer checks need a Layout test. That is an invalid assumption.
(In reply to comment #13) > > This is just a null pointer check to avoid a seg fault upon Browser shutdown. I don't think most null pointer checks need a Layout test. > > That is an invalid assumption. Can you give more details, Antonio? There are many thousands of null pointer checks in WebKit code. For example, lines like this are extremely common: if (!page() || !page()->settings()->fullScreenEnabled()) break; Often there are multiple tests per line. Null checks can be considered a good, preventive coding practice. Are you saying that each individual null check needs its own layout test?
(In reply to comment #14) > (In reply to comment #13) > > > This is just a null pointer check to avoid a seg fault upon Browser shutdown. I don't think most null pointer checks need a Layout test. > > > > That is an invalid assumption. > > Can you give more details, Antonio? There are many thousands of null pointer checks in WebKit code. For example, lines like this are extremely common: > > if (!page() || !page()->settings()->fullScreenEnabled()) > break; > > Often there are multiple tests per line. Null checks can be considered a good, preventive coding practice. Are you saying that each individual null check needs its own layout test? Many existing null-checks came with layout tests, that is what I am saying. If you are fixing a crash with a null check, you can in many cases test it. Shutdown is a particular situation where null-checks can be more easily accepted without a test. Otherwise, the author has to say why it is not possible to come up with a test.
Since this is a shutdown case, can this particular null check be r+'d without a test? I am trying to get this bug committed so that other related problems can be filed.
Comment on attachment 141483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141483&action=review We sure can review it. I have a question, a suggestion and a nitpick :) > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:127 > if (isFullscreen()) { > - HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient()); > - element->exitFullscreen(); > + if (HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient())) > + element->exitFullscreen(); I still think it would have to be done on a separate patch. Jer? > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:724 > +#if ENABLE(FULLSCREEN_API) > +bool ChromeClientBlackBerry::supportsFullScreenForElement(const WebCore::Element* element, bool withKeyboard) > +{ > + return !withKeyboard; > +} I reviewed a patch for the EFL port implementation of FULLSCREEN and they are simply ignoring this 'withKeyboard' parameter, and making use of 'element' for everything. could you explain me the difference, Max? > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:130 > + virtual bool supportsFullScreenForElement(const Element*, bool); we could name this bool parameter, I think.
Comment on attachment 141483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141483&action=review >> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:127 >> MediaPlayerPrivate::~MediaPlayerPrivate() >> { >> if (isFullscreen()) { >> - HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient()); >> - element->exitFullscreen(); >> + if (HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient())) >> + element->exitFullscreen(); > > I still think it would have to be done on a separate patch. Jer? I agree. A testcase would be helpful as well. Also, I realize this code was already here, but since you're in here... :) If you need to pass a message up to the HTMLMediaElement, Instead of casting the client pointer to an HTMLMediaElement, you should define a new pure virtual function of MediaPlayerClient, like: virtual void mediaPlayerExitFullscreen() = 0; ...And override it in HTMLMediaElement.
Created attachment 141994 [details] Patch
Comment on attachment 141994 [details] Patch I have separated out the WebCore change into https://bugs.webkit.org/show_bug.cgi?id=86494 Hopefully there is nothing preventing this patch from getting an r+ now :)
Created attachment 142000 [details] Patch
Comment on attachment 142000 [details] Patch Included Antonio's nit pick.
(In reply to comment #17) > I still think it would have to be done on a separate patch. Jer? Separate bug created: https://bugs.webkit.org/show_bug.cgi?id=86494 > I reviewed a patch for the EFL port implementation of FULLSCREEN and they are simply ignoring this 'withKeyboard' parameter, and making use of 'element' for everything. > > could you explain me the difference, Max? As far as I know, this just restricts support of fullscreen elements to cases not requiring keyboard input. It also matches the current code in the EFL and GTK ports. As pointed out by Jer in comment #3, this is changing soon so I will revisit this as needed. > we could name this bool parameter, I think. Done in latest patch. Thanks!
Comment on attachment 142000 [details] Patch Clearing flags on attachment: 142000 Committed r117148: <http://trac.webkit.org/changeset/117148>
All reviewed patches have been landed. Closing bug.
(In reply to comment #25) > All reviewed patches have been landed. Closing bug. Seems like this patch broke GTK, #0 0x00007f9928b55f60 in WebCore::Frame::settings() const () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 #0 0x00007f9928b55f60 in WebCore::Frame::settings() const () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 #1 0x00007f99287aeeb1 in WebCore::DOMImplementation::createDocument(WTF::String const&, WebCore::Frame*, WebCore::KURL const&, bool) () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 #2 0x00007f9928f9c185 in WebCore::DOMParser::parseFromString(WTF::String const&, WTF::String const&) () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 #3 0x00007f99290dda46 in WebCore::jsDOMParserPrototypeFunctionParseFromString(JSC::ExecState*) () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 I'll provide more details once the Debug bot builds this patch.
Hum sorry for the noise, I guess this patch is not the culprit, really being Blackberry-specific.
Closing bug for patch that landed a long time ago.