Bug 84589 - [EFL][DRT] Implementation of showModalDialog needed
Summary: [EFL][DRT] Implementation of showModalDialog needed
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 85962
  Show dependency treegraph
 
Reported: 2012-04-23 06:12 PDT by Dominik Röttsches (drott)
Modified: 2012-10-30 07:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.88 KB, patch)
2012-04-25 22:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.35 KB, patch)
2012-05-09 01:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-04-23 06:12:08 PDT
Several tests rely on showModalDialog - need an implementation to unskip. See also bug 53676.
Comment 1 Chris Dumez 2012-04-25 05:16:44 PDT
Dominik, I'll work on this one if you don't mind.
Comment 2 Chris Dumez 2012-04-25 22:52:55 PDT
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.
Comment 3 Chris Dumez 2012-04-25 22:54:28 PDT
I should mention that the implementation is based on the one in Qt port.
Comment 4 Chris Dumez 2012-05-04 06:44:48 PDT
rakuco or gyuyoung: Could I please get informal review on this one?
Comment 5 Thiago Marcos P. Santos 2012-05-04 06:58:16 PDT
(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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-05-04 08:12:03 PDT
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.
Comment 7 Chris Dumez 2012-05-08 08:35:07 PDT
Any update on this? We need it to make the tree green.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-05-08 11:28:11 PDT
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 9 Eric Seidel (no email) 2012-05-08 15:26:10 PDT
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 10 Gyuyoung Kim 2012-05-09 00:18:08 PDT
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.
Comment 11 Chris Dumez 2012-05-09 01:48:58 PDT
Created attachment 140885 [details]
Patch

Take feedback into consideration.
Comment 12 Chris Dumez 2012-05-09 01:50:36 PDT
(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 13 Eric Seidel (no email) 2012-08-22 15:25:59 PDT
Comment on attachment 140885 [details]
Patch

rs=me.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-08-22 15:30:03 PDT
Comment on attachment 140885 [details]
Patch

Not yet, please. My doubts persist.
Comment 15 Chris Dumez 2012-08-22 15:39:28 PDT
(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).
Comment 16 Chris Dumez 2012-10-30 07:08:53 PDT
Will probably not fix it for WK1 EFL since focus has moved to WK2 EFL.