[webkit-reviews] review denied: [Bug 85948] [BlackBerry] JavaScript method webkitEnterFullscreen is partially broken : [Attachment 140862] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Peter Wang
<peter.wang at torchmobile.com.cn>'s request for review:
Bug 85948: [BlackBerry] JavaScript method webkitEnterFullscreen is partially
broken
https://bugs.webkit.org/show_bug.cgi?id=85948

Attachment 140862: Patch
https://bugs.webkit.org/attachment.cgi?id=140862&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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.