[EFL][WK2] WKViewCreate*() API should not accept NULL parameters

• Component: WebKit EFL
• Status: RESOLVED DUPLICATE

[Chris Dumez]

WKViewCreate*() C API in EFL currently allows the user to pass NULL parameters for context and page group. We should prevent that to clarify the behavior and make it more consistent with other C API functions.

[Chris Dumez]

This patch is inspired by Benjamin's comment at:
https://bugs.webkit.org/show_bug.cgi?id=107820#c14

[Chris Dumez]

Created attachment 187283 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 187283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> +    Evas_Object* evasObject = EwkView::createEvasObject(canvas, EwkContext::create(contextRef), pageGroupRef, behavior);

this cannot return '0' any more, right? So the following 'if (!evasObject)' should be either removed or substituted with assertion.

> Source/WebKit2/UIProcess/WebContext.h:145
> +    WebPageGroup* defaultPageGroup() { return m_defaultPageGroup.get(); }

PassRefPtr<WebPageGroup> defaultPageGroup() ?

[Chris Dumez]

Comment on attachment 187283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
>> +    Evas_Object* evasObject = EwkView::createEvasObject(canvas, EwkContext::create(contextRef), pageGroupRef, behavior);
> 
> this cannot return '0' any more, right? So the following 'if (!evasObject)' should be either removed or substituted with assertion.

It can, if toSmartData(evasObject) returns NULL.

>> Source/WebKit2/UIProcess/WebContext.h:145
>> +    WebPageGroup* defaultPageGroup() { return m_defaultPageGroup.get(); }
> 
> PassRefPtr<WebPageGroup> defaultPageGroup() ?

This is consistent with other getters in this class. For e.g.
WebIconDatabase* iconDatabase() const { return m_iconDatabase.get(); }

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 187283 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> >> +    Evas_Object* evasObject = EwkView::createEvasObject(canvas, EwkContext::create(contextRef), pageGroupRef, behavior);
> > 
> > this cannot return '0' any more, right? So the following 'if (!evasObject)' should be either removed or substituted with assertion.
> 
> It can, if toSmartData(evasObject) returns NULL.
> 
So don't you think it's worth adding
--- a/Source/WebKit2/UIProcess/API/efl/EwkView.cpp
+++ b/Source/WebKit2/UIProcess/API/efl/EwkView.cpp
@@ -291,12 +291,7 @@ Evas_Object* EwkView::createEvasObject(Evas* canvas, Evas_Smart* smart, PassRefP
     EINA_SAFETY_ON_NULL_RETURN_VAL(evasObject, 0);
 
     Ewk_View_Smart_Data* smartData = toSmartData(evasObject);
-    if (!smartData) {
-        evas_object_del(evasObject);
-        return 0;
-    }
-
-    ASSERT(!smartData->priv);
+    ASSERT(smartData && !smartData->priv);

to fix it? :) 

(well, not sure on whether it's within the scope of the patch, but this also would make sure that smart data had been set at EwkView::handleEvasObjectAdd which has to be right.)

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 187283 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:32
> > >> +    Evas_Object* evasObject = EwkView::createEvasObject(canvas, EwkContext::create(contextRef), pageGroupRef, behavior);
> > > 
> > > this cannot return '0' any more, right? So the following 'if (!evasObject)' should be either removed or substituted with assertion.
> > 
> > It can, if toSmartData(evasObject) returns NULL.
> > 
> So don't you think it's worth adding
> --- a/Source/WebKit2/UIProcess/API/efl/EwkView.cpp
> +++ b/Source/WebKit2/UIProcess/API/efl/EwkView.cpp
> @@ -291,12 +291,7 @@ Evas_Object* EwkView::createEvasObject(Evas* canvas, Evas_Smart* smart, PassRefP
>      EINA_SAFETY_ON_NULL_RETURN_VAL(evasObject, 0);
> 
>      Ewk_View_Smart_Data* smartData = toSmartData(evasObject);
> -    if (!smartData) {
> -        evas_object_del(evasObject);
> -        return 0;
> -    }
> -
> -    ASSERT(!smartData->priv);
> +    ASSERT(smartData && !smartData->priv);
> 
> to fix it? :) 
> 
> (well, not sure on whether it's within the scope of the patch, but this also would make sure that smart data had been set at EwkView::handleEvasObjectAdd which has to be right.)

Probably out of scope for this patch but it sounds like a good idea to me.

[Alexey Proskuryakov]

Comment on attachment 187283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review

>>> Source/WebKit2/UIProcess/WebContext.h:145
>>> +    WebPageGroup* defaultPageGroup() { return m_defaultPageGroup.get(); }
>> 
>> PassRefPtr<WebPageGroup> defaultPageGroup() ?
> 
> This is consistent with other getters in this class. For e.g.
> WebIconDatabase* iconDatabase() const { return m_iconDatabase.get(); }

PassRefPtr means that ownership is being transferred, and it's an optimization for this case. Using it here would be less efficient than a plain pointer, and misleading for readers.

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 187283 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review
> 
> >>> Source/WebKit2/UIProcess/WebContext.h:145
> >>> +    WebPageGroup* defaultPageGroup() { return m_defaultPageGroup.get(); }
> >> 
> >> PassRefPtr<WebPageGroup> defaultPageGroup() ?
> > 
> > This is consistent with other getters in this class. For e.g.
> > WebIconDatabase* iconDatabase() const { return m_iconDatabase.get(); }
> 
> PassRefPtr means that ownership is being transferred, and it's an optimization for this case. Using it here would be less efficient than a plain pointer, and misleading for readers.

Thanks for the explanation Alexey!

[Mikhail Pozdnyakov]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 187283 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review
> > 
> > >>> Source/WebKit2/UIProcess/WebContext.h:145
> > >>> +    WebPageGroup* defaultPageGroup() { return m_defaultPageGroup.get(); }
> > >> 
> > >> PassRefPtr<WebPageGroup> defaultPageGroup() ?
> > > 
> > > This is consistent with other getters in this class. For e.g.
> > > WebIconDatabase* iconDatabase() const { return m_iconDatabase.get(); }
> > 
> > PassRefPtr means that ownership is being transferred, and it's an optimization for this case. Using it here would be less efficient than a plain pointer, and misleading for readers.
> 
> Thanks for the explanation Alexey!
+1

[Benjamin Poulain]

Comment on attachment 187283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187283&action=review

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:81
> +WKPageGroupRef WKContextGetDefaultPageGroup(WKContextRef contextRef)
> +{
> +    return toAPI(toImpl(contextRef)->defaultPageGroup());
> +}
> +

Hum, this looks like fixing one of the EFL quirks by adding an new EFL quirk. :(

If anything, I'd rather get rid of the default page group. I'll have to look into that.

[Kenneth Rohde Christiansen]

*** This bug has been marked as a duplicate of bug 112794 ***