Bug 150310 - [EFL] Implement WebContextMenuProxyEfl::showContextMenu after r191194
Summary: [EFL] Implement WebContextMenuProxyEfl::showContextMenu after r191194
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hunseop Jeong
URL:
Keywords:
Depends on: 150240
Blocks: 150311
  Show dependency treegraph
 
Reported: 2015-10-18 21:09 PDT by Hunseop Jeong
Modified: 2015-12-21 00:39 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.65 KB, patch)
2015-10-18 21:13 PDT, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (3.65 KB, patch)
2015-10-18 22:15 PDT, Hunseop Jeong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.