Bug 84589 – [EFL][DRT] Implementation of showModalDialog needed

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

[Dominik Röttsches (drott)]

Several tests rely on showModalDialog - need an implementation to unskip. See also bug 53676.

[Chris Dumez]

Dominik, I'll work on this one if you don't mind.

[Chris Dumez]

Created attachment 138936 [details]
Patch

This patch fixes the following test cases which are not skipped:
 - http/tests/security/cross-frame-access-call.html
 - http/tests/security/cross-frame-access-get.html
 - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-invalidation-by-header.html
 - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-invalidation-by-method.html
 - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-timeout.html
 - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache.html

It allows to remove several test cases from the skipped list as well.
Some more tests could be unskipped if we implement support for LayoutTestController.abortModal() but I will handle this in a separate patch.

[Chris Dumez]

I should mention that the implementation is based on the one in Qt port.

[Chris Dumez]

rakuco or gyuyoung: Could I please get informal review on this one?

[Thiago Marcos P. Santos]

(In reply to comment #2)
> Created an attachment (id=138936) [details]
> Patch
> 
> This patch fixes the following test cases which are not skipped:
>  - http/tests/security/cross-frame-access-call.html
>  - http/tests/security/cross-frame-access-get.html

Yes.

>  - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-invalidation-by-header.html
>  - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-invalidation-by-method.html
>  - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-timeout.html
>  - http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache.html
> 

I don't think so. It's likely to be bug 85613.

> It allows to remove several test cases from the skipped list as well.
> Some more tests could be unskipped if we implement support for LayoutTestController.abortModal() but I will handle this in a separate patch.

[Raphael Kubo da Costa (:rakuco)]

I've CC'ed Rafael Antognolli in case he remembers anything about the work done on modal stuff a few years ago (was it Lucas de Marchi who did it?).

At the time there were some problems with nested event loops that I don't remember very well and might have already been solved.

I, for one, am not sure if ecore_thread_main_loop is the right solution here (I've never used it before) -- if it is, remember to bump the minimum ecore version in FindEFL.cmake to 1.1.x.

[Chris Dumez]

Any update on this? We need it to make the tree green.

[Raphael Kubo da Costa (:rakuco)]

From my side, I'm still unsure about using ecore_thread_main_loop_{begin,end} here. There isn't much documentation for it besides <http://docs.enlightenment.org/auto/elementary/efl_thread_1.html>, which doesn't really seem to be our case here.

[Eric Seidel (no email)]

Comment on attachment 138936 [details]
Patch

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

Difficult for me to know if this is the right way to do modal dialogs in EFL.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:86
> +    : m_view(view), m_isRunningModal(false)

Normally this goes on the next line, starting with a ","

[Gyuyoung Kim]

Comment on attachment 138936 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_private.h:134
> +void ewk_view_modal_get(Evas_Object* ewkView, bool* modal);

Add const keyword to _get function.

[Chris Dumez]

Created attachment 140885 [details]
Patch

Take feedback into consideration.

[Chris Dumez]

(In reply to comment #10)
> (From update of attachment 138936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138936&action=review
> 
> > Source/WebKit/efl/ewk/ewk_private.h:134
> > +void ewk_view_modal_get(Evas_Object* ewkView, bool* modal);
> 
> Add const keyword to _get function.

I don't think I can because I'm calling evas_object_smart_callback_call() on the view in this method (or am I supposed to const_cast?). If you look at methods around in ewk_private.h, they don't use const either.

[Eric Seidel (no email)]

Comment on attachment 140885 [details]
Patch

rs=me.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140885 [details]
Patch

Not yet, please. My doubts persist.

[Chris Dumez]

(In reply to comment #14)
> (From update of attachment 140885 [details])
> Not yet, please. My doubts persist.

This patch is really old. Completely forgot about it. I'll take a look at it again tomorrow (getting late here).

[Chris Dumez]

Will probably not fix it for WK1 EFL since focus has moved to WK2 EFL.