[webkit-reviews] review denied: [Bug 86129] [BlackBerry] Enable the Fullscreen API : [Attachment 141265] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Max Feil <mfeil at rim.com>'s request for
review:
Bug 86129: [BlackBerry] Enable the Fullscreen API
https://bugs.webkit.org/show_bug.cgi?id=86129

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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:12
8
> +	       element->exitFullscreen();

You could combine:
if (HTMLMediaElement* element =
static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient()))
  element->exitFullscreen();

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:12
9
>      }

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.