Bug 120609 - Web Inspector: Context Menu doesn't work on EFL port
Summary: Web Inspector: Context Menu doesn't work on EFL port
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Andre Loureiro (IRC: loureiro)
URL:
Keywords: InRadar
Depends on: 118676
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-02 12:37 PDT by Andre Loureiro (IRC: loureiro)
Modified: 2016-12-13 15:32 PST (History)
13 users (show)

See Also:


Attachments
showing context menu at WebInspector view (197.94 KB, image/png)
2013-10-04 12:13 PDT, Andre Loureiro (IRC: loureiro)
no flags Details
Patch attached (24.37 KB, patch)
2013-10-09 09:02 PDT, Andre Loureiro (IRC: loureiro)
no flags Details | Formatted Diff | Diff
Patch (24.71 KB, patch)
2013-10-09 11:52 PDT, Andre Loureiro (IRC: loureiro)
gyuyoung.kim: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Loureiro (IRC: loureiro) 2013-09-02 12:37:47 PDT
Right click on Toolbar doesn't show the context menu allowing the user change among Icons layouts, for example:
Icon and Text (Vertical)
Icon and Text (Horizontal)
Icon Only
Text Only
Small Icons
Comment 1 Radar WebKit Bug Importer 2013-09-02 12:38:29 PDT
<rdar://problem/14892871>
Comment 2 Andre Loureiro (IRC: loureiro) 2013-10-04 12:13:53 PDT
Created attachment 213387 [details]
showing context menu at WebInspector view

Screenshot from a initial implementation of context menu in a WebInspector view
Comment 3 Timothy Hatcher 2013-10-04 22:35:08 PDT
Is there a reason why the Linux ports don't use a native InspectorFrontEndHost like Mac and Windows? That would let you use a real context menu and not need to create it in HTML. It would be better to have a common path and no split the ports.
Comment 4 Andre Loureiro (IRC: loureiro) 2013-10-07 06:09:38 PDT
(In reply to comment #3)
> Is there a reason why the Linux ports don't use a native InspectorFrontEndHost like Mac and Windows? That would let you use a real context menu and not need to create it in HTML. It would be better to have a common path and no split the ports.

Do you mean there is a way to implement cross platform context menus? If so, Is it a feature of the old inspector that would be keep in the new inspector implementation? besides that, I don't know how it will impact in the EFL port, because it uses "EWK" to implement the UI. I believe someone from EFL port could explain better, @gyuyoung?
Comment 5 Timothy Hatcher 2013-10-07 09:14:28 PDT
The Mac and Windows ports use the native InspectorFrontEndHost, not InspectorFrontEndHostStubs.js.

So we use WebCore's ContextMenu code. See:

WebCore::InspectorFrontendHost::showContextMenu
Comment 6 Gyuyoung Kim 2013-10-07 18:29:50 PDT
CC'ing Michal, could you take a look this ?
Comment 7 Andre Loureiro (IRC: loureiro) 2013-10-09 09:02:24 PDT
Created attachment 213782 [details]
Patch attached
Comment 8 WebKit Commit Bot 2013-10-09 09:05:00 PDT
Attachment 213782 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/UIProcess/API/efl/ewk_view.cpp', u'Source/WebKit2/UIProcess/API/efl/ewk_view.h', u'Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp', u'Source/WebKit2/UIProcess/WebInspectorProxy.cpp', u'Source/WebKit2/UIProcess/WebInspectorProxy.h', u'Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp', u'Tools/ChangeLog', u'Tools/MiniBrowser/efl/main.c']" exit_code: 1
Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:95:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:98:  Declaration has space between type name and * in Evas_Object *bg  [whitespace/declaration] [3]
Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:106:  Declaration has space between type name and * in Evas_Object *layout  [whitespace/declaration] [3]
Source/WebKit2/PlatformEfl.cmake:250:  Alphabetical sorting problem. "${EET_LIBRARIES}" should be before "${EINA_LIBRARIES}".  [list/order] [5]
Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:477:  Declaration has space between type name and * in Evas_Object *ewk_view_inspector_window_get  [whitespace/declaration] [3]
Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:493:  Declaration has space between type name and * in Evas_Object *ewk_view_inspector_view_get  [whitespace/declaration] [3]
Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 8 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Andre Loureiro (IRC: loureiro) 2013-10-09 11:52:39 PDT
Created attachment 213796 [details]
Patch

Fix code style
Comment 10 Gyuyoung Kim 2013-10-09 23:15:47 PDT
Comment on attachment 213796 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Use EFL Elementary Window widget set to create the a new inspector window instead

Hmm, EFL community has objected to use Elementary inside WebKit. Because, they have a elm_web as a sub module of elementary. So, if we use Elementary inside WebKit, circular dependency occurs. Ryuan, how do you think about this ?
Comment 11 Ryuan Choi 2013-10-09 23:25:15 PDT
(In reply to comment #10)
> (From update of attachment 213796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213796&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Use EFL Elementary Window widget set to create the a new inspector window instead
> 
> Hmm, EFL community has objected to use Elementary inside WebKit. Because, they have a elm_web as a sub module of elementary. So, if we use Elementary inside WebKit, circular dependency occurs. Ryuan, how do you think about this ?

Yes, this is not acceptable.
Comment 12 Daniel Juyung Seo 2013-10-10 00:02:21 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 213796 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213796&action=review
> > 
> > > Source/WebKit2/ChangeLog:8
> > > +        Use EFL Elementary Window widget set to create the a new inspector window instead
> > 
> > Hmm, EFL community has objected to use Elementary inside WebKit. Because, they have a elm_web as a sub module of elementary. So, if we use Elementary inside WebKit, circular dependency occurs. Ryuan, how do you think about this ?
> 
> Yes, this is not acceptable.

Hello, this is Daniel Juyung Seo from EFL team.
Sorry but as Elementary depends on WebKit, WebKit can not depend on Elementary.
That will lead to a circular dependency. "elm_web" widget of Elementary uses ewk_view internally.

I know it will be good if elm_web is separated from Elementary and WebKit uses Elementary. But this is not what it is at the moment.

That can happen in EFL 2.0 but there is no plan for EFL 2.0 yet.

Thanks for understanding.
Comment 13 Andre Loureiro (IRC: loureiro) 2013-10-10 06:55:49 PDT
> Hello, this is Daniel Juyung Seo from EFL team.
> Sorry but as Elementary depends on WebKit, WebKit can not depend on Elementary.
> That will lead to a circular dependency. "elm_web" widget of Elementary uses ewk_view internally.
> 
> I know it will be good if elm_web is separated from Elementary and WebKit uses Elementary. But this is not what it is at the moment.
> 
> That can happen in EFL 2.0 but there is no plan for EFL 2.0 yet.
> 
> Thanks for understanding.

Ok, I got it, so what you suggest to implement it?
Anyway thanks for you reply.
Comment 14 Gyuyoung Kim 2013-10-10 22:55:08 PDT
(In reply to comment #13)
> Ok, I got it, so what you suggest to implement it?

We can use element in MiniBrowser. So, we have delegate the elementary use to application side(e.g. MiniBrowser). Isn't there method to delegate the elementary use to MiniBrowser ? For instance, you may add new APIs for it.
Comment 15 Gyuyoung Kim 2013-10-10 22:55:51 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Ok, I got it, so what you suggest to implement it?
> 
> We can use element in MiniBrowser. So, we have delegate the elementary use to application side(e.g. MiniBrowser). Isn't there method to delegate the elementary use to MiniBrowser ? For instance, you may add new APIs for it.

We can use element in MiniBrowser.

*element* -> *elementary*
Comment 16 Gyuyoung Kim 2013-10-10 22:58:45 PDT
Comment on attachment 213796 [details]
Patch

Set r- because of review comments.
Comment 17 Joseph Pecoraro 2013-10-14 11:51:56 PDT
(In reply to comment #3)
> Is there a reason why the Linux ports don't use a native InspectorFrontEndHost like Mac and Windows? That would let you use a real context menu and not need to create it in HTML. It would be better to have a common path and no split the ports.

If EFL has remote inspection via a WebSocket connection then there will need to be an HTML context menu. But for now it sounds like this is not regarding remote inspection and it sounds like EFL is working toward a good solution.