Summary: | [EFL][WK2] Move and make WKColorPickerResultListener to EFL specific interface | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abecsi, buildbot, bunhere, cdumez, cmarcelo, commit-queue, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, menard, mikhail.pozdnyakov, mpakulavelrutka, rakuco, rego+ews, rniwa, ruthiecftg, santosh.ma, sergio, xan.lopez | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 126960 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Ryuan Choi
2013-07-25 19:30:20 PDT
Created attachment 207502 [details]
Patch
Created attachment 207892 [details]
Patch
(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 ? 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.
Created attachment 208110 [details]
another suggestion
(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. 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 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 Comment on attachment 208110 [details] another suggestion Attachment 208110 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1378051 Created attachment 208113 [details]
another suggestion
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 Created attachment 208114 [details]
another suggestion
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 Created attachment 208115 [details]
another suggestion
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 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 ? Created attachment 208116 [details]
another suggestion
(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. 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. Hi ryuan Crash is happening while clicking to launch color picker. It looks to me efl need your colorful this patch. Any update ? Comment on attachment 208116 [details]
another suggestion
Clearing flags. At least, this patch should be rebased.
Created attachment 223207 [details]
rebased
(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. 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? 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. Created attachment 223209 [details]
rebased
Created attachment 223210 [details]
rebased
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. 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. Created attachment 224459 [details]
Patch
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.
Comment on attachment 224459 [details] Patch Clearing flags on attachment: 224459 Committed r164271: <http://trac.webkit.org/changeset/164271> All reviewed patches have been landed. Closing bug. *** Bug 126067 has been marked as a duplicate of this bug. *** |