| Summary: | [EFL] Context menu restore for WebKit-EFL | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||||||||||||||||||||||||||
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | cdumez, gyuyoung.kim, gyuyoung.kim, kenneth, kling, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||
| Bug Blocks: | 74182 | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Michal Pakula vel Rutka
2011-12-09 06:25:12 PST
Created attachment 118569 [details]
proposed patch
Created attachment 118578 [details]
ChangeLog fixes
Created attachment 118579 [details]
ChangeLog fixes
Created attachment 118580 [details]
new files added to previous patch
Comment on attachment 118580 [details] new files added to previous patch View in context: https://bugs.webkit.org/attachment.cgi?id=118580&action=review Personally, I like this patch. EFL port needs context menu so far. :-) > Source/WebKit/efl/ewk/ewk_private.h:170 > +void ewk_context_menu_item_append(Ewk_Context_Menu_Core* o, const WebCore::ContextMenuItem& core); We don't use efl variable name style in internal functions. Change o with menu or coreMenu. > Source/WebKit/efl/ewk/ewk_private.h:173 > +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* o, Vector<Ewk_Context_Menu_Item*>& itemsVector); ditto. Created attachment 120004 [details]
variable style fixes
Fixed internal functions variables names.
Comment on attachment 120004 [details] variable style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=120004&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318 > +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector) I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get? Comment on attachment 120004 [details] variable style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=120004&action=review >> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318 >> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector) > > I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get? how about list ewk_context_menu_core_items_get? (In reply to comment #8) > (From update of attachment 120004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120004&action=review > > >> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318 > >> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector) > > > > I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get? > > how about list ewk_context_menu_core_items_get? Looks better. Created attachment 120444 [details]
change function name
Comment on attachment 120444 [details] change function name View in context: https://bugs.webkit.org/attachment.cgi?id=120444&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:323 > + Eina_List* l; Why don't we use more clear name instead of *l* ? OK, I will change it to listIterator Created attachment 120448 [details]
change list variable name
Comment on attachment 120448 [details] change list variable name View in context: https://bugs.webkit.org/attachment.cgi?id=120448&action=review > Source/JavaScriptCore/wtf/Platform.h:1078 > +#if PLATFORM(WIN) || PLATFORM(EFL) Almost good for me. could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ? (In reply to comment #14) > (From update of attachment 120448 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120448&action=review > > > Source/JavaScriptCore/wtf/Platform.h:1078 > > +#if PLATFORM(WIN) || PLATFORM(EFL) > > Almost good for me. > > could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ? EFL WK1 is enabling CROSS_PLATFORM_CONTEXT_MENU. But, EFL WK2 is not using CROSS_PLATFORM_MENU. As discussed in private channel, Michal will support CROSS_PLATFORM_CONTEXT_MENU on EFL WebKit2. Why don't we disable CROSS_PLATFORM_CONTEXT_MENU when we build EFL WebKit2 until Michal fix it ? If CORSS_PLATFORM_CONTEXT_MENUS is turned off, I remember there were build errors during the WebKit EFL port building. (In reply to comment #14) > (From update of attachment 120448 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120448&action=review > > > Source/JavaScriptCore/wtf/Platform.h:1078 > > +#if PLATFORM(WIN) || PLATFORM(EFL) > > Almost good for me. > > could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ? OK, done, Platform.h definition is enough. (In reply to comment #15) > EFL WK1 is enabling CROSS_PLATFORM_CONTEXT_MENU. But, EFL WK2 is not using CROSS_PLATFORM_MENU. As discussed in private channel, Michal will support CROSS_PLATFORM_CONTEXT_MENU on EFL WebKit2. > > Why don't we disable CROSS_PLATFORM_CONTEXT_MENU when we build EFL WebKit2 until Michal fix it ? > > If CORSS_PLATFORM_CONTEXT_MENUS is turned off, I remember there were build errors during the WebKit EFL port building. Actually from WebKit2 in EFL point there is no or very little difference in WebKit2 implementation between CROSS and non-CROSS_PLATFORM_CONTEXT_MENUS, so switching to CROSS_PLATFORM_CONTEXT_MENUS=on will require only a minor patch. Created attachment 120677 [details]
removed definition from PlatformEfl.cmake
Comment on attachment 120677 [details]
removed definition from PlatformEfl.cmake
Ok, LGTM.
Comment on attachment 120677 [details] removed definition from PlatformEfl.cmake View in context: https://bugs.webkit.org/attachment.cgi?id=120677&action=review > Source/WebCore/platform/ContextMenuItem.h:52 > +#elif PLATFORM(EFL) > +#include "ewk_contextmenu.h" WebKit2 does not use ewk_contextmenu.h How do you think finding better way to use in both webkit and webkit2? yes, you are right, I need to rebase this patch and I will think of this problem bearing in mind WK2... Comment on attachment 120677 [details]
removed definition from PlatformEfl.cmake
Clear flags because of above comment.
In that case when EFL WebKit2 is using non-cross platform context menus I changed the patch so it will use "standard" context menus, with Ewk_Context_Menu in Ewk_View_Private_Data. EWebLauncher patch will be updated soon. Created attachment 135564 [details]
new approach not using cross platform context menus
Comment on attachment 135564 [details]
new approach not using cross platform context menus
Does it make sense to enable context menus even if the client implementation is empty?
(In reply to comment #24) > (From update of attachment 135564 [details]) > Does it make sense to enable context menus even if the client implementation is empty? Unfortunately ContextMenuClientEfl have to be registered to pageClients in Page.h if CONTEXT_MENUS flag is enabled. Also it is used by ContextMenuController - there are virtual methods called on ContextMenuClient m_client, and they have to be implemented. Created attachment 138317 [details]
rebased patch
the patch needed to be rebased, plus some minor fixes added
Attachment 138317 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
Source/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.h:46: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 138319 [details]
rebased patch
style fixes
Created attachment 141178 [details] rebased patch rebased again to match new PlatformMenuDescription after https://bugs.webkit.org/show_bug.cgi?id=84136 Created attachment 142259 [details]
rebased after ewk_private changes
Rebase after splitting ewk_private into separate header files.
Comment on attachment 142259 [details] rebased after ewk_private changes View in context: https://bugs.webkit.org/attachment.cgi?id=142259&action=review Almost make sense except for my comments. > ChangeLog:11 > + No new tests. You have to mention why this change doesn't need to have test cases, Or, if this modification doesn't need to have test case, please remove this line. > Source/WebCore/ChangeLog:11 > + No new tests. ditto. > Source/WebKit/ChangeLog:11 > + No new tests. ditto. > Source/WebKit/efl/ChangeLog:11 > + No new tests. ditto. Comment on attachment 142259 [details] rebased after ewk_private changes View in context: https://bugs.webkit.org/attachment.cgi?id=142259&action=review >> ChangeLog:11 >> + No new tests. > > You have to mention why this change doesn't need to have test cases, Or, if this modification doesn't need to have test case, please remove this line. It's not a new feature, just a new way of implementing, so no new test will be needed. I will remove this lines. Created attachment 143508 [details]
change log fixes
new patch with fixed changelogs according to Gyuyoung comment
Comment on attachment 143508 [details]
change log fixes
Looks fine now. By the way, are there any test cases covered by this patch?
There are some test cases which requires isContextClick to be implemented, which requires this one to land. Comment on attachment 143508 [details]
change log fixes
Please rebase this patch for landing.
Created attachment 161997 [details]
rebased patch
Comment on attachment 161997 [details]
rebased patch
LGTM. Thanks
Comment on attachment 161997 [details] rebased patch Clearing flags on attachment: 161997 Committed r127462: <http://trac.webkit.org/changeset/127462> All reviewed patches have been landed. Closing bug. |