Bug 119120 – [EFL][WK2] Move and make WKColorPickerResultListener to EFL specific interface

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

[Ryuan Choi]

As 61276 and 119030 addressed, WK2 ports implement color picker as two way.
1. inherit WebColorPicker
2. register PageUIClient Callback

Only EFL port implements it as second way.
Second way might be good for some ports to expose UIClient as WK API.
But I think that it just make confusion.

I will refactor EFL port to use first way.

[Ryuan Choi]

Created attachment 207502 [details]
Patch

[Ryuan Choi]

Created attachment 207892 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #0)
> As 61276 and 119030 addressed, WK2 ports implement color picker as two way.
> 1. inherit WebColorPicker
> 2. register PageUIClient Callback
> 
> Only EFL port implements it as second way.
> Second way might be good for some ports to expose UIClient as WK API.
> But I think that it just make confusion.
> 
> I will refactor EFL port to use first way.

As far as I know, we have tried to use C API instead of inheriting WK2 internal class. This may violate a layer rule on EFL ewk implementation. Why not we use second way ?

[Chris Dumez]

Comment on attachment 207892 [details]
Patch

We are using the C API on purpose. Using internal wk2 types from our API layer is a layering violation. Therefore, I believe this patch is a step in the wrong direction.

[Ryuan Choi]

Created attachment 208110 [details]
another suggestion

[Ryuan Choi]

(In reply to comment #4)
> (From update of attachment 207892 [details])
> We are using the C API on purpose. Using internal wk2 types from our API layer is a layering violation. Therefore, I believe this patch is a step in the wrong direction.

OK, but previous logic looks confusing because there were two paths in WebPageProxy and m_uiClient.showColorPicker was only for EFL (at least now)

Instead, I added Efl specific WK API to isolate color picker callbacks only for EFL.

[Early Warning System Bot]

Comment on attachment 208110 [details]
another suggestion

Attachment 208110 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1357023

[Build Bot]

Comment on attachment 208110 [details]
another suggestion

Attachment 208110 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1377054

[Build Bot]

Comment on attachment 208110 [details]
another suggestion

Attachment 208110 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1378051

[Ryuan Choi]

Created attachment 208113 [details]
another suggestion

[Early Warning System Bot]

Comment on attachment 208113 [details]
another suggestion

Attachment 208113 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1380071

[Ryuan Choi]

Created attachment 208114 [details]
another suggestion

[Early Warning System Bot]

Comment on attachment 208114 [details]
another suggestion

Attachment 208114 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1375063

[Ryuan Choi]

Created attachment 208115 [details]
another suggestion

[Build Bot]

Comment on attachment 208115 [details]
another suggestion

Attachment 208115 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1376096

[Gyuyoung Kim]

Comment on attachment 208114 [details]
another suggestion

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

r- because qt-wk2 ews is failed.

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Refactor color picker implementation

I'm not sure if we can use [EFL] prefix in title. Because this patch influences on QT and GTK ports. How about changing with "[WK2] Refactor color picker implementation for EFL port." ? It looks you need to get approval of WK2 owner.

> Source/WebKit2/ChangeLog:12
> +        So, this patch introduce WKColorPickerClient which is new WK interface

introduces ?

[Ryuan Choi]

Created attachment 208116 [details]
another suggestion

[Ryuan Choi]

(In reply to comment #16)
> (From update of attachment 208114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208114&action=review
> 
> r- because qt-wk2 ews is failed.
> 

Sorry for noise, I hope that last patch be fine for all wk2 bot.

> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Refactor color picker implementation
> 
> I'm not sure if we can use [EFL] prefix in title. Because this patch influences on QT and GTK ports. How about changing with "[WK2] Refactor color picker implementation for EFL port." ? It looks you need to get approval of WK2 owner.
> 

I agree with you, so I wanted to separate patches about it because I think that this is for EFL.

If this approach is acceptable, I can move ColorPickerResultListener related files in another bug before landing this.
Or just removing "[EFL]" is acceptable, I will do it.

> > Source/WebKit2/ChangeLog:12
> > +        So, this patch introduce WKColorPickerClient which is new WK interface
> 
> introduces ?

OK, I will fix it in next patch.

[Chris Dumez]

Comment on attachment 208116 [details]
another suggestion

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

It is really not clear to me why this patch improves the current situation and the Changelog does not clarify things. It seems this patch mostly move generic WK2 code and makes it EFL-specific (thus preventing other ports from using it).

> Source/WebKit2/ChangeLog:8
> +        Since r153541, WK2 port removed a way to register PageUIClient Callbacks,

I don't understand this sentence.

> Source/WebKit2/ChangeLog:19
> +        * UIProcess/API/C/efl/WKColorPickerResultListener.cpp: Renamed from Source/WebKit2/UIProcess/API/C/WKColorPickerResultListener.cpp.

I'm not sure we want to move this one to efl/. iirc, the file was put there in the first place because other ports showed interest in using  it as well.

[Santosh Mahto]

Hi ryuan

Crash is happening while clicking to launch color picker.
It looks to me efl need your colorful this patch.

[Gyuyoung Kim]

Any update ?

[Ryuan Choi]

Comment on attachment 208116 [details]
another suggestion

Clearing flags. At least, this patch should be rebased.

[Ryuan Choi]

Created attachment 223207 [details]
rebased

[Ryuan Choi]

(In reply to comment #19)
> (From update of attachment 208116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208116&action=review
> 
> It is really not clear to me why this patch improves the current situation and the Changelog does not clarify things. It seems this patch mostly move generic WK2 code and makes it EFL-specific (thus preventing other ports from using it).
> 
> > Source/WebKit2/ChangeLog:8
> > +        Since r153541, WK2 port removed a way to register PageUIClient Callbacks,
> 
> I don't understand this sentence.
> 
> > Source/WebKit2/ChangeLog:19
> > +        * UIProcess/API/C/efl/WKColorPickerResultListener.cpp: Renamed from Source/WebKit2/UIProcess/API/C/WKColorPickerResultListener.cpp.
> 
> I'm not sure we want to move this one to efl/. iirc, the file was put there in the first place because other ports showed interest in using  it as well.

I updated bug title and change log little bit.

Since I made this patch, EFL port has a crash when accessed color input.
I don't know who want to use WKColorPickerResultListener but I believe that EFL port will only use it until new port are appeared.

So, I want to move them out of the common code until someone raise their needs once more.

[Jinwoo Song]

Comment on attachment 223207 [details]
rebased

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

> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:56
> +#if ENABLE(TOUCH_EVENTS)

INPUT_TYPE_COLOR?

[Ryuan Choi]

Comment on attachment 223207 [details]
rebased

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

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:56
>> +#if ENABLE(TOUCH_EVENTS)
> 
> INPUT_TYPE_COLOR?

Unbelievable. I will fix it.

Thank you so much.

[Ryuan Choi]

Created attachment 223209 [details]
rebased

[Ryuan Choi]

Created attachment 223210 [details]
rebased

[Gyuyoung Kim]

Comment on attachment 223210 [details]
rebased

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

> Source/WebKit2/ChangeLog:9
> +        Indeed, it does not work since refactored common logic for ColorPicker in WebPageProxy.

Are you sure if Mac or GTK won't use this logic in future ?

> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:199
> +#if ENABLE(INPUT_TYPE_COLOR)

Why can't we initialize colorPickerClient when color picker needs to be shown ?

> Source/WebKit2/UIProcess/efl/WebColorPickerEfl.h:48
> +    void didChooseColor(const WebCore::Color&);

Add a new line.

[Ryuan Choi]

Comment on attachment 223210 [details]
rebased

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

>> Source/WebKit2/ChangeLog:9
>> +        Indeed, it does not work since refactored common logic for ColorPicker in WebPageProxy.
> 
> Are you sure if Mac or GTK won't use this logic in future ?

Absolutely.
Mac implemented color picker using another way and GTK port does not use WK interface(And GTK also use color picker like Mac port, IIRC)
They don't need to use interface because they can implement picker in the platform.

But in EFL side, we can't implement complex UI components in ewebkit because we don't use elm.

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:199
>> +#if ENABLE(INPUT_TYPE_COLOR)
> 
> Why can't we initialize colorPickerClient when color picker needs to be shown ?

It is to receive callback which notify when color picker needs to be shown.

>> Source/WebKit2/UIProcess/efl/WebColorPickerEfl.h:48
>> +    void didChooseColor(const WebCore::Color&);
> 
> Add a new line.

OK, I will add it.

[Ryuan Choi]

Created attachment 224459 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 224459 [details]
Patch

This patch was created a very long time ago. I think now is time to move this to EFL specific place. However, if any port will want to use this concept in future, we need to roll this change back. Looks fine for now.

[Ryuan Choi]

Comment on attachment 224459 [details]
Patch

Clearing flags on attachment: 224459

Committed r164271: <http://trac.webkit.org/changeset/164271>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.

[Michal Pakula vel Rutka]

*** Bug 126067 has been marked as a duplicate of this bug. ***