WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95672
[EFL][WK2] Add javascript popup api.
https://bugs.webkit.org/show_bug.cgi?id=95672
Summary
[EFL][WK2] Add javascript popup api.
Byungwoo Lee
Reported
2012-09-03 00:56:09 PDT
There is no javascript popup api to create popup for alert(), confirm(), prompt().
Attachments
Patch
(19.17 KB, patch)
2012-09-03 09:16 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(13.89 KB, patch)
2012-09-09 00:50 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(13.89 KB, patch)
2012-09-09 01:07 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(14.24 KB, patch)
2012-09-11 17:26 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(16.26 KB, patch)
2012-09-12 02:26 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(19.38 KB, patch)
2012-09-14 08:40 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(17.63 KB, patch)
2012-09-17 08:35 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Patch
(18.00 KB, patch)
2012-09-17 19:50 PDT
,
Byungwoo Lee
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Byungwoo Lee
Comment 1
2012-09-03 09:16:52 PDT
Created
attachment 161938
[details]
Patch
Gyuyoung Kim
Comment 2
2012-09-03 18:52:23 PDT
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 3
2012-09-03 23:09:00 PDT
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 4
2012-09-04 06:26:23 PDT
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 5
2012-09-04 06:41:52 PDT
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 6
2012-09-04 06:59:58 PDT
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 7
2012-09-04 07:23:00 PDT
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 8
2012-09-04 08:42:07 PDT
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
Comment 9
2012-09-04 09:56:19 PDT
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
Comment 10
2012-09-06 19:01:42 PDT
@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
Comment 11
2012-09-09 00:50:00 PDT
Created
attachment 162984
[details]
Patch
Chris Dumez
Comment 12
2012-09-09 00:56:03 PDT
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 13
2012-09-09 01:02:47 PDT
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
Comment 14
2012-09-09 01:07:57 PDT
Created
attachment 162985
[details]
Patch
Gyuyoung Kim
Comment 15
2012-09-09 02:25:11 PDT
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 16
2012-09-09 02:56:09 PDT
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 17
2012-09-09 03:35:31 PDT
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
Comment 18
2012-09-09 03:57:55 PDT
(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
Comment 19
2012-09-09 03:59:25 PDT
(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 20
2012-09-09 04:02:17 PDT
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 21
2012-09-09 04:14:42 PDT
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
Comment 22
2012-09-09 04:19:52 PDT
(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
Comment 23
2012-09-09 04:24:06 PDT
(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
Comment 24
2012-09-09 04:47:25 PDT
(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
Comment 25
2012-09-09 06:43:30 PDT
(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 26
2012-09-10 01:26:16 PDT
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
Comment 27
2012-09-11 17:26:42 PDT
Created
attachment 163486
[details]
Patch
Ryuan Choi
Comment 28
2012-09-11 18:36:47 PDT
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 29
2012-09-11 18:52:38 PDT
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
Comment 30
2012-09-12 02:26:35 PDT
Created
attachment 163555
[details]
Patch
Gyuyoung Kim
Comment 31
2012-09-13 05:11:15 PDT
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 32
2012-09-13 05:31:37 PDT
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 33
2012-09-13 16:48:47 PDT
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
Comment 34
2012-09-13 18:12:07 PDT
(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
Comment 35
2012-09-13 18:21:05 PDT
(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
Comment 36
2012-09-14 08:40:45 PDT
Created
attachment 164163
[details]
Patch
Ryuan Choi
Comment 37
2012-09-16 16:35:37 PDT
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 38
2012-09-17 02:02:08 PDT
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
Comment 39
2012-09-17 06:27:19 PDT
(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
Comment 40
2012-09-17 08:35:42 PDT
Created
attachment 164400
[details]
Patch
Gyuyoung Kim
Comment 41
2012-09-17 18:25:58 PDT
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
Comment 42
2012-09-17 19:35:22 PDT
(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
Comment 43
2012-09-17 19:50:10 PDT
Created
attachment 164481
[details]
Patch
WebKit Review Bot
Comment 44
2012-09-17 21:49:46 PDT
Comment on
attachment 164481
[details]
Patch Clearing flags on attachment: 164481 Committed
r128849
: <
http://trac.webkit.org/changeset/128849
>
WebKit Review Bot
Comment 45
2012-09-17 21:49:52 PDT
All reviewed patches have been landed. Closing bug.
Raphael Kubo da Costa (:rakuco)
Comment 46
2012-09-18 06:15:47 PDT
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
Comment 47
2012-09-18 06:18:37 PDT
Re-opened since this is blocked by 97007
Byungwoo Lee
Comment 48
2012-09-18 06:53:59 PDT
(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
Comment 49
2012-09-18 18:19:07 PDT
(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
Comment 50
2012-09-18 21:21:57 PDT
(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 51
2012-09-18 22:47:40 PDT
Comment on
attachment 164481
[details]
Patch I tested it again and works fine :)
WebKit Review Bot
Comment 52
2012-09-18 22:57:18 PDT
Comment on
attachment 164481
[details]
Patch Clearing flags on attachment: 164481 Committed
r128967
: <
http://trac.webkit.org/changeset/128967
>
WebKit Review Bot
Comment 53
2012-09-18 22:57:25 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug