Bug 89140 – [EFL][WK2] Add ewk_view_cursor_set to change cursor.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

WebKit2/Efl can receive setCursor from WebProcess after r120369,
but implementation is missing.

[Ryuan Choi]

Created attachment 147684 [details]
Patch

[Chris Dumez]

Comment on attachment 147684 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:553
> +    WebCore::applyFallbackCursor(ecoreEvas, group);

I'm not familiar with this code but why do we have only a fallback implementation here? It looks odd. If I look at the WK1 implementation, we have a normal implementation AND a fallback.

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 147684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147684&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:553
> > +    WebCore::applyFallbackCursor(ecoreEvas, group);
> 
> I'm not familiar with this code but why do we have only a fallback implementation here? It looks odd. If I look at the WK1 implementation, we have a normal implementation AND a fallback.

OK, if then I will revise this after Bug 900107 is landed because implementation of WebKit1/Efl needs theme.

[Ryuan Choi]

Created attachment 152718 [details]
Patch

[Chris Dumez]

Comment on attachment 152718 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 152718 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:818
> +        width = 16;

I think it is better to use constant variable for width and height.

[Ryuan Choi]

Created attachment 152961 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 152718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152718&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:818
> > +        width = 16;
> 
> I think it is better to use constant variable for width and height.

Ok, I add defaultCursorSize for it.

[Gyuyoung Kim]

Comment on attachment 152961 [details]
Patch

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

Looks good

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:334
> +    g_parentSmartClass.add(ewkView);

I wonder why do you move this line ? Do we need to add a view to parent smart class though view object can't get _Ewk_View_Private_Data ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336
> +    smartData->priv = _ewk_view_priv_new(smartData);

Should we move this line ?

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 152961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152961&action=review
> 
> Looks good
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:334
> > +    g_parentSmartClass.add(ewkView);
> 
> I wonder why do you move this line ? Do we need to add a view to parent smart class though view object can't get _Ewk_View_Private_Data ?

Thank you.

By calling `add` of parent smart class, child instance like ewkView initialize internal objects of smart data such as base.evas.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336
> > +    smartData->priv = _ewk_view_priv_new(smartData);
> 
> Should we move this line ?

Yes.
Before calling `add` of parent smart class, smartData is not concrete to use. so previous position is wrong.

And because this patch need to use smartData in _ewk_view_priv_new, we should move it to right position.

[Gyuyoung Kim]

Comment on attachment 152961 [details]
Patch

LGTM.

[Ryuan Choi]

Created attachment 153899 [details]
rebased

[WebKit Review Bot]

Comment on attachment 153899 [details]
rebased

Clearing flags on attachment: 153899

Committed r123593: <http://trac.webkit.org/changeset/123593>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.