Bug 109276 - [EFL][WK2] WKViewCreate*() API should not accept NULL parameters
Summary: [EFL][WK2] WKViewCreate*() API should not accept NULL parameters
Status: RESOLVED DUPLICATE of bug 112794
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 02:31 PST by Chris Dumez
Modified: 2013-03-20 06:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.06 KB, patch)
2013-02-08 03:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-02-08 02:31:27 PST
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.
Comment 1 Chris Dumez 2013-02-08 03:23:57 PST
This patch is inspired by Benjamin's comment at:
https://bugs.webkit.org/show_bug.cgi?id=107820#c14
Comment 2 Chris Dumez 2013-02-08 03:54:15 PST
Created attachment 187283 [details]
Patch
Comment 3 Mikhail Pozdnyakov 2013-02-08 04:06:37 PST
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() ?
Comment 4 Chris Dumez 2013-02-08 04:14:00 PST
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(); }
Comment 5 Mikhail Pozdnyakov 2013-02-08 04:32:06 PST
(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.)
Comment 6 Chris Dumez 2013-02-08 04:45:43 PST
(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.
Comment 7 Alexey Proskuryakov 2013-02-08 09:49:23 PST
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.
Comment 8 Chris Dumez 2013-02-08 09:52:28 PST
(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!
Comment 9 Mikhail Pozdnyakov 2013-02-08 13:38:01 PST
(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
Comment 10 Benjamin Poulain 2013-02-10 18:11:16 PST
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.
Comment 11 Kenneth Rohde Christiansen 2013-03-20 06:06:30 PDT

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