[webkit-reviews] review denied: [Bug 110877] [WK2][EFL] Encapsulate view states set-up within WebView : [Attachment 192920] patch v2

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Mikhail Pozdnyakov
<mikhail.pozdnyakov at intel.com>'s request for review:
Bug 110877: [WK2][EFL] Encapsulate view states set-up within WebView
https://bugs.webkit.org/show_bug.cgi?id=110877

Attachment 192920: patch v2
https://bugs.webkit.org/attachment.cgi?id=192920&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192920&action=review


> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:66
> +void WKViewSetFocused(WKViewRef viewRef, bool flag)

Not a fan of "flag" for the argument name.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:76
> +void WKViewSetVisible(WKViewRef viewRef, bool flag)

Ditto.

SetIsVisible maybe?

> Source/WebKit2/UIProcess/efl/WebView.cpp:95
> +void WebView::setFocused(bool focused)
> +{
> +    if (m_focused == focused)
> +	   return;
> +
> +    m_focused = focused;
> +    m_page->viewStateDidChange(WebPageProxy::ViewIsFocused |
WebPageProxy::ViewWindowIsActive);
> +}

setFocused(false) call viewStateDidChange(WebPageProxy::ViewIsFocused)???

> Source/WebKit2/UIProcess/efl/WebView.cpp:104
> +void WebView::setVisible(bool visible)
> +{
> +    if (m_visible == visible)
> +	   return;
> +
> +    m_visible = visible;
> +    m_page->viewStateDidChange(WebPageProxy::ViewIsVisible);
> +}

You duplicate states between WebPageProxy and WebView. This is a really bad
idea.

> Source/WebKit2/UIProcess/efl/WebView.cpp:305
>  bool WebView::isViewFocused()
>  {
> -    return m_ewkView->isFocused();
> +    return isFocused();
>  }
>  
>  bool WebView::isViewVisible()
>  {
> -    return m_ewkView->isVisible();
> +    return isVisible();
>  }

Why don't you get rid of those APIs?