Bug 150310

Summary: [EFL] Implement WebContextMenuProxyEfl::showContextMenu after r191194
Product: WebKit Reporter: Hunseop Jeong <hs85.jeong>
Component: WebKit EFLAssignee: Hunseop Jeong <hs85.jeong>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gyuyoung.kim, lucas.de.marchi, ossy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150240    
Bug Blocks: 150311    
Attachments:
Description Flags
Patch
none
Patch none

Description Hunseop Jeong 2015-10-18 21:09:19 PDT
After r191194, showContextMenu implementation is moved from the generic file to the Mac port.
It poses the compile error and the API test fail in EFL port.
Compile error was already fixed in r191239 but API test still failed because showContextMenu() doesn't work correctly.
So we need to implement the showContextMenu().
Comment 1 Hunseop Jeong 2015-10-18 21:13:14 PDT
Created attachment 263439 [details]
Patch
Comment 2 Gyuyoung Kim 2015-10-18 21:26:32 PDT
Comment on attachment 263439 [details]
Patch

Can't we test the showContextMenu() using test_ewk2_context_menu.cpp ?
Comment 3 Hunseop Jeong 2015-10-18 21:48:14 PDT
Yes, we can't test using the the showContextMenu() in test_ewk2_context_menu.cpp because we don't have the implementation of the showContextMenu() in WebContexMenuProxyEfl.cpp. After r191194, showContextMenu() generic implementation codes was moved to MacPort.
So we need the implementation code.

Now contextMenu didn't work correctly in the MiniBrowser and API tests.
Comment 4 Gyuyoung Kim 2015-10-18 21:50:38 PDT
(In reply to comment #3)
> Yes, we can't test using the the showContextMenu() in
> test_ewk2_context_menu.cpp because we don't have the implementation of the
> showContextMenu() in WebContexMenuProxyEfl.cpp. After r191194,
> showContextMenu() generic implementation codes was moved to MacPort.
> So we need the implementation code.
> 
> Now contextMenu didn't work correctly in the MiniBrowser and API tests.

If so, please file a new bug for EFL context menu. (e.g. [EFL] Context menu doesn't work on MiniBrowser.) Then link this bug to the new bug.
Comment 5 Hunseop Jeong 2015-10-18 21:56:49 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Yes, we can't test using the the showContextMenu() in
> > test_ewk2_context_menu.cpp because we don't have the implementation of the
> > showContextMenu() in WebContexMenuProxyEfl.cpp. After r191194,
> > showContextMenu() generic implementation codes was moved to MacPort.
> > So we need the implementation code.
> > 
> > Now contextMenu didn't work correctly in the MiniBrowser and API tests.
> 
> If so, please file a new bug for EFL context menu. (e.g. [EFL] Context menu
> doesn't work on MiniBrowser.) Then link this bug to the new bug.

Okay, I will make the new bug followed your comment.
Comment 6 Gyuyoung Kim 2015-10-18 22:05:48 PDT
Comment on attachment 263439 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.cpp:78
> +    if (!m_ewkView)

How can we call m_ewkView->showContextMenu() when m_ewkView is null ?
Comment 7 Gyuyoung Kim 2015-10-18 22:06:48 PDT
Comment on attachment 263439 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.cpp:45
> +    , m_page(&page)

We don't need to manage m_page with pointer. Just use a reference.
Comment 8 Hunseop Jeong 2015-10-18 22:15:12 PDT
Created attachment 263441 [details]
Patch
Comment 9 Hunseop Jeong 2015-10-18 22:18:41 PDT
(In reply to comment #7)
> Comment on attachment 263439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263439&action=review
> 
> > Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.cpp:45
> > +    , m_page(&page)
> 
> We don't need to manage m_page with pointer. Just use a reference.

I changed the m_page to use the reference.

(In reply to comment #6)
> Comment on attachment 263439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263439&action=review
> 
> > Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.cpp:78
> > +    if (!m_ewkView)
> 
> How can we call m_ewkView->showContextMenu() when m_ewkView is null ?

Oops.... I want to check the m_ewkView whether is null.
Comment 10 WebKit Commit Bot 2015-10-19 02:14:23 PDT
Comment on attachment 263441 [details]
Patch

Clearing flags on attachment: 263441

Committed r191282: <http://trac.webkit.org/changeset/191282>
Comment 11 WebKit Commit Bot 2015-10-19 02:14:27 PDT
All reviewed patches have been landed.  Closing bug.