Bug 84589

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

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.

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

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

(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.

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.

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

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.

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 ","

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.

Created attachment 140885 [details] Patch Take feedback into consideration.

(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.

Comment on attachment 140885 [details] Patch rs=me.

Comment on attachment 140885 [details] Patch Not yet, please. My doubts persist.

(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).

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