[webkit-reviews] review denied: [Bug 108062] [EFL][WK2] Encapsulate Ewk View evas smart object code inside EwkView class : [Attachment 186638] patch v6

[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 108062: [EFL][WK2] Encapsulate Ewk View evas smart object code inside
EwkView class
https://bugs.webkit.org/show_bug.cgi?id=108062

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

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


> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> +static inline WKViewRef createWKView(Evas* canvas, WKContextRef contextRef,
WKPageGroupRef pageGroupRef, EwkView::ViewBehavior behavior)
>  {
> -    EwkView* ewkView = ewk_view_base_add(canvas, contextRef, pageGroupRef,
EwkView::LegacyBehavior);
> -    if (!ewkView)
> +    RefPtr<EwkContext> context = contextRef ? EwkContext::create(contextRef)
: EwkContext::defaultContext();

Something bother me about the way you use contextRef.

If you are passing a null WKContextRef to an API, you are using it wrong.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:71
> +#include <wtf/Compiler.h>

This should not be needed.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1238
> +    red = clampTo(red, 0, alpha);
> +    green = clampTo(green, 0, alpha);
> +    blue = clampTo(blue, 0, alpha);

Why do you clamp color value by alpha?
It is a little weird to get colors without color profile. Is that in a default
color space?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1425
> +    if (UNLIKELY(!evasSmart)) {
> +	   EINA_LOG_CRIT("%p (%s) is not a smart object!", evasObject,
evasObjectType ? evasObjectType : "(null)");
> +	   return false;
> +    }
> +
> +    const Evas_Smart_Class* smartClass = evas_smart_class_get(evasSmart);
> +    if (UNLIKELY(!smartClass)) {
> +	   EINA_LOG_CRIT("%p (%s) is not a smart class object!", evasObject,
evasObjectType ? evasObjectType : "(null)");
> +	   return false;
> +    }
> +
> +    if (UNLIKELY(smartClass->data != smartClassName)) {
> +	   EINA_LOG_CRIT("%p (%s) is not of an ewk_view (need %p, got %p)!",
evasObject, evasObjectType ? evasObjectType : "(null)",
> +	       smartClassName, smartClass->data);
> +	   return false;
> +    }

Unless the branches are creating a hot path, which I highly doubt given the
code, UNLIKELY is a bad idea.

> Source/WebKit2/UIProcess/API/efl/EwkView.h:226
> +    ~EwkView(); // Will be deleted when Evas Object is deleted.

This comment is not helping the reader.

If you want to document the object lifecycle, make a complete explanation in a
visible place (above the class declaration for example).

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:103
> +    WKContextRef contextRef = toAPI(page()->process()->context());
> +    m_inspectorView =
EwkView::createEvasObject(ecore_evas_get(m_inspectorWindow), contextRef ?
EwkContext::create(contextRef) : EwkContext::defaultContext(),
toAPI(inspectorPageGroup()), EwkView::LegacyBehavior);

You get null context our of WebProcessProxy->context()?