Bug 85948 - [BlackBerry] JavaScript method webkitEnterFullscreen is partially broken
Summary: [BlackBerry] JavaScript method webkitEnterFullscreen is partially broken
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-08 19:35 PDT by Peter Wang
Modified: 2015-01-17 20:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2012-05-08 21:26 PDT, Peter Wang
rwlbuis: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Wang 2012-05-08 19:35:30 PDT
RIM PR#140677
On <video> objects, the method .webkitEnterFullscreen() overlays a mask over
the screen but does not resize the video the the full size of the screen and
there are no controls to exit fullscreen mode or to manipulate the video.
Comment 1 Peter Wang 2012-05-08 21:26:40 PDT
Created attachment 140862 [details]
Patch
Comment 2 Rob Buis 2012-05-09 10:48:33 PDT
Comment on attachment 140862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140862&action=review

Looks good, can be improved some more.

> Source/WebKit/blackberry/ChangeLog:8
> +        When FULLSCREEN_API is enabled, we should at least overrid

Typo: override

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:694
> +bool ChromeClientBlackBerry::supportsFullScreenForElement(const Element* element, bool withKeyboard)

Does what we return depend on withKeyboard? If not you can remove the name, just say , bool).

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:701
> +    Document* doc = element->document();

Might as well name it document instead of doc, WebKit does not prefer abbriviations.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:705
> +    if (doc->frame()) {

You can combine this with above:
if (!doc || !doc->frame())
    return;

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:707
> +        m_webPagePrivate->enterFullscreenForNode(static_cast<Node*>(element));

I don't think you need the static_cast, element is a Node*.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:714
> +    Document* doc = element->document();

Might as well name it document instead of doc, WebKit does not prefer abbriviations.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:718
> +    if (doc->frame()) {

You can combine this with above:
if (!doc || !doc->frame())
    return;

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:720
> +        m_webPagePrivate->exitFullscreenForNode(static_cast<Node*>(element));

I don't think you need the static_cast, element is a Node*.

> ChangeLog:8
> +        * ManualTests/blackberry/video-fullscreen-by-JS.html: Added.

Can it be an automated test?

> ManualTests/blackberry/video-fullscreen-by-JS.html:19
> +<video id="vv" src="./resources/dizzy.mp4" controls>Your browser doesn't support the <code>Video</code> element</video>

Ia vv really a good name? Better make it more descriptive.