[webkit-reviews] review denied: [Bug 115890] [EFL] Implement colorpicker for HTML5 input type color on Minibrowser : [Attachment 202262] Patch

[bugzilla-daemon at webkit.org]

Christophe Dumez <dchris at gmail.com> has denied Jungsik Tae
<davidtlay88 at gmail.com>'s request for review:
Bug 115890: [EFL] Implement colorpicker for HTML5 input type color on
Minibrowser
https://bugs.webkit.org/show_bug.cgi?id=115890

Attachment 202262: Patch
https://bugs.webkit.org/attachment.cgi?id=202262&action=review

------- Additional Comments from Christophe Dumez <dchris at gmail.com>
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?