Bug 130182 – [EFL][WK2] Restore cursor when moving mouse into webview

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

Now, ewebkit changed mouse cursor when the type of cursor is changed.

But ewebkit does not know whether other widgets such as elm_entry change the cursor.
So we should restore the cursor when mouser enters on webview not to show wrong cursor.

[Ryuan Choi]

Created attachment 226581 [details]
suggestion

[Jinwoo Song]

Comment on attachment 226581 [details]
suggestion

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1165
> +    EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::subscribe(evasObject);

Why don't you follow the alphabetic order?

[Ryuan Choi]

Created attachment 226871 [details]
Patch

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 226581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226581&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1165
> > +    EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::subscribe(evasObject);
> 
> Why don't you follow the alphabetic order?

Fixed order.

[Jinwoo Song]

Looks good to me.

[Gyuyoung Kim]

Comment on attachment 226871 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
> +        m_useCustomCursor = true;

If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?

> Source/WebKit2/UIProcess/API/efl/EwkView.h:280
> +    } m_cursorIdentifier; // This is an address, do not free it.

Hmm, is m_cursorIdentifier still an address ?

[Ryuan Choi]

Created attachment 226899 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 226871 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226871&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
> > +        m_useCustomCursor = true;
> 
> If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?
> 

Unfortunately, we should save the state of cursor for the MOUSE_IN callback.

> > Source/WebKit2/UIProcess/API/efl/EwkView.h:280
> > +    } m_cursorIdentifier; // This is an address, do not free it.
> 
> Hmm, is m_cursorIdentifier still an address ?

Right, I removed the comment.

[Gyuyoung Kim]

Comment on attachment 226871 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
>>> +        m_useCustomCursor = true;
>> 
>> If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?
> 
> Unfortunately, we should save the state of cursor for the MOUSE_IN callback.

It looks m_useCustomCursor is only used by updateCursor(). Could you let me know how is this used by the MOUSE_IN callback ?

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 226871 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226871&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
> >>> +        m_useCustomCursor = true;
> >> 
> >> If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?
> > 
> > Unfortunately, we should save the state of cursor for the MOUSE_IN callback.
> 
> It looks m_useCustomCursor is only used by updateCursor(). Could you let me know how is this used by the MOUSE_IN callback ?

Yes and EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent() and setCursor() call updateCursor().

Although we can pass the true/false to updateCursor(), we should keep the any member variable to know which type of cursor(group or image) we will restore in the MOUSE_IN callback.

[Gyuyoung Kim]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 226871 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=226871&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
> > >>> +        m_useCustomCursor = true;
> > >> 
> > >> If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?
> > > 
> > > Unfortunately, we should save the state of cursor for the MOUSE_IN callback.
> > 
> > It looks m_useCustomCursor is only used by updateCursor(). Could you let me know how is this used by the MOUSE_IN callback ?
> 
> Yes and EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent() and setCursor() call updateCursor().

I can't find "EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent()" in EwkView.cpp. Is it supported by WebKit EFL trunk now ? Please let me know where it is implemented.

[Gyuyoung Kim]

Comment on attachment 226871 [details]
Patch

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

>>>>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:498
>>>>>> +        m_useCustomCursor = true;
>>>>> 
>>>>> If possible, I prefer not to keep a member variable for boolean flag. Can't we pass this true/false to updateCursor() as an argument ?
>>>> 
>>>> Unfortunately, we should save the state of cursor for the MOUSE_IN callback.
>>> 
>>> It looks m_useCustomCursor is only used by updateCursor(). Could you let me know how is this used by the MOUSE_IN callback ?
>> 
>> Yes and EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent() and setCursor() call updateCursor().
>> 
>> Although we can pass the true/false to updateCursor(), we should keep the any member variable to know which type of cursor(group or image) we will restore in the MOUSE_IN callback.
> 
> I can't find "EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent()" in EwkView.cpp. Is it supported by WebKit EFL trunk now ? Please let me know where it is implemented.

I missed to see "EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent()" in this patch. Sorry for noise.

[Gyuyoung Kim]

Comment on attachment 226899 [details]
Patch

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

LGTM now.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:492
> +    if (cursor.image()) {

Trivial recommendation : How about using isNull() explicitly as below ?

!cursor.image()->isNull()

[WebKit Commit Bot]

Comment on attachment 226899 [details]
Patch

Clearing flags on attachment: 226899

Committed r165730: <http://trac.webkit.org/changeset/165730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.