Summary: | [EFL][DRT] EventSender requires contextClick implementation . | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Krzysztof Czech <k.czech> | ||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | g.czajkowski, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, sw0524.lee, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||
Bug Depends on: | 94925 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Krzysztof Czech
2012-05-10 06:16:56 PDT
Created attachment 164529 [details]
patch
This patch contains an contextClick implementation basing on a WebKit GTK one. A change in ewk_context_menu was introduced to allow selecting context menu items without having a pointer to parent menu object.
Created attachment 164531 [details]
rebased patch
Comment on attachment 164529 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=164529&action=review We started to add unit test cases for WK1 public API too. Please support it as well. > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56 > + Ewk_Context_Menu* parentmenu; Missing comment. > Source/WebKit/efl/ewk/ewk_contextmenu.h:329 > + * @return a context menu object You need to mention on failure case as well. > Source/WebKit/efl/ewk/ewk_contextmenu.h:331 > +EAPI Ewk_Context_Menu *ewk_context_menu_item_parent_get(Ewk_Context_Menu_Item *o); Missing const keyword. > Source/WebKit/efl/ewk/ewk_view.h:2751 > +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(Evas_Object* ewkView); Missing const keyword for _get function. (In reply to comment #3) > (From update of attachment 164529 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164529&action=review > > We started to add unit test cases for WK1 public API too. Please support it as well. > OK I will add two cases after 94925 will be updated > > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56 > > + Ewk_Context_Menu* parentmenu; > > Missing comment. > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:329 > > + * @return a context menu object > > You need to mention on failure case as well. > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:331 > > +EAPI Ewk_Context_Menu *ewk_context_menu_item_parent_get(Ewk_Context_Menu_Item *o); > > Missing const keyword. > > > Source/WebKit/efl/ewk/ewk_view.h:2751 > > +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(Evas_Object* ewkView); > > Missing const keyword for _get function. Done, I will also add some documentation to ewk_view_context_menu_get Created attachment 164885 [details] unit tests added Added const modifiers, documentation and unit tests for new and changed API functions. Patch will not build until https://bugs.webkit.org/show_bug.cgi?id=94925 will land again. Comment on attachment 164885 [details] unit tests added Attachment 164885 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13961004 Comment on attachment 164885 [details] unit tests added View in context: https://bugs.webkit.org/attachment.cgi?id=164885&action=review > LayoutTests/platform/efl-wk2/TestExpectations:206 > +// EFL's EventSender does not implement contextClick yet // WebKitTestRunner needs an implementation for eventSender.contextClick (https://bugs.webkit.org/show_bug.cgi?id=86881) > Source/WebKit/PlatformEfl.cmake:331 > + test_ewk_contextmenu Please keep alphabetical order. > Source/WebKit/efl/ewk/ewk_contextmenu.h:326 > + * Gets the parent menu for context menu item Missing comma. Created attachment 166846 [details]
rebased patch
Created attachment 166851 [details]
rebased again
Comment on attachment 166851 [details] rebased again View in context: https://bugs.webkit.org/attachment.cgi?id=166851&action=review > LayoutTests/platform/efl/TestExpectations:1291 > +webkit.org/b/86633 editing/spelling/context-menu-suggestions.html [ Failure ] Wrong alphabetical order. > Source/WebKit/efl/tests/test_ewk_contextmenu.cpp:67 > + Eina_Bool itemChecked = EINA_FALSE; Please use standard boolean. We decided to use standard boolean for test case lately. > Tools/DumpRenderTree/efl/EventSender.cpp:323 > +static JSValueRef contextClickCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) JSValueRef* exception -> JSValueRef* exception ? Created attachment 166886 [details]
fixes
Fixed according to Gyuyoung comments.
Comment on attachment 166886 [details] fixes View in context: https://bugs.webkit.org/attachment.cgi?id=166886&action=review > LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1 > +layer at (0,0) size 800x600 Pixel expectation? > LayoutTests/platform/efl/fast/events/context-no-deselect-expected.txt:1 > +layer at (0,0) size 800x600 Pixel expectation? > Source/WebKit/efl/ewk/ewk_contextmenu.h:203 > +EAPI Ewk_Context_Menu_Item *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu *submenu, Ewk_Context_Menu *parentmenu, const char *title, Eina_Bool checked, Eina_Bool enabled); Missing doc for parent menu argument. Why is parentmenu passed *after* submenu? That seems a bit odd. > Source/WebKit/efl/ewk/ewk_view.h:2784 > +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(const Evas_Object* o); Stars on wrong side. Comment on attachment 166886 [details] fixes View in context: https://bugs.webkit.org/attachment.cgi?id=166886&action=review >> LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1 >> +layer at (0,0) size 800x600 > > Pixel expectation? already did by Raphael >> LayoutTests/platform/efl/fast/events/context-no-deselect-expected.txt:1 >> +layer at (0,0) size 800x600 > > Pixel expectation? same as above >> Source/WebKit/efl/ewk/ewk_contextmenu.h:203 >> +EAPI Ewk_Context_Menu_Item *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu *submenu, Ewk_Context_Menu *parentmenu, const char *title, Eina_Bool checked, Eina_Bool enabled); > > Missing doc for parent menu argument. > Why is parentmenu passed *after* submenu? That seems a bit odd. doc added, I will move parentmenu before submenu >> Source/WebKit/efl/ewk/ewk_view.h:2784 >> +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(const Evas_Object* o); > > Stars on wrong side. fixed Created attachment 168705 [details]
fixed patch
Created attachment 171413 [details]
rebased patch
rebased patch with one pixel test expectation added and TestExpectations file updated due spellcheck changes
Comment on attachment 171413 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=171413&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56 > + Ewk_Context_Menu* parentmenu; /**< contains the pointer to parent menu of the item */ parent_menu would be the usual EFL style, right? > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:102 > +Ewk_Context_Menu_Item* ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu* parentmenu, I would prefer "parentMenu" to follow WebKit coding style. > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:210 > + return item->parentmenu; We usually have a blank line before return statements. > Source/WebKit/efl/ewk/ewk_view.cpp:4740 > + return priv->contextMenu; blank line before. > Source/WebKit/efl/tests/test_ewk_contextmenu.cpp:69 > + Ewk_Context_Menu_Item* contextMenuItem = ewk_context_menu_item_new(itemType, itemAction, contextMenu, 0, itemTitle, itemChecked, itemEnabled); never freed? > Tools/DumpRenderTree/efl/EventSender.cpp:364 > + return valueRef; Blank line before return. Comment on attachment 171413 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=171413&action=review >> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56 >> + Ewk_Context_Menu* parentmenu; /**< contains the pointer to parent menu of the item */ > > parent_menu would be the usual EFL style, right? as it is "private" structure I will use parentMenu as below Created attachment 171465 [details]
fixed patch
Fixed patch according to Christophe's comments.
Comment on attachment 171465 [details]
fixed patch
LGTM.
Comment on attachment 171465 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=171465&action=review > LayoutTests/ChangeLog:3 > + [EFL][DRT] EventSender requires contextClick implementation. Sorry I did not notice that before but we do not usually end bug title with a period (.) Created attachment 171591 [details]
patch for landing
Comment on attachment 171591 [details] patch for landing Clearing flags on attachment: 171591 Committed r133000: <http://trac.webkit.org/changeset/133000> All reviewed patches have been landed. Closing bug. |