Bug 110209 - [EFL][WK2] ewk_popup_menu_close() does not work as intended
Summary: [EFL][WK2] ewk_popup_menu_close() does not work as intended
Status: RESOLVED FIXED
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: 108934 111543
  Show dependency tree / graph
 
Reported: 2013-02-19 04:22 PST by Chris Dumez
Modified: 2013-03-12 20:49 PDT (History)
9 users (show)

See Also:

Attachments
Patch (1.64 KB, patch)
2013-02-19 04:32 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-02-19 04:22:51 PST 
ewk_popup_menu_close() is called by the browser to notify WebKit that the popup menu was closed. However, the current implementation calls a smart function asking the browser to hide the popup menu instead of passing the information along to the WebProcess.
Comment 1 Chris Dumez 2013-02-19 04:32:58 PST 
Created attachment 189055 [details]
Patch
Comment 2 Chris Dumez 2013-03-11 15:23:25 PDT 
Could I please get informal review?
Comment 3 Gyuyoung Kim 2013-03-11 23:53:03 PDT 
Comment on attachment 189055 [details]
Patch

It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ?
Comment 4 Chris Dumez 2013-03-12 00:02:37 PDT 
(In reply to comment #3)
> (From update of attachment 189055 [details])
> It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ?

Yes, we need to provide ewk_popup_menu_close() so that the browser lets us know when the popup menu gets hidden (so that WebCore updates its state). See MiniBrowser implementation in Bug 108934.

Some other ports do not need such API because they run an event loop while displaying the menu. However, this is not appropriate for EFL port as EFL has issues with nested event loops.
Comment 5 Gyuyoung Kim 2013-03-12 00:25:07 PDT 
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 189055 [details] [details])
> > It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ?
> 
> Yes, we need to provide ewk_popup_menu_close() so that the browser lets us know when the popup menu gets hidden (so that WebCore updates its state). See MiniBrowser implementation in Bug 108934.
> 
> Some other ports do not need such API because they run an event loop while displaying the menu. However, this is not appropriate for EFL port as EFL has issues with nested event loops.

I see. I talk with Byungwoo. Yes, we need to provide this API because of EFL nested event loop issue. Tizen WebKit also has same problem. BTW, I considered that we can add a new C API to hide popup menu. But, it looks it isn't good to add new C APIs for only EFL specific issue. LGTM.
Comment 6 Benjamin Poulain 2013-03-12 14:16:43 PDT 
Comment on attachment 189055 [details]
Patch

Ok for WebKit2.
Comment 7 Laszlo Gombos 2013-03-12 20:40:57 PDT 
Comment on attachment 189055 [details]
Patch

r=me.
Comment 8 WebKit Review Bot 2013-03-12 20:49:12 PDT 
Comment on attachment 189055 [details]
Patch

Clearing flags on attachment: 189055

Committed r145676: <http://trac.webkit.org/changeset/145676>
Comment 9 WebKit Review Bot 2013-03-12 20:49:16 PDT 
All reviewed patches have been landed.  Closing bug.