Bug 86129 – [BlackBerry] Enable the Fullscreen API

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Max Feil]

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.

[Max Feil]

Created attachment 141247 [details]
Patch

[WebKit Review Bot]

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.

[Jer Noble]

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().)

[Max Feil]

(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.

[Max Feil]

(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.

[Max Feil]

Created attachment 141265 [details]
Patch

[Jer Noble]

(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. :)

[Eric Carlson]

(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.

[Max Feil]

(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.

[Rob Buis]

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.

[Max Feil]

(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.

[Max Feil]

Created attachment 141483 [details]
Patch

[Antonio Gomes]

> > 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.

[Max Feil]

(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?

[Antonio Gomes]

(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.

[Max Feil]

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.

[Antonio Gomes]

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.

[Jer Noble]

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.

[Max Feil]

Created attachment 141994 [details]
Patch

[Max Feil]

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 :)

[Max Feil]

Created attachment 142000 [details]
Patch

[Max Feil]

Comment on attachment 142000 [details]
Patch

Included Antonio's nit pick.

[Max Feil]

(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!

[WebKit Review Bot]

Comment on attachment 142000 [details]
Patch

Clearing flags on attachment: 142000

Committed r117148: <http://trac.webkit.org/changeset/117148>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(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.

[Philippe Normand]

Hum sorry for the noise, I guess this patch is not the culprit, really being Blackberry-specific.

[Max Feil]

Closing bug for patch that landed a long time ago.