Bug 150310 – [EFL] Implement WebContextMenuProxyEfl::showContextMenu after r191194

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hunseop Jeong]

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

[Hunseop Jeong]

Created attachment 263439 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 263439 [details]
Patch

Can't we test the showContextMenu() using test_ewk2_context_menu.cpp ?

[Hunseop Jeong]

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.

[Gyuyoung Kim]

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

[Hunseop Jeong]

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

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

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.

[Hunseop Jeong]

Created attachment 263441 [details]
Patch

[Hunseop Jeong]

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

[WebKit Commit Bot]

Comment on attachment 263441 [details]
Patch

Clearing flags on attachment: 263441

Committed r191282: <http://trac.webkit.org/changeset/191282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.