Bug 115890 – [EFL] Implement colorpicker for HTML5 input type color on Minibrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jungsik Tae]

Colorpicker implemnetation by using existent colorpicker API.

[Jungsik Tae]

Created attachment 201512 [details]
Patch

[Jinwoo Song]

Comment on attachment 201512 [details]
Patch

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

> Tools/ChangeLog:3
> +        [EFL]Implement colorpicker for HTML5 input type color on Minibrowser

s/colorpicker/color picker UI/ ?

> Tools/MiniBrowser/efl/main.c:67
> +

Unnecessary modification.

> Tools/MiniBrowser/efl/main.c:95
> +    Ewk_Color_Picker *colorPicker;

s/colorPicker/color_picker/

> Tools/MiniBrowser/efl/main.c:605
> +on_changed_color(void *data, Evas_Object *obj, void *event_info)

on_color_changed?

> Tools/MiniBrowser/efl/main.c:608
> +    Color_Picker_Data *cp_data = (Color_Picker_Data *) data;

Remove blank after right bracket.

> Tools/MiniBrowser/efl/main.c:618
> +    Color_Picker_Data *cp_data = (Color_Picker_Data *) data;

ditto.

> Tools/MiniBrowser/efl/main.c:619
> +    Elm_Object_Item *color_item = (Elm_Object_Item *) event_info;

ditto.

> Tools/MiniBrowser/efl/main.c:661
> +    window->color_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_DIALOG_BASIC);

IMO, we should use a modal popup instead of elm_window.

[Jungsik Tae]

Created attachment 201667 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201667 [details]
Patch

Attachment 201667 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/465523

[Jungsik Tae]

Created attachment 201671 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201671 [details]
Patch

Attachment 201671 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/472033

[Jungsik Tae]

Created attachment 201672 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201672 [details]
Patch

Attachment 201672 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/475015

[Gyuyoung Kim]

Comment on attachment 201671 [details]
Patch

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

r- because of efl ews fail and some comments.

> Tools/ChangeLog:8
> +        When user click HTML5 input type color, 

click -> clicks

> Tools/ChangeLog:11
> +        The evas_objet_color_set() set a color value returning to browser.

set -> sets ?

> Tools/MiniBrowser/efl/main.c:666
> +    cp_data->colorPicker = color_Picker;

colorPicker -> color_picker ?

> Tools/MiniBrowser/efl/main.c:693
> +    fr = elm_frame_add(window->color_selector);

I don't see why you add a frame to *fr* as well as 679 line.

[Jungsik Tae]

Created attachment 201680 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 201680 [details]
Patch

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

It would be good if you submit a screenshot of color picker by this patch.

> Tools/MiniBrowser/efl/main.c:614
> +on_colorpalette_clicked(void *data, Evas_Object *obj, void *event_info)

Looks color_palette better.

> Tools/MiniBrowser/efl/main.c:625
> +on_colorpalette_longpressed(void *data, Evas_Object *obj, void *event_info)

ditto. longpressed -> long_pressed ?

> Tools/MiniBrowser/efl/main.c:636
> +on_colorpicker_ok_clicked(void *data, Evas_Object *obj, void *event_info)

ditto.

> Tools/MiniBrowser/efl/main.c:648
> +on_colorpicker_dismiss(Ewk_View_Smart_Data *sd)

ditto

> Tools/MiniBrowser/efl/main.c:657
> +on_colorpicker_request(Ewk_View_Smart_Data *sd, Ewk_Color_Picker* color_picker)

Wrong * place at Ewk_Color_Picker* color_picker.

[Chris Dumez]

Comment on attachment 201680 [details]
Patch

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

The color picker looks nice but there are a few outstanding issues that need fixing.

> Tools/ChangeLog:14
> +        (_Color_Picker_Data): To deliver two object of different type.

This comment is not really needed / useful.

> Tools/MiniBrowser/efl/main.c:96
> +} Color_Picker_Data;

I don't think we really need this struct.

> Tools/MiniBrowser/efl/main.c:117
> +    Evas_Object *color_selector;

This could be an inline struct with 2 members:
Evas_Object *color_selector;
Evas_Object *rect_data;

Similar to the "popup" above.

>> Tools/MiniBrowser/efl/main.c:625
>> +on_colorpalette_longpressed(void *data, Evas_Object *obj, void *event_info)
> 
> ditto. longpressed -> long_pressed ?

This function looks identical to on_colorpalette_clicked, please reuse the same function instead of duplicating code.

> Tools/MiniBrowser/efl/main.c:641
> +    evas_object_color_get(cp_data->rect_data, &r, &g, &b, &a);

If you pass the window as userData, you can get the color from the colorSelector widget instead of the rect.

> Tools/MiniBrowser/efl/main.c:652
> +

Looks like cp_data may be leaked if the view requests to dismiss the color picker.

> Tools/MiniBrowser/efl/main.c:660
> +    window->color_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_DIALOG_BASIC);

You probably need to do some cleanup when the color picker window is destroyed or it may lead to problems. Try the following:
1. Open http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_input_type_color
2. Click the color input element to show the color picker
3. Close the color picker window by clicking the 'x' button (NOT by clicking OK button)
4. Click the "Submit" button

CRASH:
CRI<2046>:evas_main main.c:97 evas_debug_magic_null() Input object is zero'ed out (maybe a freed object or zero-filled RAM)!
ASSERTION FAILED: m_colorChooser
/home/chris/devel/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp(2965) : void WebKit::WebPageProxy::endColorChooser()
1   0x7ff0113fa726 WebKit::WebPageProxy::endColorChooser()
2   0x7ff01160f370 void CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::Arguments0 const&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
3   0x7ff01160b97a void CoreIPC::handleMessage<Messages::WebPageProxy::EndColorChooser, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::MessageDecoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
4   0x7ff0116058e0 WebKit::WebPageProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
5   0x7ff01131bb7d CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
6   0x7ff0113316b5 WebKit::ChildProcessProxy::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
7   0x7ff011438b5f WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
8   0x7ff011308e2e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
9   0x7ff011308f0e CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
10  0x7ff01130911f CoreIPC::Connection::dispatchOneMessage()
11  0x7ff01131aeb7 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
12  0x7ff01131aa3c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
13  0x7ff0115413d4 WTF::Function<void ()>::operator()() const
14  0x7ff00cb7a825 WebCore::RunLoop::performWork()
15  0x7ff00d78ab1c WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)

> Tools/MiniBrowser/efl/main.c:671
> +    elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);

Does not seem related to color picker and should go somewhere else if it is really needed.

> Tools/MiniBrowser/efl/main.c:673
> +    bx = elm_box_add(window->color_selector);

bx -> box ?

> Tools/MiniBrowser/efl/main.c:679
> +    fr = elm_frame_add(window->color_selector);

fr -> frame ?

> Tools/MiniBrowser/efl/main.c:703
> +    bt = elm_button_add(window->color_selector);

bt -> ok_button ?

> Tools/MiniBrowser/efl/main.c:704
> +    elm_object_text_set(bt, "OK");

It would be nice to have a Cancel button as well.

> Tools/MiniBrowser/efl/main.c:709
> +    evas_object_smart_callback_add(bt, "clicked", on_colorpicker_ok_clicked, cp_data);

You can probably pass the window pointer as cp_data.

> Tools/MiniBrowser/efl/main.c:710
> +    evas_object_smart_callback_add(color_picker_obj, "changed", on_color_changed, cp_data);

You could simply pass the "rect" as userData instead of cp_data.

> Tools/MiniBrowser/efl/main.c:711
> +    evas_object_smart_callback_add(color_picker_obj, "color,item,selected", on_colorpalette_clicked, cp_data);

Ditto.

> Tools/MiniBrowser/efl/main.c:712
> +    evas_object_smart_callback_add(color_picker_obj, "color,item,longpressed", on_colorpalette_longpressed, cp_data);

Actually, I would completely remove handling for longpress. I don't think this is really useful here.

[Jungsik Tae]

Created attachment 202262 [details]
Patch

[Chris Dumez]

Comment on attachment 202262 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:110
> +        Ewk_Color_Picker *color_picker;

How about "ewk_picker" for clarity?

> Tools/MiniBrowser/efl/main.c:111
> +        Evas_Object *color_selector;

How about "elm_picker" for clarity?

> Tools/MiniBrowser/efl/main.c:112
> +        Evas_Object *rect_data;

I don't believe the rect_data needs to be part of the struct.

> Tools/MiniBrowser/efl/main.c:612
> +on_colorpalette_clicked(void *data, Evas_Object *obj, void *event_info)

I believe Gyuyoung had some naming suggestions that you did not take into consideration? (color_palette?)

> Tools/MiniBrowser/efl/main.c:627
> +    evas_object_color_get(window->color_selector.rect_data, &r, &g, &b, &a);

You can get the color using elm_colorselector_color_get(window->color_selector, ...) instead right?

> Tools/MiniBrowser/efl/main.c:637
> +    ewk_color_picker_color_get(window->color_selector.color_picker, &r, &g, &b, &a);

I think this shows we need a new EWK API to cancel.. Would you mind adding that API in a separate patch?

[Chris Dumez]

Also note that the crash I reported is still reproduceable with your latest patch.

I also think it would be much nicer if the OK / Cancel buttons were side by side instead of on top of each other.

[Chris Dumez]

(In reply to comment #15)
> Also note that the crash I reported is still reproduceable with your latest patch.
> 
> I also think it would be much nicer if the OK / Cancel buttons were side by side instead of on top of each other.

To fix the crash, my guess is that you need to handle the "delete,request" smart callback on your new elm window and cancel color picking.

[Jungsik Tae]

(In reply to comment #14)
> (From update of attachment 202262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202262&action=review

> > Tools/MiniBrowser/efl/main.c:112
> > +        Evas_Object *rect_data;
> 
> I don't believe the rect_data needs to be part of the struct.
> 

When we click the ok_button, we have two choice passing data on your advice.
 1. window
 2. rect
The color value is stored in rect because we pass the rect to change the color.
So, in first case, we don't get the color from the rect.
And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
What do you think about it?


> 
> I think this shows we need a new EWK API to cancel.. Would you mind adding that API in a separate patch?

I'll think about it after solving this bug.

[Jungsik Tae]

> 
> When we click the ok_button, we have two choice passing data on your advice.
>  1. window
>  2. rect
> The color value is stored in rect because we pass the rect to change the color.
> So, in first case, we don't get the color from the rect.
> And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
> What do you think about it?
> 

Oh, I didn't think of that. I think It would be solved if I pass the window in callback func of on_color_changed().

[Jungsik Tae]

i'm sorry it is incorrect upper comment #18.

[Jungsik Tae]

Created attachment 202375 [details]
Patch

[Jungsik Tae]

I'm sorry about replying now.
 Jinwoo Song, I heard that the EFL does not support modal popup.

 Gyuyoung Kim, I tried to implement the color picker without frame but i looked at some problem that were color afterimage during translocating color bar and does not take colorpicker's position.
 So, I thought the EFL is unstable and I decided using the frame.

[Jungsik Tae]

Comment on attachment 202375 [details]
Patch

a

[Jungsik Tae]

Comment on attachment 202375 [details]
Patch

This patch has still rect_data.

[Jungsik Tae]

Created attachment 202376 [details]
color picker screen shot.

[Gyuyoung Kim]

Comment on attachment 202375 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:622
> +on_colorpicker_ok_clicked(void *data, Evas_Object *obj, void *event_info)

It seems to me *color_picker* is better than colorpicker in EFL side.

> Tools/MiniBrowser/efl/main.c:626
> +    Browser_Window* window = (Browser_Window *)data;

Wrong * place.

> Tools/MiniBrowser/efl/main.c:636
> +    Browser_Window* window = (Browser_Window *)data;

ditto

> Tools/MiniBrowser/efl/main.c:662
> +

It would be good if you add comments for each UI component as on_fullscreen_enter().

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #14)
> > (From update of attachment 202262 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202262&action=review
> 
> > > Tools/MiniBrowser/efl/main.c:112
> > > +        Evas_Object *rect_data;
> > 
> > I don't believe the rect_data needs to be part of the struct.
> > 
> 
> When we click the ok_button, we have two choice passing data on your advice.
>  1. window
>  2. rect
> The color value is stored in rect because we pass the rect to change the color.
> So, in first case, we don't get the color from the rect.
> And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
> What do you think about it?

I already proposed (several times) that we pass the window and get the color from the elm_selector instead of the rect. If this is not possible, please explain why.

[Jungsik Tae]

(In reply to comment #26)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > (From update of attachment 202262 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=202262&action=review
> > 
> > > > Tools/MiniBrowser/efl/main.c:112
> > > > +        Evas_Object *rect_data;
> > > 
> > > I don't believe the rect_data needs to be part of the struct.
> > > 
> > 
> > When we click the ok_button, we have two choice passing data on your advice.
> >  1. window
> >  2. rect
> > The color value is stored in rect because we pass the rect to change the color.
> > So, in first case, we don't get the color from the rect.
> > And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
> > What do you think about it?
> 
> I already proposed (several times) that we pass the window and get the color from the elm_selector instead of the rect. If this is not possible, please explain why.

I get it now. I mean it includes rect view for showing color.
If there is not rect view it is possible.
But I think the color picker should have view of color.
I wanna hear your opinion.

[Chris Dumez]

Comment on attachment 202262 [details]
Patch

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

>>>>> Tools/MiniBrowser/efl/main.c:112
>>>>> +        Evas_Object *rect_data;
>>>> 
>>>> I don't believe the rect_data needs to be part of the struct.
>>> 
>>> When we click the ok_button, we have two choice passing data on your advice.
>>>  1. window
>>>  2. rect
>>> The color value is stored in rect because we pass the rect to change the color.
>>> So, in first case, we don't get the color from the rect.
>>> And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
>>> What do you think about it?
>> 
>> I already proposed (several times) that we pass the window and get the color from the elm_selector instead of the rect. If this is not possible, please explain why.
> 
> I get it now. I mean it includes rect view for showing color.
> If there is not rect view it is possible.
> But I think the color picker should have view of color.
> I wanna hear your opinion.

Yes, of course, keeping the rect in the color picker window to visualize the color is fine. My point is merely that rect_data does not need to be part of the color_selector struct because you don't need to use it in the on_colorpicker_ok_clicked callback.

[Jungsik Tae]

(In reply to comment #28)
> (From update of attachment 202262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202262&action=review
> 
> >>>>> Tools/MiniBrowser/efl/main.c:112
> >>>>> +        Evas_Object *rect_data;
> >>>> 
> >>>> I don't believe the rect_data needs to be part of the struct.
> >>> 
> >>> When we click the ok_button, we have two choice passing data on your advice.
> >>>  1. window
> >>>  2. rect
> >>> The color value is stored in rect because we pass the rect to change the color.
> >>> So, in first case, we don't get the color from the rect.
> >>> And in second case, it is possible we get the color, but we don't set the color to window->color_selector.elm_selector.
> >>> What do you think about it?
> >> 
> >> I already proposed (several times) that we pass the window and get the color from the elm_selector instead of the rect. If this is not possible, please explain why.
> > 
> > I get it now. I mean it includes rect view for showing color.
> > If there is not rect view it is possible.
> > But I think the color picker should have view of color.
> > I wanna hear your opinion.
> 
> Yes, of course, keeping the rect in the color picker window to visualize the color is fine. My point is merely that rect_data does not need to be part of the color_selector struct because you don't need to use it in the on_colorpicker_ok_clicked callback.

I understand it.
I use the rect_data to visualize the color. A changed color is stored in rect_data since I delivery the rect in on_color_changed() and on_color_palette_clicked(). As a result, I need to get a changed color from rect_data.
But If you wanna delete the rect_data, I will.

[Jungsik Tae]

If I delete the rect_data, a rect view would to be removed.

[Chris Dumez]

Comment on attachment 202375 [details]
Patch

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

You say you understand but I don't think you do (because the rect would stay in the new window, just be removed from the struct), here is what I mean:

> Tools/MiniBrowser/efl/main.c:112
> +        Evas_Object *rect_data;

Remove this struct member.

> Tools/MiniBrowser/efl/main.c:627
> +    evas_object_color_get(window->color_selector.rect_data, &r, &g, &b, &a);

Replace with: elm_colorselector_color_get(window->color_selector.elm_picker, &r, &g, &b, &a);

> Tools/MiniBrowser/efl/main.c:680
> +    window->color_selector.rect_data = rect;

Remove this assignment.

[Jungsik Tae]

Created attachment 202488 [details]
Patch

[Jungsik Tae]

I fixed it.
 But, I didn't implement that the rect view was changed whenever a color bar was translocated since I didn't know to access the rect through window.
 In on_color_changed(), I used 
elm_colorselector_color_set(window->color_selector.elm_selector, r, g, b, a) to change the rect (remove it this patch). And then I encountered 
 ERR<14891>:elementary elm_widget.c:3563 elm_widget_type_check() Passing Object: 0x1d0a640 in function: elm_colorselector_color_set, of type: 'elm_win' when expecting type: 'elm_colorselector'

It is normally operated except this.

[Jungsik Tae]

(In reply to comment #25)
> (From update of attachment 202375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202375&action=review
> 
> > Tools/MiniBrowser/efl/main.c:622
> > +on_colorpicker_ok_clicked(void *data, Evas_Object *obj, void *event_info)
> 
> It seems to me *color_picker* is better than colorpicker in EFL side.
> 
Modify *colorpicker* in every function name relate to color picker. 

> > Tools/MiniBrowser/efl/main.c:662
> > +
> 
> It would be good if you add comments for each UI component as on_fullscreen_enter().

I just simply add comments. Is it correct?

[Chris Dumez]

Comment on attachment 202488 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:111
> +        Evas_Object *elm_selector;

inconsistent naming, why not call it elm_picker?

> Tools/MiniBrowser/efl/main.c:608
> +    evas_object_color_set(window->color_selector.elm_selector, r, g, b, a);

You're supposed to set the rect color here, not the elm_selector's.

> Tools/MiniBrowser/efl/main.c:619
> +    evas_object_color_set(window->color_selector.elm_selector, r, g, b, a);

ditto.

> Tools/MiniBrowser/efl/main.c:716
> +    evas_object_smart_callback_add(color_picker_obj, "changed", on_color_changed, window);

You're supposed to pass rect here, not window.

> Tools/MiniBrowser/efl/main.c:717
> +    evas_object_smart_callback_add(color_picker_obj, "color,item,selected", on_color_palette_clicked, window);

Ditto.

[Jungsik Tae]

(In reply to comment #35)
> (From update of attachment 202488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202488&action=review
> 
> > Tools/MiniBrowser/efl/main.c:111
> > +        Evas_Object *elm_selector;
> 
> inconsistent naming, why not call it elm_picker?
> 
> > Tools/MiniBrowser/efl/main.c:608
> > +    evas_object_color_set(window->color_selector.elm_selector, r, g, b, a);
> 
> You're supposed to set the rect color here, not the elm_selector's.
> 
> > Tools/MiniBrowser/efl/main.c:619
> > +    evas_object_color_set(window->color_selector.elm_selector, r, g, b, a);
> 
> ditto.
> 
> > Tools/MiniBrowser/efl/main.c:716
> > +    evas_object_smart_callback_add(color_picker_obj, "changed", on_color_changed, window);
> 
> You're supposed to pass rect here, not window.
> 
> > Tools/MiniBrowser/efl/main.c:717
> > +    evas_object_smart_callback_add(color_picker_obj, "color,item,selected", on_color_palette_clicked, window);
> 
> Ditto.

I fixed it on your review. But, when I clicked the ok_button I have a problem.
It is a 
ASSERTION FAILED: value == sanitizeValue(value) || sanitizeValue(value).isEmpty()
/home/jstae/WebKit/Source/WebCore/html/HTMLInputElement.cpp(1093) : void WebCore::HTMLInputElement::setValueFromRenderer(const WTF::String&)
1   0x7f3cb932e9ab WebCore::HTMLInputElement::setValueFromRenderer(WTF::String const&)
2   0x7f3cb92dcec0 WebCore::ColorInputType::didChooseColor(WebCore::Color const&)
3   0x7f3cbe2e3326 WebKit::WebColorChooser::didChooseColor(WebCore::Color const&)
4   0x7f3cbe3225dc WebKit::WebPage::didChooseColor(WebCore::Color const&)
5   0x7f3cbe3ee833 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&), WebCore::Color>(CoreIPC::Arguments1<WebCore::Color> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
6   0x7f3cbe3ebb9a void CoreIPC::handleMessage<Messages::WebPage::DidChooseColor, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
7   0x7f3cbe3e6755 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
8   0x7f3cbe3234ba WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
9   0x7f3cbe0e02fd CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
10  0x7f3cbe24b081 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
11  0x7f3cbe0cdc6e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
12  0x7f3cbe0cdd4e CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
13  0x7f3cbe0cdf5f CoreIPC::Connection::dispatchOneMessage()
14  0x7f3cbe0df639 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
15  0x7f3cbe0df1be WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
16  0x7f3cbe30099e WTF::Function<void ()>::operator()() const
17  0x7f3cb973a3fd WebCore::RunLoop::performWork()
18  0x7f3cba33d2be WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
19  0x7f3cb61a88c7
20  0x7f3cb61a78e1
21  0x7f3cb61a7d57 ecore_main_loop_begin
22  0x7f3cba33d24f WebCore::RunLoop::run()
23  0x7f3cbe3abdc7 WebProcessMainEfl
24  0x40082c main
25  0x7f3cbd3c576d __libc_start_main
26  0x400729
ERR<15965>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj.

I passed the window in on_color_picker_ok_clicked() callback and I get a color by evas_object_color_get(window->color_selector.elm_picker,...).
What is wrong with me?

[Chris Dumez]

Comment on attachment 202488 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:628
> +    evas_object_color_get(window->color_selector.elm_selector, &r, &g, &b, &a);

Should use elm_colorselector_color_get(), not evas_object_color_get().

[Jungsik Tae]

(In reply to comment #37)
> (From update of attachment 202488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202488&action=review
> 
> > Tools/MiniBrowser/efl/main.c:628
> > +    evas_object_color_get(window->color_selector.elm_selector, &r, &g, &b, &a);
> 
> Should use elm_colorselector_color_get(), not evas_object_color_get().

I use elm_colorselector_color_get(window->color_selector.elm_selector,,,). But It is error.

ERR<16131>:elementary elm_widget.c:3563 elm_widget_type_check() Passing Object: 0x1765860 in function: elm_colorselector_color_get, of type: 'elm_win' when expecting type: 'elm_colorselector'
ASSERTION FAILED: value == sanitizeValue(value) || sanitizeValue(value).isEmpty()
/home/jstae/WebKit/Source/WebCore/html/HTMLInputElement.cpp(1093) : void WebCore::HTMLInputElement::setValueFromRenderer(const WTF::String&)
1   0x7f2a80b9b9ab WebCore::HTMLInputElement::setValueFromRenderer(WTF::String const&)
2   0x7f2a80b49ec0 WebCore::ColorInputType::didChooseColor(WebCore::Color const&)
3   0x7f2a85b50326 WebKit::WebColorChooser::didChooseColor(WebCore::Color const&)
4   0x7f2a85b8f5dc WebKit::WebPage::didChooseColor(WebCore::Color const&)
5   0x7f2a85c5b833 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&), WebCore::Color>(CoreIPC::Arguments1<WebCore::Color> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
6   0x7f2a85c58b9a void CoreIPC::handleMessage<Messages::WebPage::DidChooseColor, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
7   0x7f2a85c53755 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
8   0x7f2a85b904ba WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
9   0x7f2a8594d2fd CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
10  0x7f2a85ab8081 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
11  0x7f2a8593ac6e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
12  0x7f2a8593ad4e CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
13  0x7f2a8593af5f CoreIPC::Connection::dispatchOneMessage()
14  0x7f2a8594c639 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
15  0x7f2a8594c1be WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
16  0x7f2a85b6d99e WTF::Function<void ()>::operator()() const
17  0x7f2a80fa73fd WebCore::RunLoop::performWork()
18  0x7f2a81baa2be WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
19  0x7f2a7da158c7
20  0x7f2a7da148e1
21  0x7f2a7da14d57 ecore_main_loop_begin
22  0x7f2a81baa24f WebCore::RunLoop::run()
23  0x7f2a85c18dc7 WebProcessMainEfl
24  0x40082c main
25  0x7f2a84c3276d __libc_start_main
26  0x400729
ERR<16131>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj

[Chris Dumez]

Comment on attachment 202488 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:656
> +    window->color_selector.elm_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);

You're assigning an elm_win to elm_selector...

> Tools/MiniBrowser/efl/main.c:692
> +    color_picker_obj = elm_colorselector_add(window->color_selector.elm_selector);

This one should be assigned to elm_selector

[Jungsik Tae]

(In reply to comment #39)
> (From update of attachment 202488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202488&action=review
> 
> > Tools/MiniBrowser/efl/main.c:656
> > +    window->color_selector.elm_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
> 
> You're assigning an elm_win to elm_selector...
> 
> > Tools/MiniBrowser/efl/main.c:692
> > +    color_picker_obj = elm_colorselector_add(window->color_selector.elm_selector);
> 
> This one should be assigned to elm_selector

I'm really sorry I couldn't understand because of lack of ability.

When I removed the rect view (color view), I implemented it.
This patch is it. (I didn't remove the rect object yet.)
If the on_color_chagned() and on_color_palette_clicked() pass the rect of void and color_picker_obj of Evas_object, I would set the color to rect.
So, when I click the ok_button, I have to get a data of rect. (To change the color view)

The callback of ok_button pass the window(has ewk_color_picker, evas_object).
In the case, Neither elm_colorselector_color_get() or evas_object_color_get() has a error.

You said I could access the rect because window->color_selector.elm_selector(elm_picker). But I couldn't know the way of accessing the rect object through color_selector_elm_selector.

As a result, If I remove the color view (rect object), I would think there is no problem.

ps) If you have a spare time, i think you better execute the code.

[Chris Dumez]

Comment on attachment 202488 [details]
Patch

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

>>> Tools/MiniBrowser/efl/main.c:656
>>> +    window->color_selector.elm_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
>> 
>> You're assigning an elm_win to elm_selector...
> 
> I'm really sorry I couldn't understand because of lack of ability.
> 
> When I removed the rect view (color view), I implemented it.
> This patch is it. (I didn't remove the rect object yet.)
> If the on_color_chagned() and on_color_palette_clicked() pass the rect of void and color_picker_obj of Evas_object, I would set the color to rect.
> So, when I click the ok_button, I have to get a data of rect. (To change the color view)
> 
> The callback of ok_button pass the window(has ewk_color_picker, evas_object).
> In the case, Neither elm_colorselector_color_get() or evas_object_color_get() has a error.
> 
> You said I could access the rect because window->color_selector.elm_selector(elm_picker). But I couldn't know the way of accessing the rect object through color_selector_elm_selector.
> 
> As a result, If I remove the color view (rect object), I would think there is no problem.
> 
> ps) If you have a spare time, i think you better execute the code.

- Please keep the color view in the new elm window, it looks much better.
- You do *not* need to get the color from the rect in on_color_picker_ok_clicked(). You can get the color from the elm_picker using elm_colorselector_color_get(). The color of the colorselector is the *same* as the color of the rect because of your code in on_color_changed(). You take the color from the elm_picker and assign it to the rect.

[Jungsik Tae]

(In reply to comment #41)
> (From update of attachment 202488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202488&action=review
> 
> >>> Tools/MiniBrowser/efl/main.c:656
> >>> +    window->color_selector.elm_selector = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
> >> 
> >> You're assigning an elm_win to elm_selector...
> > 
> > I'm really sorry I couldn't understand because of lack of ability.
> > 
> > When I removed the rect view (color view), I implemented it.
> > This patch is it. (I didn't remove the rect object yet.)
> > If the on_color_chagned() and on_color_palette_clicked() pass the rect of void and color_picker_obj of Evas_object, I would set the color to rect.
> > So, when I click the ok_button, I have to get a data of rect. (To change the color view)
> > 
> > The callback of ok_button pass the window(has ewk_color_picker, evas_object).
> > In the case, Neither elm_colorselector_color_get() or evas_object_color_get() has a error.
> > 
> > You said I could access the rect because window->color_selector.elm_selector(elm_picker). But I couldn't know the way of accessing the rect object through color_selector_elm_selector.
> > 
> > As a result, If I remove the color view (rect object), I would think there is no problem.
> > 
> > ps) If you have a spare time, i think you better execute the code.
> 
> - Please keep the color view in the new elm window, it looks much better.
> - You do *not* need to get the color from the rect in on_color_picker_ok_clicked(). You can get the color from the elm_picker using elm_colorselector_color_get(). The color of the colorselector is the *same* as the color of the rect because of your code in on_color_changed(). You take the color from the elm_picker and assign it to the rect.


Situation (since it is comment #35):
 1. The rect is delivered to on_color_changed(). 
   -> I could change the color view (rect object) to use evas_object_color_set(rect). It I don't do this, I couldn't change the color view (rect object).
 2. The window is delivered to on_color_picker_ok_clicked().
   -> evas_object_color_get(..elm_picker)
   -> elm_colorselector_color_get(..elm_picker)
     -> Both of them occurred an error.
ERR<16446>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj
LEAK: 1 WebContext
and
ERR<17479>:elementary elm_widget.c:3563 elm_widget_type_check() Passing Object: 0x1b82390 in function: elm_colorselector_color_get, of type: 'elm_win' when expecting type: 'elm_colorselector'

In this situation, I will need the rect_data.

And, In Comment #41, elm_picker differ from colorselector (elm_colorselector_add..).


Also, I couldn't understand a Comment #39.

[Ryuan Choi]

Comment on attachment 202488 [details]
Patch

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

>>> Tools/MiniBrowser/efl/main.c:111
>>> +        Evas_Object *elm_selector;
>> 
>> inconsistent naming, why not call it elm_picker?
> 
> I fixed it on your review. But, when I clicked the ok_button I have a problem.
> It is a 
> ASSERTION FAILED: value == sanitizeValue(value) || sanitizeValue(value).isEmpty()
> /home/jstae/WebKit/Source/WebCore/html/HTMLInputElement.cpp(1093) : void WebCore::HTMLInputElement::setValueFromRenderer(const WTF::String&)
> 1   0x7f3cb932e9ab WebCore::HTMLInputElement::setValueFromRenderer(WTF::String const&)
> 2   0x7f3cb92dcec0 WebCore::ColorInputType::didChooseColor(WebCore::Color const&)
> 3   0x7f3cbe2e3326 WebKit::WebColorChooser::didChooseColor(WebCore::Color const&)
> 4   0x7f3cbe3225dc WebKit::WebPage::didChooseColor(WebCore::Color const&)
> 5   0x7f3cbe3ee833 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&), WebCore::Color>(CoreIPC::Arguments1<WebCore::Color> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
> 6   0x7f3cbe3ebb9a void CoreIPC::handleMessage<Messages::WebPage::DidChooseColor, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
> 7   0x7f3cbe3e6755 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 8   0x7f3cbe3234ba WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 9   0x7f3cbe0e02fd CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 10  0x7f3cbe24b081 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 11  0x7f3cbe0cdc6e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
> 12  0x7f3cbe0cdd4e CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
> 13  0x7f3cbe0cdf5f CoreIPC::Connection::dispatchOneMessage()
> 14  0x7f3cbe0df639 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
> 15  0x7f3cbe0df1be WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
> 16  0x7f3cbe30099e WTF::Function<void ()>::operator()() const
> 17  0x7f3cb973a3fd WebCore::RunLoop::performWork()
> 18  0x7f3cba33d2be WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
> 19  0x7f3cb61a88c7
> 20  0x7f3cb61a78e1
> 21  0x7f3cb61a7d57 ecore_main_loop_begin
> 22  0x7f3cba33d24f WebCore::RunLoop::run()
> 23  0x7f3cbe3abdc7 WebProcessMainEfl
> 24  0x40082c main
> 25  0x7f3cbd3c576d __libc_start_main
> 26  0x400729
> ERR<15965>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj.
> 
> I passed the window in on_color_picker_ok_clicked() callback and I get a color by evas_object_color_get(window->color_selector.elm_picker,...).
> What is wrong with me?

Agree with chris.

You probably keep three members.
ewk_picker to change input color.
elm_selector(bad naming.) to dismiss color picker window when we closed parent window.
elm_colorselector to follow the color which user changes.

>> Tools/MiniBrowser/efl/main.c:608
>> +    evas_object_color_set(window->color_selector.elm_selector, r, g, b, a);
> 
> You're supposed to set the rect color here, not the elm_selector's.

Like chris mentioned, you should receive evas object and use it here.

elm_selector is window, so this is wrong.

>>> Tools/MiniBrowser/efl/main.c:628
>>> +    evas_object_color_get(window->color_selector.elm_selector, &r, &g, &b, &a);
>> 
>> Should use elm_colorselector_color_get(), not evas_object_color_get().
> 
> I use elm_colorselector_color_get(window->color_selector.elm_selector,,,). But It is error.
> 
> ERR<16131>:elementary elm_widget.c:3563 elm_widget_type_check() Passing Object: 0x1765860 in function: elm_colorselector_color_get, of type: 'elm_win' when expecting type: 'elm_colorselector'
> ASSERTION FAILED: value == sanitizeValue(value) || sanitizeValue(value).isEmpty()
> /home/jstae/WebKit/Source/WebCore/html/HTMLInputElement.cpp(1093) : void WebCore::HTMLInputElement::setValueFromRenderer(const WTF::String&)
> 1   0x7f2a80b9b9ab WebCore::HTMLInputElement::setValueFromRenderer(WTF::String const&)
> 2   0x7f2a80b49ec0 WebCore::ColorInputType::didChooseColor(WebCore::Color const&)
> 3   0x7f2a85b50326 WebKit::WebColorChooser::didChooseColor(WebCore::Color const&)
> 4   0x7f2a85b8f5dc WebKit::WebPage::didChooseColor(WebCore::Color const&)
> 5   0x7f2a85c5b833 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&), WebCore::Color>(CoreIPC::Arguments1<WebCore::Color> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
> 6   0x7f2a85c58b9a void CoreIPC::handleMessage<Messages::WebPage::DidChooseColor, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
> 7   0x7f2a85c53755 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 8   0x7f2a85b904ba WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 9   0x7f2a8594d2fd CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 10  0x7f2a85ab8081 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
> 11  0x7f2a8593ac6e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
> 12  0x7f2a8593ad4e CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
> 13  0x7f2a8593af5f CoreIPC::Connection::dispatchOneMessage()
> 14  0x7f2a8594c639 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
> 15  0x7f2a8594c1be WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
> 16  0x7f2a85b6d99e WTF::Function<void ()>::operator()() const
> 17  0x7f2a80fa73fd WebCore::RunLoop::performWork()
> 18  0x7f2a81baa2be WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
> 19  0x7f2a7da158c7
> 20  0x7f2a7da148e1
> 21  0x7f2a7da14d57 ecore_main_loop_begin
> 22  0x7f2a81baa24f WebCore::RunLoop::run()
> 23  0x7f2a85c18dc7 WebProcessMainEfl
> 24  0x40082c main
> 25  0x7f2a84c3276d __libc_start_main
> 26  0x400729
> ERR<16131>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj

I agree with chris. So you'd better to add elm_colorselector in color_selector structure and call elm_colorselector_color_set(window->color_selector.elm_colorselector)

[Jungsik Tae]

Created attachment 202753 [details]
Patch

[Ryuan Choi]

Comment on attachment 202753 [details]
Patch

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

Good, quite better than before.

> Tools/MiniBrowser/efl/main.c:659
> +    window->color_selector.elm_selector_window = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);

Shouldn't we add delete,request signal for this?

> Tools/MiniBrowser/efl/main.c:667
> +    elm_win_resize_object_add(window->color_selector.elm_selector_window, box);

hint looks wrong.
Because box follow elm_selector_window, you'd better to give EVAS_HINT_EXPAND instead of 0.9

> Tools/MiniBrowser/efl/main.c:672
> +    evas_object_size_hint_align_set(rect_frame, EVAS_HINT_FILL, EVAS_HINT_FILL);

Please check these hints also.

> Tools/MiniBrowser/efl/main.c:697
> +    evas_object_size_hint_weight_set(button_box, EVAS_HINT_EXPAND, 0.1);

Ditto.
I think that min size hint looks better for here.

[Jungsik Tae]

Created attachment 202759 [details]
Patch

[Jungsik Tae]

(In reply to comment #45)
> (From update of attachment 202753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202753&action=review
> 
> Good, quite better than before.
> 
> > Tools/MiniBrowser/efl/main.c:659
> > +    window->color_selector.elm_selector_window = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
> 
> Shouldn't we add delete,request signal for this?
> 
I couldn't know what to do.
When I used win_auto_del(), I encountered error.

[Ryuan Choi]

(In reply to comment #47)
> (In reply to comment #45)
> > (From update of attachment 202753 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202753&action=review
> > 
> > Good, quite better than before.
> > 
> > > Tools/MiniBrowser/efl/main.c:659
> > > +    window->color_selector.elm_selector_window = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
> > 
> > Shouldn't we add delete,request signal for this?
> > 
> I couldn't know what to do.
> When I used win_auto_del(), I encountered error.

It's wrong comment. please ignore it.

[Ryuan Choi]

Comment on attachment 202759 [details]
Patch

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

please rebase patch, this will not be merged because of conflict.

> Tools/MiniBrowser/efl/main.c:608
> +    

Unnecessary spaces.

> Tools/MiniBrowser/efl/main.c:680
> +    evas_object_color_set(rect, r, g, b, a);

I think you should update color selector as this color.
If not, color bar will indicates initial color.

[Jungsik Tae]

(In reply to comment #49)
> (From update of attachment 202759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202759&action=review
> 

> > Tools/MiniBrowser/efl/main.c:680
> > +    evas_object_color_set(rect, r, g, b, a);
> 
> I think you should update color selector as this color.
> If not, color bar will indicates initial color.

I just change the rect for color view.
Otherwise, Whenever we execute the color picker we will see the white color rect.

[Ryuan Choi]

(In reply to comment #50)
> (In reply to comment #49)
> > (From update of attachment 202759 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202759&action=review
> > 
> 
> > > Tools/MiniBrowser/efl/main.c:680
> > > +    evas_object_color_set(rect, r, g, b, a);
> > 
> > I think you should update color selector as this color.
> > If not, color bar will indicates initial color.
> 
> I just change the rect for color view.
> Otherwise, Whenever we execute the color picker we will see the white color rect.

Please update both rect and color selector.
You only updated rect.

[Chris Dumez]

Comment on attachment 202759 [details]
Patch

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

>>>> Tools/MiniBrowser/efl/main.c:680
>>>> +    evas_object_color_set(rect, r, g, b, a);
>>> 
>>> I think you should update color selector as this color.
>>> If not, color bar will indicates initial color.
>> 
>> I just change the rect for color view.
>> Otherwise, Whenever we execute the color picker we will see the white color rect.
> 
> Please update both rect and color selector.
> You only updated rect.

+1

[Jungsik Tae]

Created attachment 202938 [details]
Patch

[Ryuan Choi]

Comment on attachment 202938 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:672
> +

Please clear the elm_selector_window to NULL.

In addition, for the exception handling, you should release elm_selector_window when parent window is closed.
If not, we may got a crash when clicked OK/Cancel button after closed parent window(which contains webview).

> Tools/MiniBrowser/efl/main.c:687
> +

IMO, you should add elm_bg.
If not, elm_selector_window may have broken background.

[Chris Dumez]

Comment on attachment 202938 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:687
>> +
> 
> IMO, you should add elm_bg.
> If not, elm_selector_window may have broken background.

Or simply use elm_win_util_standard_add() (http://docs.enlightenment.org/stable/elementary/group__Win.html#gac75d7752662fab73d6420706ce5be996) and then we don't need to set a bg or a title. Shorter code is always nice.

[Jungsik Tae]

Created attachment 203556 [details]
Patch

[Chris Dumez]

Comment on attachment 203556 [details]
Patch

the new patch does not apply cleanly nor does it address the feedback.

[Jungsik Tae]

But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
I think we have to create the color picker as window's descendent. Do you agree?(In reply to comment #55)
> (From update of attachment 202938 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202938&action=review
> 
> >> Tools/MiniBrowser/efl/main.c:687
> >> +
> > 
> > IMO, you should add elm_bg.
> > If not, elm_selector_window may have broken background.
> 
> Or simply use elm_win_util_standard_add() (http://docs.enlightenment.org/stable/elementary/group__Win.html#gac75d7752662fab73d6420706ce5be996) and then we don't need to set a bg or a title. Shorter code is always nice.

But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
I think we have to create the color picker as window's descendent. Do you agree?

[Chris Dumez]

(In reply to comment #58)
> But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
> I think we have to create the color picker as window's descendent. Do you agree?(In reply to comment #55)
> > (From update of attachment 202938 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202938&action=review
> > 
> > >> Tools/MiniBrowser/efl/main.c:687
> > >> +
> > > 
> > > IMO, you should add elm_bg.
> > > If not, elm_selector_window may have broken background.
> > 
> > Or simply use elm_win_util_standard_add() (http://docs.enlightenment.org/stable/elementary/group__Win.html#gac75d7752662fab73d6420706ce5be996) and then we don't need to set a bg or a title. Shorter code is always nice.
> 
> But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
> I think we have to create the color picker as window's descendent. Do you agree?

Then please comment *BEFORE* reuploading a new patch for review or at least explain why you did not take the feedback into consideration when uploading a new patch.

The fact is that you still need to add a bg for your elm_win. So please add it manually as Ryuan suggested since elm_win_util_standard_add() does not seem to be suitable.

Ryuan also had a comment about resetting a pointer to NULL which I don't think you addressed, right.

[Jungsik Tae]

(In reply to comment #59)
> (In reply to comment #58)
> > But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
> > I think we have to create the color picker as window's descendent. Do you agree?(In reply to comment #55)
> > > (From update of attachment 202938 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=202938&action=review
> > > 
> > > >> Tools/MiniBrowser/efl/main.c:687
> > > >> +
> > > > 
> > > > IMO, you should add elm_bg.
> > > > If not, elm_selector_window may have broken background.
> > > 
> > > Or simply use elm_win_util_standard_add() (http://docs.enlightenment.org/stable/elementary/group__Win.html#gac75d7752662fab73d6420706ce5be996) and then we don't need to set a bg or a title. Shorter code is always nice.
> > 
> > But, I looked at the code about elm_win_util_standard_add() and It uses elm_wind_add(NULL....) in the code.
> > I think we have to create the color picker as window's descendent. Do you agree?
> 
> Then please comment *BEFORE* reuploading a new patch for review or at least explain why you did not take the feedback into consideration when uploading a new patch.
> 
> The fact is that you still need to add a bg for your elm_win. So please add it manually as Ryuan suggested since elm_win_util_standard_add() does not seem to be suitable.
> 
> Ryuan also had a comment about resetting a pointer to NULL which I don't think you addressed, right.

I'm so sorry. I didn't see your comments.
So, That patch is modified your comments.
I added bg. 
And As Ryuan said, I solved the crash problem. (line 275)

[Chris Dumez]

Comment on attachment 203556 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:650
> +    evas_object_del(window->color_selector.elm_selector_window);

As ryuan said, you should probably set window->color_selector.elm_selector_window to NULL after this call.

[Jungsik Tae]

Created attachment 203634 [details]
Patch

[Jungsik Tae]

(In reply to comment #61)
> (From update of attachment 203556 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203556&action=review
> 
> > Tools/MiniBrowser/efl/main.c:650
> > +    evas_object_del(window->color_selector.elm_selector_window);
> 
> As ryuan said, you should probably set window->color_selector.elm_selector_window to NULL after this call.

I fixed it.

[Chris Dumez]

(In reply to comment #63)
> (In reply to comment #61)
> > (From update of attachment 203556 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=203556&action=review
> > 
> > > Tools/MiniBrowser/efl/main.c:650
> > > +    evas_object_del(window->color_selector.elm_selector_window);
> > 
> > As ryuan said, you should probably set window->color_selector.elm_selector_window to NULL after this call.
> 
> I fixed it.

The patch needs rebasing.

[Jungsik Tae]

Created attachment 203669 [details]
Patch

[Jungsik Tae]

(In reply to comment #65)
> Created an attachment (id=203669) [details]
> Patch

I rebased it.

[Chris Dumez]

(In reply to comment #66)
> (In reply to comment #65)
> > Created an attachment (id=203669) [details] [details]
> > Patch
> 
> I rebased it.

The window is not modal. This is an issue because I can click the "submit" button while the color picker window is still showing and I get the following crash:
ASSERTION FAILED: m_colorChooser
/home/chris/devel/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp(2995) : void WebKit::WebPageProxy::endColorChooser()
1   0x7f5b4948faaf WTFCrash
2   0x7f5b49255d7e WebKit::WebPageProxy::endColorChooser()
3   0x7f5b4946816e void CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::Arguments0 const&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
4   0x7f5b49464552 void CoreIPC::handleMessage<Messages::WebPageProxy::EndColorChooser, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::MessageDecoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
5   0x7f5b4945e140 WebKit::WebPageProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
6   0x7f5b4917f0ba CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
7   0x7f5b491940ed WebKit::ChildProcessProxy::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
8   0x7f5b492928dd WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
9   0x7f5b4916d2c8 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
10  0x7f5b4916d3a8 CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
11  0x7f5b4916d5b9 CoreIPC::Connection::dispatchOneMessage()
12  0x7f5b4917e499 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
13  0x7f5b4917e01e WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
14  0x7f5b49399429 WTF::Function<void ()>::operator()() const
15  0x7f5b44a53a2c WebCore::RunLoop::performWork()
16  0x7f5b455c8150 WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
17  0x7f5b49b1fa07
18  0x7f5b49b1ea21
19  0x7f5b49b1ee97 ecore_main_loop_begin
20  0x40a6be elm_main
21  0x40a708 main
22  0x7f5b48459ea5 __libc_start_main
23  0x4053a9
Segmentation fault (core dumped)

[Chris Dumez]

Comment on attachment 203669 [details]
Patch

r- until the crash is addressed.

[Ryuan Choi]

(In reply to comment #68)
> (From update of attachment 203669 [details])
> r- until the crash is addressed.

IMO, it looks WebCore or WebKit2 issue.

So I added bug to fix the crash.

Anyway, I agree with you that we'd better to fix the crash before landing this.

[Chris Dumez]

(In reply to comment #69)
> (In reply to comment #68)
> > (From update of attachment 203669 [details] [details])
> > r- until the crash is addressed.
> 
> IMO, it looks WebCore or WebKit2 issue.
> 
> So I added bug to fix the crash.
> 
> Anyway, I agree with you that we'd better to fix the crash before landing this.

I have just checked with Chromium. The color picker dialog is not modal, however, if I click the submit button while the picker is showing, there is no problem.

I believe you are right that this is a WK2 issue. This should definitely be fixed in another patch.

We can fix the crash before or after this patch. I don't have a strong opinion on this anymore.

[Chris Dumez]

Comment on attachment 203669 [details]
Patch

Patch looks OK to me now. Ryuan, do you have any remaining comment? If not, I would be tempted to land the patch since the crash issue is handled separately and not strictly related.

[Ryuan Choi]

Comment on attachment 203669 [details]
Patch

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

> Tools/ChangeLog:16
> +        (on_color_item_selected): Change color of rectangle with clicking color boxes basically offered by elemnet API.

Typo. s/elemnet/elementary

BTW,

> Tools/MiniBrowser/efl/main.c:692
> +    Browser_Window *window = window_find_with_ewk_view(sd->self);
> +    window->color_selector.elm_selector_window = elm_win_add(window->elm_window, "color selector", ELM_WIN_BASIC);
> +    window->color_selector.ewk_picker = color_picker;
> +
> +    elm_win_title_set(window->color_selector.elm_selector_window, "Color selector");
> +
> +    /* Show color view */

This patch is still missing elm_bg.(I fixed same issues in file dialog at Bug 117189)

Others looks fine to me.

[Chris Dumez]

Comment on attachment 203669 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:692
>> +    /* Show color view */
> 
> This patch is still missing elm_bg.(I fixed same issues in file dialog at Bug 117189)
> 
> Others looks fine to me.

Damn, it was there 3 patches ago. Not sure why Jungsik silently removed it.

[Jungsik Tae]

(In reply to comment #73)
> (From update of attachment 203669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203669&action=review
> 
> >> Tools/MiniBrowser/efl/main.c:692
> >> +    /* Show color view */
> > 
> > This patch is still missing elm_bg.(I fixed same issues in file dialog at Bug 117189)
> > 
> > Others looks fine to me.
> 
> Damn, it was there 3 patches ago. Not sure why Jungsik silently removed it.

 I had a problem about uploading patch yesterday since there was a conflict while rebasing. So I copied a backup data without checking by my mistake.
I will upload again. I am sorry to bother you.

[Jungsik Tae]

Created attachment 203979 [details]
Patch

[Ryuan Choi]

Comment on attachment 203979 [details]
Patch

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

Without typos, looks good to me.

> Tools/ChangeLog:16
> +        (on_color_item_selected): Change color of rectangle with clicking color boxes basically offered by element API.

s/element/elementary

Anyway, below looks better to me.
"Change color of rectangle when clicked color palettes offered by elm_color_chooser."

[Jungsik Tae]

Created attachment 203985 [details]
Patch

[Chris Dumez]

Comment on attachment 203985 [details]
Patch

Ok, r=me.

[Jungsik Tae]

(In reply to comment #78)
> (From update of attachment 203985 [details])
> Ok, r=me.

Thank you for your review.

[Chris Dumez]

Ryuan, can you please take one last look and cq+?

[Ryuan Choi]

Comment on attachment 203985 [details]
Patch

Looks fine to me, too.

cq+ed.

[WebKit Commit Bot]

Comment on attachment 203985 [details]
Patch

Clearing flags on attachment: 203985

Committed r151308: <http://trac.webkit.org/changeset/151308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.