Bug 99021 – [EFL][WK2] Add support for JavaScript popup boxes to MiniBrowser

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

[Chris Dumez]

MiniBrowser does not currently support JavaScript alert / confirm / prompt popups.

[Chris Dumez]

Created attachment 168194 [details]
Patch

[Byungwoo Lee]

Comment on attachment 168194 [details]
Patch

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

There is a same bug (bug 96616)

> Tools/MiniBrowser/efl/main.c:430
> +    ecore_main_loop_begin();

Is it ok to use the nested run loop?
As I know, EFL recommends not to use the nested run loop.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 168194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> 
> There is a same bug (bug 96616)

Sorry, I did not notice.

> > Tools/MiniBrowser/efl/main.c:430
> > +    ecore_main_loop_begin();
> 
> Is it ok to use the nested run loop?
> As I know, EFL recommends not to use the nested run loop.

Well, it works. I've tested everything with:
http://www.w3schools.com/js/js_popup.asp

FYI, we already use this for the Web Inspector:
Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp-    // Because it can't complete loading inspector.html before loading testURL.
Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp-    Evas_Object* inspectorView = ewk_view_web_inspector_view_get(mainView());
Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp-    if (inspectorView)
Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:        ecore_main_loop_begin();

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 168194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> 
> There is a same bug (bug 96616)
> 
> > Tools/MiniBrowser/efl/main.c:430
> > +    ecore_main_loop_begin();
> 
> Is it ok to use the nested run loop?
> As I know, EFL recommends not to use the nested run loop.

I believe that this case is special. Those JS dialogs are meant to suspend the JS execution until the dialog is closed and this is exactly what my call to ecore_main_loop_begin() does.

I personally don't know of any other way to achieve this. Waiting for a condition to become true and calling ecore_main_loop_iterate() is not an acceptable solution here.

[Byungwoo Lee]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 168194 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> > 
> > There is a same bug (bug 96616)
> > 
> > > Tools/MiniBrowser/efl/main.c:430
> > > +    ecore_main_loop_begin();
> > 
> > Is it ok to use the nested run loop?
> > As I know, EFL recommends not to use the nested run loop.
> 
> I believe that this case is special. Those JS dialogs are meant to suspend the JS execution until the dialog is closed and this is exactly what my call to ecore_main_loop_begin() does.
> 
> I personally don't know of any other way to achieve this. Waiting for a condition to become true and calling ecore_main_loop_iterate() is not an acceptable solution here.

Yes, ecore_main_loop_iterate() will make busy loop, so it is not acceptable.
So as you told, ecore_main_loop_begin() might be a solution.

But I'm worrying about some side effect with the nested main loop. (especially related with some timing issue related with timer or pipe)

Can you guarantee that there will be no issues?

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 168194 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> > > 
> > > There is a same bug (bug 96616)
> > > 
> > > > Tools/MiniBrowser/efl/main.c:430
> > > > +    ecore_main_loop_begin();
> > > 
> > > Is it ok to use the nested run loop?
> > > As I know, EFL recommends not to use the nested run loop.
> > 
> > I believe that this case is special. Those JS dialogs are meant to suspend the JS execution until the dialog is closed and this is exactly what my call to ecore_main_loop_begin() does.
> > 
> > I personally don't know of any other way to achieve this. Waiting for a condition to become true and calling ecore_main_loop_iterate() is not an acceptable solution here.
> 
> Yes, ecore_main_loop_iterate() will make busy loop, so it is not acceptable.
> So as you told, ecore_main_loop_begin() might be a solution.
> 
> But I'm worrying about some side effect with the nested main loop. (especially related with some timing issue related with timer or pipe)
> 
> Can you guarantee that there will be no issues?

If this is the only way to do it, I don't think we have a choice.
All I can test is that my testing did not show any issue.

And as I said before, this is already used by Web Inspector code in DRT.

[Byungwoo Lee]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > (From update of attachment 168194 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> > > > 
> > > > There is a same bug (bug 96616)
> > > > 
> > > > > Tools/MiniBrowser/efl/main.c:430
> > > > > +    ecore_main_loop_begin();
> > > > 
> > > > Is it ok to use the nested run loop?
> > > > As I know, EFL recommends not to use the nested run loop.
> > > 
> > > I believe that this case is special. Those JS dialogs are meant to suspend the JS execution until the dialog is closed and this is exactly what my call to ecore_main_loop_begin() does.
> > > 
> > > I personally don't know of any other way to achieve this. Waiting for a condition to become true and calling ecore_main_loop_iterate() is not an acceptable solution here.
> > 
> > Yes, ecore_main_loop_iterate() will make busy loop, so it is not acceptable.
> > So as you told, ecore_main_loop_begin() might be a solution.
> > 
> > But I'm worrying about some side effect with the nested main loop. (especially related with some timing issue related with timer or pipe)
> > 
> > Can you guarantee that there will be no issues?
> 
> If this is the only way to do it, I don't think we have a choice.
> All I can test is that my testing did not show any issue.
> 
> And as I said before, this is already used by Web Inspector code in DRT.

I made a bug for one issue about the nested loop and elm_popup. (bug 99067)

Actually, I have been preparing some patches or discussions about javascript popup on efl.

How about waiting to apply the javascript popup to mini browser until it is prepared?

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #2)
> > > > > (From update of attachment 168194 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=168194&action=review
> > > > > 
> > > > > There is a same bug (bug 96616)
> > > > > 
> > > > > > Tools/MiniBrowser/efl/main.c:430
> > > > > > +    ecore_main_loop_begin();
> > > > > 
> > > > > Is it ok to use the nested run loop?
> > > > > As I know, EFL recommends not to use the nested run loop.
> > > > 
> > > > I believe that this case is special. Those JS dialogs are meant to suspend the JS execution until the dialog is closed and this is exactly what my call to ecore_main_loop_begin() does.
> > > > 
> > > > I personally don't know of any other way to achieve this. Waiting for a condition to become true and calling ecore_main_loop_iterate() is not an acceptable solution here.
> > > 
> > > Yes, ecore_main_loop_iterate() will make busy loop, so it is not acceptable.
> > > So as you told, ecore_main_loop_begin() might be a solution.
> > > 
> > > But I'm worrying about some side effect with the nested main loop. (especially related with some timing issue related with timer or pipe)
> > > 
> > > Can you guarantee that there will be no issues?
> > 
> > If this is the only way to do it, I don't think we have a choice.
> > All I can test is that my testing did not show any issue.
> > 
> > And as I said before, this is already used by Web Inspector code in DRT.
> 
> I made a bug for one issue about the nested loop and elm_popup. (bug 99067)
> 
> Actually, I have been preparing some patches or discussions about javascript popup on efl.
> 
> How about waiting to apply the javascript popup to mini browser until it is prepared?

I need this patch landed as soon as possible as we have people waiting on it. Moreover the current patch works. You can always improve later if needed.

Please do not add dependencies to my bugs without discussing it first.

[Kenneth Rohde Christiansen]

Comment on attachment 168194 [details]
Patch

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

> Tools/ChangeLog:3
> +        [EFL][WK2] Add support for Javascript popup boxes to MiniBrowser

JavaScript ?

[WebKit Review Bot]

Comment on attachment 168194 [details]
Patch

Clearing flags on attachment: 168194

Committed r131064: <http://trac.webkit.org/changeset/131064>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Byungwoo Lee]

> I need this patch landed as soon as possible as we have people waiting on it. Moreover the current patch works. You can always improve later if needed.

Actually, I heard and observed some problems(like pipe broken or some strange crash) about using the nested main loop with webkit on efl.

MiniBrowser source can be a reference code, so I thought that to share or discuss about the issue before landing will be better.

But as you told, I have not observed such an issue with this patch.

Anyway, this is already landed :)
(There might be some urgent issue on your side)

> Please do not add dependencies to my bugs without discussing it first.

I didn't consider that adding dependencies to some other one's bug can make him unpleasant. I'm sorry if you feel bad.

[Byungwoo Lee]

*** Bug 96616 has been marked as a duplicate of this bug. ***