[webkit-reviews] review denied: [Bug 56318] WebKit2: WKTR should support WebKit2 full screen APIs : [Attachment 88366] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 56318: WebKit2: WKTR should support WebKit2 full screen APIs
https://bugs.webkit.org/show_bug.cgi?id=56318

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88366&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:143
> +void WKBundlePageWillEnterFullScreenForElement(WKBundlePageRef pageRef,
WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +	   return;
> +
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitWillEnterFullScreenForElement(element);
> +}
> +
> +void WKBundlePageDidEnterFullScreenForElement(WKBundlePageRef pageRef,
WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +	   return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitDidEnterFullScreenForElement(element);
> +}
> +
> +void WKBundlePageWillExitFullScreenForElement(WKBundlePageRef pageRef,
WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +	   return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitWillExitFullScreenForElement(element);
> +}
> +
> +void WKBundlePageDidExitFullScreenForElement(WKBundlePageRef pageRef,
WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +	   return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitDidExitFullScreenForElement(element);
> +}

All the logic should be in WebPage, not in the API layer.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:247
> +#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API
> +// Full Screen client

Please don't put #ifdefs like these in API headers.  Clients will not not know
the correct value to set.  Instead, the functions should just be no-ops if the
feature is not supported.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:257
> +    WKBundlePageEnterFullScreenForElement				  
enterFullScreenForElement;
> +    WKBundlePageExitFullScreenForElement				  
exitFullScreenForElement;

We usually prefix the set type of clients with Will or Did.

>
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFullScreenClient.h:4
6
> +    bool supportsFullScreen(WebPage*, bool withKeyboard);

Instead of a bool, this should probably be an enum.