Bug 101904 – [EFL][WK2] Add API to execute js script

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jongseok Yang]

Add the API to execute the specific javascript

[Jongseok Yang]

Created attachment 173571 [details]
Patch

[Jongseok Yang]

Created attachment 173587 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 173587 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:926
> +struct _Ewk_View_Script_Execute_Callback_Context {

I think ewk_view_private.h or EwkViewImpl.h is more better place for this struct.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:939
> +    if (!ewkView || error)

What is error role here ? It looks QT and GTK ports don't take care of error.

Beside don't you need to delete callbackContext here ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
> +    JSGlobalContextRef jsGlobalContext = impl->ewkContext()->jsGlobalContext();

Don't you need to free jsGlobalContext using JSGlobalContextRelease() after finishing to use it ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:945
> +    if (!callbackContext->scriptExecuteCallback)

I think this line should be placed before above two lines.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:975
> +

Unneeded line.

[Chris Dumez]

Comment on attachment 173587 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:926
>> +struct _Ewk_View_Script_Execute_Callback_Context {
> 
> I think ewk_view_private.h or EwkViewImpl.h is more better place for this struct.

Why? if this struct is used only in ewk_view.cpp, then I think this is the right place for it. However, we don't use _ prefix for such structures anymore.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:936
> +    Ewk_View_Script_Execute_Callback_Context* callbackContext = static_cast<Ewk_View_Script_Execute_Callback_Context*>(context);

It is a good idea to use OwnPtr here and adopt to avoid having to call delete manually.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
>> +    JSGlobalContextRef jsGlobalContext = impl->ewkContext()->jsGlobalContext();
> 
> Don't you need to free jsGlobalContext using JSGlobalContextRelease() after finishing to use it ?

Looks like the global context object is owned by Ewk_Context so I don't think we should free it here.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:950
> +        JSRetainPtr<JSStringRef> jsStringValue(JSValueToStringCopy(jsGlobalContext, value, 0));

Looks like you may be leaking here. Don't you need to adopt the value returned by JSValueToStringCopy() ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:822
> + * The result value for the executeion can be got from the asynchronous callback.

"execution"
"got" -> "retrieved"

[Gyuyoung Kim]

Comment on attachment 173587 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:926
>>> +struct _Ewk_View_Script_Execute_Callback_Context {
>> 
>> I think ewk_view_private.h or EwkViewImpl.h is more better place for this struct.
> 
> Why? if this struct is used only in ewk_view.cpp, then I think this is the right place for it. However, we don't use _ prefix for such structures anymore.

If so, what do you think ewk_view_private.h role ? Do you think it isn't for internal data type or struct ?

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
>>> +    JSGlobalContextRef jsGlobalContext = impl->ewkContext()->jsGlobalContext();
>> 
>> Don't you need to free jsGlobalContext using JSGlobalContextRelease() after finishing to use it ?
> 
> Looks like the global context object is owned by Ewk_Context so I don't think we should free it here.

Ok

[Chris Dumez]

Comment on attachment 173587 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:926
>>>> +struct _Ewk_View_Script_Execute_Callback_Context {
>>> 
>>> I think ewk_view_private.h or EwkViewImpl.h is more better place for this struct.
>> 
>> Why? if this struct is used only in ewk_view.cpp, then I think this is the right place for it. However, we don't use _ prefix for such structures anymore.
> 
> If so, what do you think ewk_view_private.h role ? Do you think it isn't for internal data type or struct ?

ewk_view_private.h should be used for structures / macros that need to be shared between several files.
In this case, Ewk_View_Script_Execute_Callback_Context is only used internally by ewk_view.cpp. There is no point in sharing it with other implementation files since none of them will use it. It would only make compile time longer.

[Mikhail Pozdnyakov]

Comment on attachment 173587 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:931
> +typedef struct _Ewk_View_Script_Execute_Callback_Context Ewk_View_Script_Execute_Callback_Context;

could be just 'struct EwkViewScriptExecuteCallbackContext'

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:935
> +    EINA_SAFETY_ON_NULL_RETURN(context);

maybe assert is better since 'runJavaScriptCallback' is always invoked internally? How context can be '0'?

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:936
>> +    Ewk_View_Script_Execute_Callback_Context* callbackContext = static_cast<Ewk_View_Script_Execute_Callback_Context*>(context);
> 
> It is a good idea to use OwnPtr here and adopt to avoid having to call delete manually.

indeed.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:961
> +Eina_Bool ewk_view_script_execute(Evas_Object* ewkView, const char* script, Ewk_View_Script_Execute_Cb callback, void* userData)

isn't it worth mentioning where the script will be executed?  like "runJavaScriptInMainFrame"

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:967
> +    context->scriptExecuteCallback = callback;

maybe this could be done inside structure constructor?

[Gyuyoung Kim]

Comment on attachment 173587 [details]
Patch

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

>>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:926
>>>>> +struct _Ewk_View_Script_Execute_Callback_Context {
>>>> 
>>>> I think ewk_view_private.h or EwkViewImpl.h is more better place for this struct.
>>> 
>>> Why? if this struct is used only in ewk_view.cpp, then I think this is the right place for it. However, we don't use _ prefix for such structures anymore.
>> 
>> If so, what do you think ewk_view_private.h role ? Do you think it isn't for internal data type or struct ?
> 
> ewk_view_private.h should be used for structures / macros that need to be shared between several files.
> In this case, Ewk_View_Script_Execute_Callback_Context is only used internally by ewk_view.cpp. There is no point in sharing it with other implementation files since none of them will use it. It would only make compile time longer.

I agree. Thanks.

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:943
>>>> +    JSGlobalContextRef jsGlobalContext = impl->ewkContext()->jsGlobalContext();
>>> 
>>> Don't you need to free jsGlobalContext using JSGlobalContextRelease() after finishing to use it ?
>> 
>> Looks like the global context object is owned by Ewk_Context so I don't think we should free it here.
> 
> Ok

BTW, could you explain why jsGlobalContext should be maintained by ewkContext ? Because, it seems jsGlobalContext() is just used by this callback now.
If there is no reason, is below code enough ?

JSGlobalContextRef jsGlobalContext = JSGlobalContextCreate(0);
...
...
JSGlobalContextRelease(jsGlobalContext);

[Gyuyoung Kim]

Comment on attachment 173587 [details]
Patch

Cleared review? from attachment 173587 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

[Ryuan Choi]

Created attachment 220411 [details]
Patch

[Ryuan Choi]

(In reply to comment #10)
> Created an attachment (id=220411) [details]
> Patch

I tried to follow almost comments except below comment.

(In reply to comment #7)
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:961
> > +Eina_Bool ewk_view_script_execute(Evas_Object* ewkView, const char* script, Ewk_View_Script_Execute_Cb callback, void* userData)
> 
> isn't it worth mentioning where the script will be executed?  like "runJavaScriptInMainFrame"

I think that we can assume MainFrame from the *ewk_view* becaus ewk_view implicitly represents Page or MainFrame.

And we don't have plan to support this API for each frame.

[Ryuan Choi]

Created attachment 220578 [details]
improve testcase

[Gyuyoung Kim]

Comment on attachment 220578 [details]
improve testcase

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:76
> +    , m_jsGlobalContext(0)

Please use *nullptr* instead of 0. As you know, we are talking about using *nullptr* instead of 0 or NULL in webkit-dev.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:648
> +    if (error)

Isn't it better to add a log here ?

[Ryuan Choi]

Created attachment 220675 [details]
Patch

[Ryuan Choi]

Comment on attachment 220578 [details]
improve testcase

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:76
>> +    , m_jsGlobalContext(0)
> 
> Please use *nullptr* instead of 0. As you know, we are talking about using *nullptr* instead of 0 or NULL in webkit-dev.

Done

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:648
>> +    if (error)
> 
> Isn't it better to add a log here ?

I realized that this callback does not need error.
I removed.

[Ryuan Choi]

Comment on attachment 220675 [details]
Patch

Clearing flags on attachment: 220675

Committed r161527: <http://trac.webkit.org/changeset/161527>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.