Add the API to execute the specific javascript
Created attachment 173571 [details] Patch
Created attachment 173587 [details] Patch
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.
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"
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
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.
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?
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);
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.
Created attachment 220411 [details] Patch
(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.
Created attachment 220578 [details] improve testcase
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 ?
Created attachment 220675 [details] Patch
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.
Comment on attachment 220675 [details] Patch Clearing flags on attachment: 220675 Committed r161527: <http://trac.webkit.org/changeset/161527>
All reviewed patches have been landed. Closing bug.