[Webkit-unassigned] [Bug 85948] [BlackBerry] JavaScript method webkitEnterFullscreen is partially broken

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85948


Rob Buis <rwlbuis at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #140862|review?                     |review-
               Flag|                            |




--- Comment #2 from Rob Buis <rwlbuis at gmail.com>  2012-05-09 10:48:33 PST ---
(From update of attachment 140862)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.