[Webkit-unassigned] [Bug 115890] [EFL] Implement colorpicker for HTML5 input type color on Minibrowser

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115890


Christophe Dumez <dchris at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #201680|review?                     |review-
               Flag|                            |




--- Comment #12 from Christophe Dumez <dchris at gmail.com>  2013-05-16 11:57:05 PST ---
(From update of attachment 201680)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.