Bug 110209

Summary: [EFL][WK2] ewk_popup_menu_close() does not work as intended
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108934, 111543    
Attachments:
Description Flags
Patch none

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.