[EFL][WK2] Add javascript popup api.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

There is no javascript popup api to create popup for alert(), confirm(), prompt().

[Byungwoo Lee]

Created attachment 161938 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161938 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Added ewk api for javascript alert(), confirm() and prompt().

s/api/API/g

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1594
> +void ewk_view_javascript_prompt_finished(Evas_Object*ewkView, const char* result)

Missing a space in Evas_Object*ewkView.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1599
> +    priv->javascriptPromptResult = adoptPtr(new CString(result ? result : ""));

Isn't this better ?

result ? adoptPtr(new CString(result)) : 0;

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:243
> +            char *message; /**< message for alert()*/

Missing a space in "alert()*/"

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:246
> +            char *message; /**< message for confirm()*/

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:249
> +            char *message; /**< message for prompt()*/

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:250
> +            char *default_value; /**< default value for prompt() input box*/

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
> +PassOwnPtr<CString> ewk_view_run_javascript_prompt(Evas_Object* ewkView, char* message, char* default_value);

s/defalut_value/defaultValue/g

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:77
> +    return WKStringCreateWithUTF8CString(result ? result->data() : "");

I think this one is better. 

result ? adoptWK(WKStringCreateWithUTF8CString(result->data())) : 0;

[Chris Dumez]

Comment on attachment 161938 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:106
> +    OwnPtr<CString> javascriptPromptResult;

Why does does this need to be a pointer? What prevents you from using simply a CString?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:122
> +        , javascriptPromptResult(nullptr)

Not needed (either with CString or OwnPtr<CString> types).

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:239
> + */

You should document that the strings are stringshared.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
>> +PassOwnPtr<CString> ewk_view_run_javascript_prompt(Evas_Object* ewkView, char* message, char* default_value);
> 
> s/defalut_value/defaultValue/g

Why does this function return a PassOwnPtr<CString> instead of a simple CString?

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:51
> +    OwnArrayPtr<char> alertTextBuffer = adoptArrayPtr(new char[length]);

In EFL port, we usually expose stringshared strings to the client. I believe you should use WKEinaSharedString instead of OwnArrayPtr:
WKEinaSharedString text(alertText);

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:276
> +    destination->alert.message = strdup(source->alert.message);

Should use eina_stringshare_ref() instead of strdup().

[Byungwoo Lee]

Comment on attachment 161938 [details]
Patch

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

Thank you for your comments.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:106
>> +    OwnPtr<CString> javascriptPromptResult;
> 
> Why does does this need to be a pointer? What prevents you from using simply a CString?

I used pointer to release string data after returning ewk_view_run_javascript_prompt() function.
Otherwise, Ewk_View_Private_Data should keep the prompt result data.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:239
>> + */
> 
> You should document that the strings are stringshared.

I think this structure have no need to use the stringshared.
There is no use case to return some string with this structure from application side to webkit side.
This structure type just passes the message or default value string through the callback function.
So user can just read strings and use it, and have no need to delete or change this.

>>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
>>> +PassOwnPtr<CString> ewk_view_run_javascript_prompt(Evas_Object* ewkView, char* message, char* default_value);
>> 
>> s/defalut_value/defaultValue/g
> 
> Why does this function return a PassOwnPtr<CString> instead of a simple CString?

As replied above, to release prompt result string after the popup is finished.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:77
>> +    return WKStringCreateWithUTF8CString(result ? result->data() : "");
> 
> I think this one is better. 
> 
> result ? adoptWK(WKStringCreateWithUTF8CString(result->data())) : 0;

Ok I'll apply it. I found a bug about returning null with applying this comment and added it to the test case also.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:276
>> +    destination->alert.message = strdup(source->alert.message);
> 
> Should use eina_stringshare_ref() instead of strdup().

Should this structure be changed to using eina stringshare because of this test case code?
Actually test case just use this structure to keep the strings for checking,
and can use some other way or define other structure for keeping the data.

If strdup in test case is a problem, how about changing the test case code?

[Chris Dumez]

Comment on attachment 161938 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:239
>>> + */
>> 
>> You should document that the strings are stringshared.
> 
> I think this structure have no need to use the stringshared.
> There is no use case to return some string with this structure from application side to webkit side.
> This structure type just passes the message or default value string through the callback function.
> So user can just read strings and use it, and have no need to delete or change this.

The point is that the client application may want to keep the strings and they could use eina_stringshare_ref() for this instead of strdup(). Exposing non-stringshared strings is unexpected for EFL port and is error prone.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:276
>>> +    destination->alert.message = strdup(source->alert.message);
>> 
>> Should use eina_stringshare_ref() instead of strdup().
> 
> Should this structure be changed to using eina stringshare because of this test case code?
> Actually test case just use this structure to keep the strings for checking,
> and can use some other way or define other structure for keeping the data.
> 
> If strdup in test case is a problem, how about changing the test case code?

The point is that the application may need to "copy" the strings in the structure as well. And using eina_stringshare_ref() should be more efficient than strdup(). As far as I know, we never expose "raw" strings to the client so it knows it can safely can eina_stringshare_ref() on them.

[Byungwoo Lee]

Comment on attachment 161938 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:276
>>>> +    destination->alert.message = strdup(source->alert.message);
>>> 
>>> Should use eina_stringshare_ref() instead of strdup().
>> 
>> Should this structure be changed to using eina stringshare because of this test case code?
>> Actually test case just use this structure to keep the strings for checking,
>> and can use some other way or define other structure for keeping the data.
>> 
>> If strdup in test case is a problem, how about changing the test case code?
> 
> The point is that the application may need to "copy" the strings in the structure as well. And using eina_stringshare_ref() should be more efficient than strdup(). As far as I know, we never expose "raw" strings to the client so it knows it can safely can eina_stringshare_ref() on them.

Ok~ I understood your point, and found that all the other exposed string is the stringshared.
I'll apply those. Thanks for your guide :)

[Ryuan Choi]

Comment on attachment 161938 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:49
> + * - "javascript,alert", Ewk_JavaScript_Popup_Information*: reports that an alert() function is dispatched in javascript.
> + * - "javascript,confirm", Ewk_JavaScript_Popup_Information*: reports that a confirm() function is dispatched in javascript.
> + * - "javascript,prompt", Ewk_JavaScript_Popup_Informtion*: reports that a prompt() function is dispatched in javascript.

IMO, we'd better to provide methods of smart class in this case. (like webkit1/efl)
Application may want to disable previous callbacks which another module(such as elm_web) added.

[Byungwoo Lee]

Comment on attachment 161938 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:49
>> + * - "javascript,prompt", Ewk_JavaScript_Popup_Informtion*: reports that a prompt() function is dispatched in javascript.
> 
> IMO, we'd better to provide methods of smart class in this case. (like webkit1/efl)
> Application may want to disable previous callbacks which another module(such as elm_web) added.

I can understand about the requirement to disable the previous callback.
I think the issue will be in the module which overrides or inherits the ewk_view
and provides it's own default javascript popup. (like elm_web as you explained)

The way to support those requirement might be a smart class member function,
as current webkit efl implementation.

But overriding smart class or make an evas object which inherits ewk_view for using javascript popup callback
might be too difficult or complicate way for users who just use ewk_view.

Is there any easy way for supporting this?
How about adding api for setting javascript callback?

[Byungwoo Lee]

Or, how about providing smart class member function that intercepts the signals?

@@ -111,6 +111,14 @@ struct _Ewk_View_Smart_Class {
     Eina_Bool (*mouse_move)(Ewk_View_Smart_Data *sd, const Evas_Event_Mouse_Move *ev);
     Eina_Bool (*key_down)(Ewk_View_Smart_Data *sd, const Evas_Event_Key_Down *ev);
     Eina_Bool (*key_up)(Ewk_View_Smart_Data *sd, const Evas_Event_Key_Up *ev);
+
+    // intercept signals
+    //   - If overridden, related signal will not be emitted.
+    struct {
+        void (*javascript_alert)(Ewk_View_Smart_Data *sd, const char *message);
+        Eina_Bool (*javascript_confirm)(Ewk_View_Smart_Data *sd, const char *message);
+        Eina_Bool (*javascript_prompt)(Ewk_View_Smart_Data *sd, const char *message, const char *default_value, const char **value);
+    } intercept_signal;
 };

Then elm_web or some other modules can override this method to customize the javascript popup callbacks, and other ewk_view users can use smart signal simply.

[Byungwoo Lee]

@Ryuan: 
As you explained on offline, using smart member will be better because it is the official way for overriding by EFL platform. And there can be some issue about consistency between wk1 and wk2 API.

I also agree with that, and discussion about the API policy seems out of the boundary of this patch.

I'll change the signal to smart member function.

Thanks :)

[Byungwoo Lee]

Created attachment 162984 [details]
Patch

[Chris Dumez]

Comment on attachment 162984 [details]
Patch

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

LGTM with small nit:

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:119
> +    const char* (*run_javascript_prompt)(Ewk_View_Smart_Data *sd, const char *message, const char *default_value); /**< return string should be stringshared. */

star on wrong side.

[Byungwoo Lee]

Comment on attachment 162984 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:119
>> +    const char* (*run_javascript_prompt)(Ewk_View_Smart_Data *sd, const char *message, const char *default_value); /**< return string should be stringshared. */
> 
> star on wrong side.

Oops. I'll fix it. Thank you :)

[Byungwoo Lee]

Created attachment 162985 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162985 [details]
Patch

I think you need to update EWK_VIEW_SMART_CLASS_VERSION macro from 2UL to 3UL.

[Jinwoo Song]

Comment on attachment 162985 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
> +    eina_stringshare_del(value);

Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:51
> +    ewk_view_run_javascript_alert(toEwkView(clientInfo), alertTextString);

nit: Why don't you simplify the code like this?
ewk_view_run_javascript_alert(toEwkView(clientInfo), WKEinaSharedString(alertText));

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:58
> +    return ewk_view_run_javascript_confirm(toEwkView(clientInfo), messageString);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:66
> +    WKEinaSharedString value = ewk_view_run_javascript_prompt(toEwkView(clientInfo), messageString, defaultValueString);

ditto.

[Chris Dumez]

Comment on attachment 162985 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
>> +    eina_stringshare_del(value);
> 
> Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.

Yes, indeed. Nice catch :)

[Byungwoo Lee]

(In reply to comment #17)
> (From update of attachment 162985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162985&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
> >> +    eina_stringshare_del(value);
> > 
> > Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.
> 
> Yes, indeed. Nice catch :)

I think this should be called.
In the callback function, application created eina stringshare and returns it.
So the ref count of the returned stringshare will be 1.
After assigning the returned stringshare to WKEinaSharedString value,
the ref count will be increased so the ref count will be 2.
Without eina_stringshare_del(value), the returned stringshare cannot be deleted, because, destructor of value only decrease one time.
So I called this.

And I'm planning to create some mechanism like adoptPtr and passRefPtr for WKEinaStringShare because of this.

Is there something that I misunderstand about this?

[Byungwoo Lee]

(In reply to comment #15)
> (From update of attachment 162985 [details])
> I think you need to update EWK_VIEW_SMART_CLASS_VERSION macro from 2UL to 3UL.

Ok~ I'll increase. Thanks.

[Byungwoo Lee]

Comment on attachment 162985 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:51
>> +    ewk_view_run_javascript_alert(toEwkView(clientInfo), alertTextString);
> 
> nit: Why don't you simplify the code like this?
> ewk_view_run_javascript_alert(toEwkView(clientInfo), WKEinaSharedString(alertText));

Ok~ I'll apply it. Thanks.

[Chris Dumez]

Comment on attachment 162985 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
>>>> +    eina_stringshare_del(value);
>>> 
>>> Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.
>> 
>> Yes, indeed. Nice catch :)
> 
> I think this should be called.
> In the callback function, application created eina stringshare and returns it.
> So the ref count of the returned stringshare will be 1.
> After assigning the returned stringshare to WKEinaSharedString value,
> the ref count will be increased so the ref count will be 2.
> Without eina_stringshare_del(value), the returned stringshare cannot be deleted, because, destructor of value only decrease one time.
> So I called this.
> 
> And I'm planning to create some mechanism like adoptPtr and passRefPtr for WKEinaStringShare because of this.
> 
> Is there something that I misunderstand about this?

I understand your point, thanks for the explanation. But then, I would add a mechanism to WKEinaSharedString to adopt the string and use it instead. Using WKEinaSharedString and explicitly altering its ref count using eina_stringshare_del feels wrong.
I suggest you add the WKEinaSharedString string adoption in this patch instead.

[Byungwoo Lee]

(In reply to comment #21)
> (From update of attachment 162985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162985&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
> >>>> +    eina_stringshare_del(value);
> >>> 
> >>> Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.
> >> 
> >> Yes, indeed. Nice catch :)
> > 
> > I think this should be called.
> > In the callback function, application created eina stringshare and returns it.
> > So the ref count of the returned stringshare will be 1.
> > After assigning the returned stringshare to WKEinaSharedString value,
> > the ref count will be increased so the ref count will be 2.
> > Without eina_stringshare_del(value), the returned stringshare cannot be deleted, because, destructor of value only decrease one time.
> > So I called this.
> > 
> > And I'm planning to create some mechanism like adoptPtr and passRefPtr for WKEinaStringShare because of this.
> > 
> > Is there something that I misunderstand about this?
> 
> I understand your point, thanks for the explanation. But then, I would add a mechanism to WKEinaSharedString to adopt the string and use it instead. Using WKEinaSharedString and explicitly altering its ref count using eina_stringshare_del feels wrong.
> I suggest you add the WKEinaSharedString string adoption in this patch instead.

Sure~ I'll make the patch first. Thanks :)

[Chris Dumez]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 162985 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162985&action=review
> > 
> > >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1579
> > >>>> +    eina_stringshare_del(value);
> > >>> 
> > >>> Is this right to call eina_stringshare_del(value) here? It seems to return NULL always.
> > >> 
> > >> Yes, indeed. Nice catch :)
> > > 
> > > I think this should be called.
> > > In the callback function, application created eina stringshare and returns it.
> > > So the ref count of the returned stringshare will be 1.
> > > After assigning the returned stringshare to WKEinaSharedString value,
> > > the ref count will be increased so the ref count will be 2.
> > > Without eina_stringshare_del(value), the returned stringshare cannot be deleted, because, destructor of value only decrease one time.
> > > So I called this.
> > > 
> > > And I'm planning to create some mechanism like adoptPtr and passRefPtr for WKEinaStringShare because of this.
> > > 
> > > Is there something that I misunderstand about this?
> > 
> > I understand your point, thanks for the explanation. But then, I would add a mechanism to WKEinaSharedString to adopt the string and use it instead. Using WKEinaSharedString and explicitly altering its ref count using eina_stringshare_del feels wrong.
> > I suggest you add the WKEinaSharedString string adoption in this patch instead.
> 
> Sure~ I'll make the patch first. Thanks :)

IMO, you could have added it in the same patch since it is very little code (just an addition adopt() static method to WKEinaSharedString class, right?).

[Gyuyoung Kim]

(In reply to comment #23)
 
> IMO, you could have added it in the same patch since it is very little code (just an addition adopt() static method to WKEinaSharedString class, right?).

Although patch is very small, AFAIK, WebKit prefers to file a new bug if there is new functionality or goal.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> 
> > IMO, you could have added it in the same patch since it is very little code (just an addition adopt() static method to WKEinaSharedString class, right?).
> 
> Although patch is very small, AFAIK, WebKit prefers to file a new bug if there is new functionality or goal.

We also don't like to add code that is not being used :)

Anyway, either way is fine by me, you're probably the one who's going to land this.

[Mikhail Pozdnyakov]

Comment on attachment 162985 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1547
> +void ewk_view_run_javascript_alert(Evas_Object* ewkView, WKEinaSharedString message)

should not pass string classes like this, use 'const WKEinaSharedString&' please

[Byungwoo Lee]

Created attachment 163486 [details]
Patch

[Ryuan Choi]

Comment on attachment 163486 [details]
Patch

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

> Source/WebKit2/ChangeLog:22
> +        * UIProcess/API/efl/tests/test_ewk2_view.cpp:

Could you mention that you added tests?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:672
> +    api->run_javascript_alert = 0;
> +    api->run_javascript_confirm = 0;
> +    api->run_javascript_prompt = 0;

IMO, there are not necessary.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1645
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);

priv looks not used.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1656
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, WKEinaSharedString());

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:119
> +    //   - All string should be guaranteed to be stringshared.

strings?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:410
> +    ewkViewClass()->run_javascript_alert = checkConfirmResultTrue;
> +    ewkViewClass()->run_javascript_confirm = checkConfirmMessageAndReturnTrue;

This is not important. but why don't you use title instead of alert?

It looks better to understand test.

[Byungwoo Lee]

Comment on attachment 163486 [details]
Patch

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

>> Source/WebKit2/ChangeLog:22
>> +        * UIProcess/API/efl/tests/test_ewk2_view.cpp:
> 
> Could you mention that you added tests?

Ok~

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:672
>> +    api->run_javascript_prompt = 0;
> 
> IMO, there are not necessary.

Yes, right. Thanks you.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1645
>> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);
> 
> priv looks not used.

Yes, smartData->api should be checked. Thanks :)

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:119
>> +    //   - All string should be guaranteed to be stringshared.
> 
> strings?

Ok~ I'll apply it.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:410
>> +    ewkViewClass()->run_javascript_confirm = checkConfirmMessageAndReturnTrue;
> 
> This is not important. but why don't you use title instead of alert?
> 
> It looks better to understand test.

I just used alert for checking return value, and there was some purpose to verify alert callback together.
But as you said, checking title will be more clear and have no dependency with alert callback.
I'll change this.

[Byungwoo Lee]

Created attachment 163555 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163555 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1650
> +WKEinaSharedString ewk_view_run_javascript_prompt(Evas_Object* ewkView, const WKEinaSharedString& message, const WKEinaSharedString& defaultValue)

Don't we need to know which button user choices in prompt dialog? In qt port case, he is using *ok* argument for it.

http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp#L587

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1656
> +        return WKEinaSharedString();

If user sets default value, should we return an empty string instead of defaultValue ?

It looks Qt port returns default value.

http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp#L593

[Ryuan Choi]

Comment on attachment 163555 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:47
> +static void runJavaScriptAlert(WKPageRef page, WKStringRef alertText, WKFrameRef frame, const void* clientInfo)

I think that you can remove page, frame like (WKPageRef, WKStringRef alertText, WKFrameRef, const void* clientInfo);

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:52
> +static bool runJavaScriptConfirm(WKPageRef page, WKStringRef message, WKFrameRef frame, const void* clientInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:57
> +static WKStringRef runJavaScriptPrompt(WKPageRef page, WKStringRef message, WKStringRef defaultValue, WKFrameRef frame, const void* clientInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:379
> +} javaScriptPopupCallbackData;

good. but Isn't it better to keep alertCallbackData, ...

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:381
> +static void checkAlert(Ewk_View_Smart_Data* smartData, const char* message)

smartData can be ommited.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:390
> +        "<html><body onload=\"alert('This is javascript alert message.');\"></body></html>";
> +    javaScriptPopupCallbackData.alert.expectedMessage = "This is javascript alert message.";

How about using eina_stringshare_printf or something similar to share result string?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:428
> +    javaScriptPopupCallbackData.confirm.result = EINA_TRUE;

s/EINA_TRUE/true/

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:431
> +    waitUntilLoadFinished();
> +    EXPECT_STREQ(ewk_view_title_get(webView()), "true");

We normally use waitUntilTitleChanged("true") for these.

[Byungwoo Lee]

Comment on attachment 163555 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1650
>> +WKEinaSharedString ewk_view_run_javascript_prompt(Evas_Object* ewkView, const WKEinaSharedString& message, const WKEinaSharedString& defaultValue)
> 
> Don't we need to know which button user choices in prompt dialog? In qt port case, he is using *ok* argument for it.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp#L587

I think Qt browser has some additional requirement about this.
If prompt entry is empty, empty string("") is returned when user press 'ok' button.
null is the result of 'cancel', so additional ok status might not be needed.
(I tested IE, Firefox, Chromium..)

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1656
>> +        return WKEinaSharedString();
> 
> If user sets default value, should we return an empty string instead of defaultValue ?
> 
> It looks Qt port returns default value.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp#L593

I think that 'default value' means the default string for the prompt entry.
And if user press ok, then string in the prompt entry will be returned,
and if user press cancel, then null will be returned.
I think that this is the behavior of most browsers.

Returning null might be better because it is the return value by pressing cancel.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:47
>> +static void runJavaScriptAlert(WKPageRef page, WKStringRef alertText, WKFrameRef frame, const void* clientInfo)
> 
> I think that you can remove page, frame like (WKPageRef, WKStringRef alertText, WKFrameRef, const void* clientInfo);

Ok~ I'll apply it.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:379
>> +} javaScriptPopupCallbackData;
> 
> good. but Isn't it better to keep alertCallbackData, ...

Ok~ I'll separate the structures. :)

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:381
>> +static void checkAlert(Ewk_View_Smart_Data* smartData, const char* message)
> 
> smartData can be ommited.

Ok~ I'll remove it.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:390
>> +    javaScriptPopupCallbackData.alert.expectedMessage = "This is javascript alert message.";
> 
> How about using eina_stringshare_printf or something similar to share result string?

Using macro might be easier. I'll change it like below.
#define ALERT_MESSAGE  "This is javascript alert message."
const char* alertHTML =
    "<html><body onload=\"alert('"ALERT_MESSAGE"');\"></body></html>";
javaScriptPopupCallbackData.alert.expectedMessage = ALERT_MESSAGE;

Thanks~

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:428
>> +    javaScriptPopupCallbackData.confirm.result = EINA_TRUE;
> 
> s/EINA_TRUE/true/

Ok, I'll use it.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:431
>> +    EXPECT_STREQ(ewk_view_title_get(webView()), "true");
> 
> We normally use waitUntilTitleChanged("true") for these.

waitUntilTitleChanged() just make timeout if title is not the string.
I think that checking with this macro will be more clear and easy to check,
because EXPECT_STREQ prints expected and actual result if there is a bug.

[Ryuan Choi]

(In reply to comment #33)
> (From update of attachment 163555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163555&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:431
> >> +    EXPECT_STREQ(ewk_view_title_get(webView()), "true");
> > 
> > We normally use waitUntilTitleChanged("true") for these.
> 
> waitUntilTitleChanged() just make timeout if title is not the string.
> I think that checking with this macro will be more clear and easy to check,
> because EXPECT_STREQ prints expected and actual result if there is a bug.

I know your concern.
But after discussed about it in bug 94670, I changed mind because it make logically more reasonable.

I think that we should find a way not to make timeout.

[Byungwoo Lee]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 163555 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163555&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:431
> > >> +    EXPECT_STREQ(ewk_view_title_get(webView()), "true");
> > > 
> > > We normally use waitUntilTitleChanged("true") for these.
> > 
> > waitUntilTitleChanged() just make timeout if title is not the string.
> > I think that checking with this macro will be more clear and easy to check,
> > because EXPECT_STREQ prints expected and actual result if there is a bug.
> 
> I know your concern.
> But after discussed about it in bug 94670, I changed mind because it make logically more reasonable.
> 
> I think that we should find a way not to make timeout.

Oh~ Ok. I got the point. Thanks for the comment :)

[Byungwoo Lee]

Created attachment 164163 [details]
Patch

[Ryuan Choi]

Comment on attachment 164163 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:450
> +    const char* confirmHTML = "<html><head><title>DefaultTitle</title></head><body onload=\"document.title = confirm('" JAVASCRIPT_STRING_CONFIRM_MESSAGE "');\"></body></html>";

I don't have objection if you want, but I think that we can simplify this
"<!doctype html><body onload=\"...</body>";

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:537
> +    const char* promptHTML = "<html><head><title>DefaultTitle</title></head><body onload=\"document.title = prompt('" JAVASCRIPT_STRING_PROMPT_MESSAGE "', '');\"></body></html>";
> +    promptCallbackData.expectedMessage = JAVASCRIPT_STRING_PROMPT_MESSAGE;
> +    promptCallbackData.expectedDefaultValue = JAVASCRIPT_STRING_EMPTY;
> +    promptCallbackData.result = JAVASCRIPT_STRING_RESULT_STRING;
> +    promptCallbackData.called = false;
> +    ewk_view_html_string_load(webView(), promptHTML, 0, 0);
> +    waitUntilTitleChangedTo(JAVASCRIPT_STRING_RESULT_STRING);
> +    EXPECT_STREQ(ewk_view_title_get(webView()), JAVASCRIPT_STRING_RESULT_STRING);
> +    EXPECT_EQ(promptCallbackData.called, true);
> +
> +    promptHTML = "<html><head><title>DefaultTitle</title></head><body onload=\"document.title = prompt('" JAVASCRIPT_STRING_PROMPT_MESSAGE "');\"></body></html>";
> +    promptCallbackData.expectedMessage = JAVASCRIPT_STRING_PROMPT_MESSAGE;
> +    promptCallbackData.expectedDefaultValue = JAVASCRIPT_STRING_EMPTY;
> +    promptCallbackData.result = JAVASCRIPT_STRING_RESULT_STRING;
> +    promptCallbackData.called = false;
> +    ewk_view_html_string_load(webView(), promptHTML, 0, 0);
> +    waitUntilTitleChangedTo(JAVASCRIPT_STRING_RESULT_STRING);
> +    EXPECT_STREQ(ewk_view_title_get(webView()), JAVASCRIPT_STRING_RESULT_STRING);
> +    EXPECT_EQ(promptCallbackData.called, true);

same tests?

[Byungwoo Lee]

Comment on attachment 164163 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:450
>> +    const char* confirmHTML = "<html><head><title>DefaultTitle</title></head><body onload=\"document.title = confirm('" JAVASCRIPT_STRING_CONFIRM_MESSAGE "');\"></body></html>";
> 
> I don't have objection if you want, but I think that we can simplify this
> "<!doctype html><body onload=\"...</body>";

Ok~ I'll apply it except #559 (that needs default title string)

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:537
>> +    EXPECT_EQ(promptCallbackData.called, true);
> 
> same tests?

Not the same test. First one has two parameter (second one is empty string) and Second one has only one parameter.

[Byungwoo Lee]

(In reply to comment #33)
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:390
> >> +    javaScriptPopupCallbackData.alert.expectedMessage = "This is javascript alert message.";
> > 
> > How about using eina_stringshare_printf or something similar to share result string?
> 
> Using macro might be easier. I'll change it like below.
> #define ALERT_MESSAGE  "This is javascript alert message."
> const char* alertHTML =
>     "<html><body onload=\"alert('"ALERT_MESSAGE"');\"></body></html>";
> javaScriptPopupCallbackData.alert.expectedMessage = ALERT_MESSAGE;
> 
> Thanks~
@ryuan
Using macro can make issue according to the bug 96793, so I'll remove macros and add static const char[] for some strings.
And I think that formatting html string using a title string is not an important issue in test case code.
Is it ok?

[Byungwoo Lee]

Created attachment 164400 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164400 [details]
Patch

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

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

Add internal description like this,

/**
 * @internal
 * bla bla.
 */

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

ditto.

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

ditto.

[Byungwoo Lee]

(In reply to comment #41)
> (From update of attachment 164400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164400&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1627
> > +
> 
> Add internal description like this,
> 
> /**
>  * @internal
>  * bla bla.
>  */
Ok~ I'll add it. Thanks :)

[Byungwoo Lee]

Created attachment 164481 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164481 [details]
Patch

Clearing flags on attachment: 164481

Committed r128849: <http://trac.webkit.org/changeset/128849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This has made test_ewk2_view start timing out because EWK2UnitTestBase.ewk_view_run_javascript_prompt seems to enter an infinite looop. I will roll this out for now to keep the bot green.

[WebKit Review Bot]

Re-opened since this is blocked by 97007

[Byungwoo Lee]

(In reply to comment #46)
> This has made test_ewk2_view start timing out because EWK2UnitTestBase.ewk_view_run_javascript_prompt seems to enter an infinite looop. I will roll this out for now to keep the bot green.
The issue is related with Bug 96793. I'll add dependency with it.
Maybe unit test timeout issue will be fixed after the bug.

[Gyuyoung Kim]

(In reply to comment #46)
> This has made test_ewk2_view start timing out because EWK2UnitTestBase.ewk_view_run_javascript_prompt seems to enter an infinite looop. I will roll this out for now to keep the bot green.

Thank you for your roll out. r128943 fixed the infinite loop problem. So, this patch will be considered to land again. Anyway, we need to get Spank Spank Spank !!.

[Byungwoo Lee]

(In reply to comment #49)
> (In reply to comment #46)
> > This has made test_ewk2_view start timing out because EWK2UnitTestBase.ewk_view_run_javascript_prompt seems to enter an infinite looop. I will roll this out for now to keep the bot green.
> 
> Thank you for your roll out. r128943 fixed the infinite loop problem. So, this patch will be considered to land again. Anyway, we need to get Spank Spank Spank !!.

Sorry for my carelessness.

I already tested about the problem so I made a patch for that.
But I made a mistake to activate the commit queue without seting the dependency.
Now I'm testing this patch again with the merged source, and will request commit queue again.

Thanks :)

[Byungwoo Lee]

Comment on attachment 164481 [details]
Patch

I tested it again and works fine :)

[WebKit Review Bot]

Comment on attachment 164481 [details]
Patch

Clearing flags on attachment: 164481

Committed r128967: <http://trac.webkit.org/changeset/128967>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.