Bug 89140

Summary: [EFL][WK2] Add ewk_view_cursor_set to change cursor.
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90107    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
rebased none

Description Ryuan Choi 2012-06-14 17:11:53 PDT
WebKit2/Efl can receive setCursor from WebProcess after r120369,
but implementation is missing.
Comment 1 Ryuan Choi 2012-06-14 17:20:11 PDT
Created attachment 147684 [details]
Patch
Comment 2 Chris Dumez 2012-06-25 04:47:38 PDT
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.
Comment 3 Ryuan Choi 2012-06-27 16:16:29 PDT
(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.
Comment 4 Ryuan Choi 2012-07-17 01:42:23 PDT
Created attachment 152718 [details]
Patch
Comment 5 Chris Dumez 2012-07-17 23:24:54 PDT
Comment on attachment 152718 [details]
Patch

LGTM.
Comment 6 Gyuyoung Kim 2012-07-17 23:39:46 PDT
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.
Comment 7 Ryuan Choi 2012-07-18 01:22:11 PDT
Created attachment 152961 [details]
Patch
Comment 8 Ryuan Choi 2012-07-18 01:25:39 PDT
(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.
Comment 9 Gyuyoung Kim 2012-07-18 06:41:24 PDT
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 ?
Comment 10 Ryuan Choi 2012-07-18 07:31:46 PDT
(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.
Comment 11 Gyuyoung Kim 2012-07-18 07:38:28 PDT
Comment on attachment 152961 [details]
Patch

LGTM.
Comment 12 Ryuan Choi 2012-07-23 16:49:52 PDT
Created attachment 153899 [details]
rebased
Comment 13 WebKit Review Bot 2012-07-25 02:16:35 PDT
Comment on attachment 153899 [details]
rebased

Clearing flags on attachment: 153899

Committed r123593: <http://trac.webkit.org/changeset/123593>
Comment 14 WebKit Review Bot 2012-07-25 02:16:40 PDT
All reviewed patches have been landed.  Closing bug.