Summary: | [EFL][WK2] add WebKit2 EFL port's NativeWebMouseEvent, NativeWebWheelEvent and NativeWebKeyboardEvent | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eunmi Lee <enmi.lee> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | antognolli+webkit, gyuyoung.kim, kenneth, leandro, ryuan.choi, sangseok.lim, tonikitoo, webkit.review.bot, youngtaeck.song | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 62451 | ||||||||||||||||||
Bug Blocks: | 61838 | ||||||||||||||||||
Attachments: |
|
Description
Eunmi Lee
2011-06-02 23:24:15 PDT
Created attachment 95856 [details]
Patch
Comment on attachment 95856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95856&action=review I know nothing about WebKit2, but I can comment about some style issues. > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:37 > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Down* event, const Evas_Point* position) > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > + , m_nativeEvent(event) > +{ > +} notImplemented(); > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:43 > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Up* event, const Evas_Point* position) > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > + , m_nativeEvent(event) > +{ > +} notImplemented(); > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:49 > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Move* event, const Evas_Point* position) > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > + , m_nativeEvent(event) > +{ > +} notImplemented(); > Source/WebKit2/Shared/efl/NativeWebWheelEventEfl.cpp:37 > +NativeWebWheelEvent::NativeWebWheelEvent(const Evas_Event_Mouse_Wheel* event, const Evas_Point* position) > + : WebWheelEvent(WebEventFactory::createWebWheelEvent(event, position)) > + , m_nativeEvent(event) > +{ > +} notImplemented(); > Source/WebKit2/Shared/efl/WebEventFactory.cpp:52 > + return (WebEvent::Modifiers)result; Use C++-style cast. > Source/WebKit2/Shared/efl/WebEventFactory.h:43 > + Whereis the #endif? Created attachment 96178 [details]
Patch
(In reply to comment #2) > (From update of attachment 95856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95856&action=review > > I know nothing about WebKit2, but I can comment about some style issues. > > > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:37 > > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Down* event, const Evas_Point* position) > > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > > + , m_nativeEvent(event) > > +{ > > +} > > notImplemented(); > Those are all codes for NativeWebMouseEvent::NativeWebMouseEvent. There is nothing to implement more. > > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:43 > > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Up* event, const Evas_Point* position) > > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > > + , m_nativeEvent(event) > > +{ > > +} > > notImplemented(); > Ditto. > > Source/WebKit2/Shared/efl/NativeWebMouseEventEfl.cpp:49 > > +NativeWebMouseEvent::NativeWebMouseEvent(const Evas_Event_Mouse_Move* event, const Evas_Point* position) > > + : WebMouseEvent(WebEventFactory::createWebMouseEvent(event, position)) > > + , m_nativeEvent(event) > > +{ > > +} > > notImplemented(); > Ditto. > > Source/WebKit2/Shared/efl/NativeWebWheelEventEfl.cpp:37 > > +NativeWebWheelEvent::NativeWebWheelEvent(const Evas_Event_Mouse_Wheel* event, const Evas_Point* position) > > + : WebWheelEvent(WebEventFactory::createWebWheelEvent(event, position)) > > + , m_nativeEvent(event) > > +{ > > +} > > notImplemented(); > Ditto. > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:52 > > + return (WebEvent::Modifiers)result; > > Use C++-style cast. > I've changed code to use C++ style cast. > > Source/WebKit2/Shared/efl/WebEventFactory.h:43 > > + > > Whereis the #endif? The #endif exists in the line 44 :) Comment on attachment 96178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96178&action=review Patch looks OK, but I don't know much about WebKit2 to properly review it. > Source/WebKit2/ChangeLog:8 > + [EFL][WK2] add WebKit2 EFL port's NativeWebMouseEvent and NativeWebWheelEvent > + https://bugs.webkit.org/show_bug.cgi?id=61993 > + > + Add WebKit2 EFL port's NativeWebMouseEvent and NativeWebWheelEvent ChangeLog entry says the same thing twice. Created attachment 96343 [details]
Patch
(In reply to comment #5) > (From update of attachment 96178 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96178&action=review > > Patch looks OK, but I don't know much about WebKit2 to properly review it. > > > Source/WebKit2/ChangeLog:8 > > + [EFL][WK2] add WebKit2 EFL port's NativeWebMouseEvent and NativeWebWheelEvent > > + https://bugs.webkit.org/show_bug.cgi?id=61993 > > + > > + Add WebKit2 EFL port's NativeWebMouseEvent and NativeWebWheelEvent > > ChangeLog entry says the same thing twice. I've changed the ChangeLog. I'm always thankful for your review :) (In reply to comment #5) > (From update of attachment 96178 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96178&action=review > > Patch looks OK, but I don't know much about WebKit2 to properly review it. LGTM. However, I don't know who can review this patch as well. Let's find proper reviewers. Created attachment 96376 [details]
Patch
Created attachment 96732 [details]
Patch
(In reply to comment #10) > Created an attachment (id=96732) [details] > Patch I've added NativeWebKeyboardEvent. Comment on attachment 96732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96732&action=review > Source/WebKit2/ChangeLog:7 > + Missing summary for Patch > Source/WebKit2/Shared/efl/WebEventFactory.cpp:60 > + if (button == 1) I think it is better to use constant instead of "1" > Source/WebKit2/Shared/efl/WebEventFactory.cpp:62 > + if (button == 2) ditto. Created attachment 97105 [details]
Patch
(In reply to comment #12) > (From update of attachment 96732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96732&action=review > > > Source/WebKit2/ChangeLog:7 > > + > > Missing summary for Patch Done. > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:60 > > + if (button == 1) > > I think it is better to use constant instead of "1" > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:62 > > + if (button == 2) > > ditto. Done. I added enum for that. Comment on attachment 97105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97105&action=review Informal R+. > Source/WebKit2/ChangeLog:8 > + [EFL][WK2] add WebKit2 EFL port's NativeWebMouseEvent, NativeWebWheelEvent and NativeWebKeyboardEvent > + https://bugs.webkit.org/show_bug.cgi?id=61993 > + > + add native mouse and keyboard event classes to convert EFL's events to NativeWebEvent. Start sentences with uppercase letters. Comment on attachment 97105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97105&action=review >> Source/WebKit2/ChangeLog:8 >> + add native mouse and keyboard event classes to convert EFL's events to NativeWebEvent. > > Start sentences with uppercase letters. Add* > Source/WebKit2/Shared/efl/WebEventFactory.cpp:46 > +enum { > + LeftButton = 1, > + MiddleButton = 2, > + RightButton = 3 > +}; Is this always the case for all mouse types? Created attachment 97217 [details]
Patch
(In reply to comment #16) > (From update of attachment 97105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97105&action=review > > >> Source/WebKit2/ChangeLog:8 > >> + add native mouse and keyboard event classes to convert EFL's events to NativeWebEvent. > > > > Start sentences with uppercase letters. > > Add* > Done. > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:46 > > +enum { > > + LeftButton = 1, > > + MiddleButton = 2, > > + RightButton = 3 > > +}; > > Is this always the case for all mouse types? I am not sure about that, but I think I can't support more because WebMouseEvent supports only those three buttons now. Comment on attachment 97217 [details] Patch Clearing flags on attachment: 97217 Committed r89211: <http://trac.webkit.org/changeset/89211> All reviewed patches have been landed. Closing bug. |