Bug 61993

Summary: [EFL][WK2] add WebKit2 EFL port's NativeWebMouseEvent, NativeWebWheelEvent and NativeWebKeyboardEvent
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2011-06-02 23:24:15 PDT
I have added NativeWebMouseEvent and NativeWebWheelEvent for WebKit2 EFL port.
Comment 1 Eunmi Lee 2011-06-02 23:36:55 PDT
Created attachment 95856 [details]
Patch
Comment 2 Leandro Pereira 2011-06-03 10:14:03 PDT
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?
Comment 3 Eunmi Lee 2011-06-06 19:14:00 PDT
Created attachment 96178 [details]
Patch
Comment 4 Eunmi Lee 2011-06-06 19:19:34 PDT
(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 5 Leandro Pereira 2011-06-07 07:34:48 PDT
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.
Comment 6 Eunmi Lee 2011-06-07 17:42:48 PDT
Created attachment 96343 [details]
Patch
Comment 7 Eunmi Lee 2011-06-07 17:46:25 PDT
(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 :)
Comment 8 Gyuyoung Kim 2011-06-07 18:50:29 PDT
(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.
Comment 9 Eunmi Lee 2011-06-07 21:50:04 PDT
Created attachment 96376 [details]
Patch
Comment 10 Eunmi Lee 2011-06-10 04:39:37 PDT
Created attachment 96732 [details]
Patch
Comment 11 Eunmi Lee 2011-06-10 04:40:16 PDT
(In reply to comment #10)
> Created an attachment (id=96732) [details]
> Patch

I've added NativeWebKeyboardEvent.
Comment 12 Gyuyoung Kim 2011-06-13 20:06:52 PDT
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.
Comment 13 Eunmi Lee 2011-06-14 06:23:19 PDT
Created attachment 97105 [details]
Patch
Comment 14 Eunmi Lee 2011-06-14 06:31:00 PDT
(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 15 Leandro Pereira 2011-06-14 08:51:35 PDT
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 16 Kenneth Rohde Christiansen 2011-06-14 15:26:00 PDT
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?
Comment 17 Eunmi Lee 2011-06-14 18:51:10 PDT
Created attachment 97217 [details]
Patch
Comment 18 Eunmi Lee 2011-06-14 18:56:11 PDT
(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 19 WebKit Review Bot 2011-06-18 20:44:15 PDT
Comment on attachment 97217 [details]
Patch

Clearing flags on attachment: 97217

Committed r89211: <http://trac.webkit.org/changeset/89211>
Comment 20 WebKit Review Bot 2011-06-18 20:44:21 PDT
All reviewed patches have been landed.  Closing bug.