Bug 142033 – [EFL] Expose JavaScript binding interface through ewk_extension

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

SSIA

[Ryuan Choi]

Created attachment 247378 [details]
Patch

[Ryuan Choi]

Created attachment 247381 [details]
Patch

[Ryuan Choi]

Created attachment 247384 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 247384 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:15
> +        DESTINATION "include/${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR}/JavaScriptCore"

You already define EWEBKIT_EXTENSION_MANAGER_INSTALL_DIR. Can't we use it here as well ?

If now, how about adding new variable to replace this long variable ? "include/${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR}"

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:42
> +    return 0;

nullptr ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:114
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(ewkPage, 0);

0 -> nullptr ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:118
> +        return 0;

ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:131
> +void ewk_page_client_del(Ewk_Page* ewkPage, Ewk_Page_Client* client)

I fail to find declaration of ewk_page_client_del(). Where is it declared ?

[Ryuan Choi]

Created attachment 248827 [details]
Patch

[Ryuan Choi]

(In reply to comment #4)
> Comment on attachment 247384 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247384&action=review
> 
> > Source/JavaScriptCore/PlatformEfl.cmake:15
> > +        DESTINATION "include/${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR}/JavaScriptCore"
> 
> You already define EWEBKIT_EXTENSION_MANAGER_INSTALL_DIR. Can't we use it
> here as well ?
> 
> If now, how about adding new variable to replace this long variable ?
> "include/${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR}"
> 
I added HEADER_INSTALL_DIR

> > Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:42
> > +    return 0;
> 
> nullptr ?
> 
Done

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:114
> > +    EINA_SAFETY_ON_NULL_RETURN_VAL(ewkPage, 0);
> 
> 0 -> nullptr ?
> 
Done

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:118
> > +        return 0;
> 
> ditto.
> 
Done

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:131
> > +void ewk_page_client_del(Ewk_Page* ewkPage, Ewk_Page_Client* client)
> 
> I fail to find declaration of ewk_page_client_del(). Where is it declared ?

My mistake. I added it in header files and also added some documentions for ewk_page.h

[Gyuyoung Kim]

Comment on attachment 248827 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:81
> +{

Do we need to delete the registered client using ewk_page_client_del() ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:81
> +    self->m_pageMap.add(page, std::unique_ptr<EwkPage>(ewkPage));

I don't think this is correct use of std::unique_ptr. You're just passing a pointer of EwkPage. You should pass an ownership of ewkPage to m_pageMap by using WTF::move().
According to your code, it looks you want to maintain EwkPage instances via m_pageMap. If so, m_pageMap should have the ownership of the EwkPage.

For example, you may re-write this logic as below,

auto ewkPage = std::make_unique<EwkPage>(page);
for (auto& it : self->m_clients) {
    if (it->page_add)
        it->page_add(ewkPage.get(), it->data);
}

self->m_pageMap.add(page, WTF::move(ewkPage));

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:96
> +            it->page_del(self->m_pageMap.get(page), it->data);

page_del is called here.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.h:64
> +EAPI JSGlobalContextRef ewk_page_js_global_context_get(Ewk_Page *page);

Missing const keyword for _get().

[Ryuan Choi]

Created attachment 248834 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> Comment on attachment 248827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248827&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:81
> > +{
> 
> Do we need to delete the registered client using ewk_page_client_del() ?
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:81
> > +    self->m_pageMap.add(page, std::unique_ptr<EwkPage>(ewkPage));
> 
> I don't think this is correct use of std::unique_ptr. You're just passing a
> pointer of EwkPage. You should pass an ownership of ewkPage to m_pageMap by
> using WTF::move().
> According to your code, it looks you want to maintain EwkPage instances via
> m_pageMap. If so, m_pageMap should have the ownership of the EwkPage.
> 
> For example, you may re-write this logic as below,
> 
OK, I did.

Thanks.

> auto ewkPage = std::make_unique<EwkPage>(page);
> for (auto& it : self->m_clients) {
>     if (it->page_add)
>         it->page_add(ewkPage.get(), it->data);
> }
> 
> self->m_pageMap.add(page, WTF::move(ewkPage));
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:96
> > +            it->page_del(self->m_pageMap.get(page), it->data);
> 
> page_del is called here.
> 
page_del is just to notify page will be deleted.
ewk_page_client_del is to unregister the client if application wants.
It's not highly required, but I think that it depends on user scenario.

Anyway, I think that register/unregister looks better after your comment.
I updated.

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.h:64
> > +EAPI JSGlobalContextRef ewk_page_js_global_context_get(Ewk_Page *page);
> 
> Missing const keyword for _get().

Correct, I added const here and applied other places in this patch I can add const modifier.

[Gyuyoung Kim]

Comment on attachment 248827 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:81
>>> +{
>> 
>> Do we need to delete the registered client using ewk_page_client_del() ?
> 
> OK, I did.
> 
> Thanks.

This function is still empty on latest patch.

[Ryuan Choi]

Created attachment 248842 [details]
Patch

[Ryuan Choi]

(In reply to comment #10)
> Comment on attachment 248827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248827&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:81
> >>> +{
> >> 
> >> Do we need to delete the registered client using ewk_page_client_del() ?
> > 
> > OK, I did.
> > 
> > Thanks.
> 
> This function is still empty on latest patch.

Oh, I misunderstood previous comment.

I removed pageDelete here because it is not required in this use case.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #10)
> > Comment on attachment 248827 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=248827&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:81
> > >>> +{
> > >> 
> > >> Do we need to delete the registered client using ewk_page_client_del() ?
> > > 
> > > OK, I did.
> > > 
> > > Thanks.
> > 
> > This function is still empty on latest patch.
> 
> Oh, I misunderstood previous comment.
> 
> I removed pageDelete here because it is not required in this use case.

However this patch introduces ewk_page_client_unregister() as well. It would be good if you test this function in test case.

[Gyuyoung Kim]

Comment on attachment 248842 [details]
Patch

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

r=me except for ewk_page_client_unregister() testing and a small nit.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.h:67
> + * Register a page client which contains several callbacks to the page

Missing period at the end of line.

[Ryuan Choi]

Created attachment 248844 [details]
Patch

[Ryuan Choi]

(In reply to comment #14)
> Comment on attachment 248842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248842&action=review
> 
> r=me except for ewk_page_client_unregister() testing and a small nit.
> 

In fact, we don't have proper test suite to test them.

Anyway, I added to call it.

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.h:67
> > + * Register a page client which contains several callbacks to the page
> 
> Missing period at the end of line.

Done.

[WebKit Commit Bot]

Comment on attachment 248844 [details]
Patch

Clearing flags on attachment: 248844

Committed r181681: <http://trac.webkit.org/changeset/181681>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.