Bug 115890 - [EFL] Implement colorpicker for HTML5 input type color on Minibrowser
Summary: [EFL] Implement colorpicker for HTML5 input type color on Minibrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jungsik Tae
URL:
Keywords:
Depends on: 117237
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-09 22:14 PDT by Jungsik Tae
Modified: 2013-06-07 00:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (7.59 KB, patch)
2013-05-12 17:08 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2013-05-13 19:29 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2013-05-13 20:30 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2013-05-13 20:47 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.39 KB, patch)
2013-05-13 22:50 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.40 KB, patch)
2013-05-19 23:54 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2013-05-20 20:54 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
color picker screen shot. (74.29 KB, image/png)
2013-05-20 21:20 PDT, Jungsik Tae
no flags Details
Patch (7.82 KB, patch)
2013-05-21 18:50 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.94 KB, patch)
2013-05-23 17:01 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2013-05-23 20:57 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2013-05-26 17:31 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2013-06-03 00:00 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2013-06-03 17:19 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2013-06-04 03:01 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2013-06-06 17:13 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2013-06-06 18:01 PDT, Jungsik Tae
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungsik Tae 2013-05-09 22:14:41 PDT
Colorpicker implemnetation by using existent colorpicker API.
Comment 1 Jungsik Tae 2013-05-12 17:08:07 PDT
Created attachment 201512 [details]
Patch
Comment 2 Jinwoo Song 2013-05-12 18:09:26 PDT
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.
Comment 3 Jungsik Tae 2013-05-13 19:29:50 PDT
Created attachment 201667 [details]
Patch
Comment 4 EFL EWS Bot 2013-05-13 19:34:05 PDT
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
Comment 5 Jungsik Tae 2013-05-13 20:30:28 PDT
Created attachment 201671 [details]
Patch
Comment 6 EFL EWS Bot 2013-05-13 20:35:31 PDT
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
Comment 7 Jungsik Tae 2013-05-13 20:47:19 PDT
Created attachment 201672 [details]
Patch
Comment 8 EFL EWS Bot 2013-05-13 20:50:00 PDT
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
Comment 9 Gyuyoung Kim 2013-05-13 20:50:24 PDT
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.
Comment 10 Jungsik Tae 2013-05-13 22:50:44 PDT
Created attachment 201680 [details]
Patch
Comment 11 Gyuyoung Kim 2013-05-13 23:02:16 PDT
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.
Comment 12 Chris Dumez 2013-05-16 11:57:05 PDT
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.
Comment 13 Jungsik Tae 2013-05-19 23:54:12 PDT
Created attachment 202262 [details]
Patch
Comment 14 Chris Dumez 2013-05-20 00:05:51 PDT
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?
Comment 15 Chris Dumez 2013-05-20 00:57:29 PDT
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.
Comment 16 Chris Dumez 2013-05-20 01:07:45 PDT
(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.
Comment 17 Jungsik Tae 2013-05-20 17:15:38 PDT
(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.
Comment 18 Jungsik Tae 2013-05-20 18:49:24 PDT
> 
> 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().
Comment 19 Jungsik Tae 2013-05-20 18:56:26 PDT
i'm sorry it is incorrect upper comment #18.
Comment 20 Jungsik Tae 2013-05-20 20:54:13 PDT
Created attachment 202375 [details]
Patch
Comment 21 Jungsik Tae 2013-05-20 20:59:38 PDT
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.
Comment 22 Jungsik Tae 2013-05-20 21:00:50 PDT
Comment on attachment 202375 [details]
Patch

a
Comment 23 Jungsik Tae 2013-05-20 21:01:32 PDT
Comment on attachment 202375 [details]
Patch

This patch has still rect_data.
Comment 24 Jungsik Tae 2013-05-20 21:20:12 PDT
Created attachment 202376 [details]
color picker screen shot.
Comment 25 Gyuyoung Kim 2013-05-20 21:40:12 PDT
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().
Comment 26 Chris Dumez 2013-05-20 22:53:06 PDT
(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.
Comment 27 Jungsik Tae 2013-05-21 01:30:17 PDT
(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.
Comment 28 Chris Dumez 2013-05-21 02:05:40 PDT
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.
Comment 29 Jungsik Tae 2013-05-21 03:58:47 PDT
(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.
Comment 30 Jungsik Tae 2013-05-21 04:04:59 PDT
If I delete the rect_data, a rect view would to be removed.
Comment 31 Chris Dumez 2013-05-21 04:13:32 PDT
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.
Comment 32 Jungsik Tae 2013-05-21 18:50:55 PDT
Created attachment 202488 [details]
Patch
Comment 33 Jungsik Tae 2013-05-21 18:55:09 PDT
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.
Comment 34 Jungsik Tae 2013-05-21 19:00:41 PDT
(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?
Comment 35 Chris Dumez 2013-05-21 22:51:40 PDT
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.
Comment 36 Jungsik Tae 2013-05-21 23:27:12 PDT
(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?
Comment 37 Chris Dumez 2013-05-21 23:41:35 PDT
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().
Comment 38 Jungsik Tae 2013-05-21 23:58:05 PDT
(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
Comment 39 Chris Dumez 2013-05-22 00:10:52 PDT
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
Comment 40 Jungsik Tae 2013-05-22 02:28:18 PDT
(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.
Comment 41 Chris Dumez 2013-05-22 02:39:45 PDT
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.
Comment 42 Jungsik Tae 2013-05-22 19:18:53 PDT
(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.
Comment 43 Ryuan Choi 2013-05-22 21:44:39 PDT
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)
Comment 44 Jungsik Tae 2013-05-23 17:01:26 PDT
Created attachment 202753 [details]
Patch
Comment 45 Ryuan Choi 2013-05-23 18:47:06 PDT
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.
Comment 46 Jungsik Tae 2013-05-23 20:57:00 PDT
Created attachment 202759 [details]
Patch
Comment 47 Jungsik Tae 2013-05-23 21:00:05 PDT
(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.
Comment 48 Ryuan Choi 2013-05-23 22:20:49 PDT
(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.
Comment 49 Ryuan Choi 2013-05-23 22:44:44 PDT
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.
Comment 50 Jungsik Tae 2013-05-23 23:26:46 PDT
(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.
Comment 51 Ryuan Choi 2013-05-23 23:32:01 PDT
(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.
Comment 52 Chris Dumez 2013-05-23 23:34:37 PDT
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
Comment 53 Jungsik Tae 2013-05-26 17:31:27 PDT
Created attachment 202938 [details]
Patch
Comment 54 Ryuan Choi 2013-05-31 00:42:22 PDT
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.
Comment 55 Chris Dumez 2013-06-02 23:15:09 PDT
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.
Comment 56 Jungsik Tae 2013-06-03 00:00:04 PDT
Created attachment 203556 [details]
Patch
Comment 57 Chris Dumez 2013-06-03 00:03:10 PDT
Comment on attachment 203556 [details]
Patch

the new patch does not apply cleanly nor does it address the feedback.
Comment 58 Jungsik Tae 2013-06-03 00:15:07 PDT
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?
Comment 59 Chris Dumez 2013-06-03 00:24:18 PDT
(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.
Comment 60 Jungsik Tae 2013-06-03 00:52:40 PDT
(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)
Comment 61 Chris Dumez 2013-06-03 01:18:48 PDT
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.
Comment 62 Jungsik Tae 2013-06-03 17:19:46 PDT
Created attachment 203634 [details]
Patch
Comment 63 Jungsik Tae 2013-06-03 17:20:16 PDT
(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.
Comment 64 Chris Dumez 2013-06-03 23:06:02 PDT
(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.
Comment 65 Jungsik Tae 2013-06-04 03:01:50 PDT
Created attachment 203669 [details]
Patch
Comment 66 Jungsik Tae 2013-06-04 03:28:28 PDT
(In reply to comment #65)
> Created an attachment (id=203669) [details]
> Patch

I rebased it.
Comment 67 Chris Dumez 2013-06-04 03:39:04 PDT
(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)
Comment 68 Chris Dumez 2013-06-04 03:41:25 PDT
Comment on attachment 203669 [details]
Patch

r- until the crash is addressed.
Comment 69 Ryuan Choi 2013-06-05 02:53:30 PDT
(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.
Comment 70 Chris Dumez 2013-06-05 03:17:29 PDT
(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.
Comment 71 Chris Dumez 2013-06-05 03:25:28 PDT
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.
Comment 72 Ryuan Choi 2013-06-05 03:42:18 PDT
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.
Comment 73 Chris Dumez 2013-06-05 03:46:01 PDT
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.
Comment 74 Jungsik Tae 2013-06-05 04:21:05 PDT
(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.
Comment 75 Jungsik Tae 2013-06-06 17:13:15 PDT
Created attachment 203979 [details]
Patch
Comment 76 Ryuan Choi 2013-06-06 17:50:45 PDT
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."
Comment 77 Jungsik Tae 2013-06-06 18:01:14 PDT
Created attachment 203985 [details]
Patch
Comment 78 Chris Dumez 2013-06-06 23:06:33 PDT
Comment on attachment 203985 [details]
Patch

Ok, r=me.
Comment 79 Jungsik Tae 2013-06-06 23:21:56 PDT
(In reply to comment #78)
> (From update of attachment 203985 [details])
> Ok, r=me.

Thank you for your review.
Comment 80 Chris Dumez 2013-06-06 23:30:52 PDT
Ryuan, can you please take one last look and cq+?
Comment 81 Ryuan Choi 2013-06-07 00:29:19 PDT
Comment on attachment 203985 [details]
Patch

Looks fine to me, too.

cq+ed.
Comment 82 WebKit Commit Bot 2013-06-07 00:51:26 PDT
Comment on attachment 203985 [details]
Patch

Clearing flags on attachment: 203985

Committed r151308: <http://trac.webkit.org/changeset/151308>
Comment 83 WebKit Commit Bot 2013-06-07 00:51:33 PDT
All reviewed patches have been landed.  Closing bug.