Bug 91832 – [EFL][WK2] Implemented color picker API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

Implement color chooser proxy for EFL. The ewk API related color chooser should be implemented after this.

[KwangYong Choi]

Created attachment 153448 [details]
Patch

[Ryuan Choi]

Comment on attachment 153448 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.cpp:37
> +    endChooser(); // FIXME: Remove this. Call the ewk API to show color chooser after implemented.

If I am right, this patch looks not complete to support color chooser.

Why don't you implement all instead of temporal blocking?

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:60
> +#endif // ENABLE(INPUT_TYPE_COLOR)
> +

unnecessary empty line.

[KwangYong Choi]

Created attachment 154248 [details]
Patch

1. Additional implementation on ewk_view. But UI is missing. InputPicker UI will be contributed by someone else.

2. An empty line is also exist on other files. (WebColorChooserProxy.h, WebColorChooser.h, ...) So, I didn't remove the empty line.

[Gyuyoung Kim]

Comment on attachment 154248 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/InputPicker.cpp:4
> + * Redistribution and use in source and binary forms, with or without

I'm checking which license we use. So, I don't know which license is best for us.

> Source/WebKit2/UIProcess/API/efl/InputPicker.h:37
> +    InputPicker(Evas_Object*);

Use *explicit* keyword.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:87
> +typedef enum _Ewk_Input_Type Ewk_Input_Type;

Please add below comment for above type definition.

/** Creates a type name for _Ewk_Input_Type_Ewk_Input_Type */

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:472
> +EAPI void ewk_view_color_chooser_color_set(Evas_Object* o, const char* color);

Nit: Move '*' to variable side.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:479
> +EAPI void ewk_view_color_chooser_close(Evas_Object* o);

ditto.

[KwangYong Choi]

Created attachment 154272 [details]
Patch

1. Added explicit keyword.

2. Added comment for type definition.

3. Moved '*' to variable side.

I just applied BSD license most files in webkit uses. It can be changed to another one if it's required.

[Chris Dumez]

Comment on attachment 154272 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:301
> +    priv->inputPicker = adoptPtr<InputPicker>(new InputPicker(smartData->self));

This should be moved to the Ewk_View_Private_Data constructor.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:308
> +    priv->inputPicker = nullptr;

Useless, please remove.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1049
> +    if (priv->colorChooser)

You should return early. if (!priv->colorChooser) return;

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:108
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type inputType, const char* inputValue);

star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:469
> + *

We should document the format of the color argument? Hexadecimal only?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:473
> +EAPI void ewk_view_color_chooser_color_set(Evas_Object *o, const char *color);

You can set but not get the color?
Also, we usually return Eina_Bool for setters.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:480
> +EAPI void ewk_view_color_chooser_close(Evas_Object *o);

We usually return Eina_Bool.

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:46
> +    ~WebColorChooserProxyEfl();

You have virtual methods, the destructor needs to be virtual.

[Gyuyoung Kim]

Comment on attachment 154272 [details]
Patch

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

Could you explain how to implement color picker by application based on this new APIs ?

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Implement color chooser interface for EFL

I think this patch implements input picker as well. So, I think it is better to mention input picker as well.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:73
> +    OwnPtr<InputPicker> inputPicker;

I think below location is proper place for this variable definition.

http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp#L55

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:308
> +    priv->inputPicker = nullptr;

You don't need to delete data based on OwnPtr. OwnPtr calls "delete" on that pointer when it goes out of scope.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:108
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type inputType, const char* inputValue);

I'm sorry I missed this previous review.

s/inputType/input_type/g and s/inputValue/input_value/g

Move '*' to variable side.

[KwangYong Choi]

Created attachment 154305 [details]
Patch

1. Changed bug title.

2. Moved member variables to proper position.

3. Removed priv->inputPicker = nullptr;

4. Moved "*" to proper position and changed variable names.

5. Moved allocation of InputPicker routine to Ewk_View_Private_Data constructor.

6. Made early return.

7. ewk_view_color_chooser_color_set() function accepts all color formats webkit can recognize. For example, named color can be used. So, I removed color format example.

8. The initial value is passed when the InputPicker::show() is called. After that, UI can change values and sets the value user choose. I think the function to get the value is not necessary right now.

9. Added Eina_Bool return type for external api functions.

[KwangYong Choi]

Created attachment 154308 [details]
Patch

Moved "*" to proper position in ewk_view.h which follows efl coding rules.

[KwangYong Choi]

Created attachment 154310 [details]
Patch

Rebased.

[Ryuan Choi]

Comment on attachment 154310 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/InputPicker.h:31
> +#include "ewk_view.h"
> +
> +typedef struct _Evas_Object Evas_Object;

ewk_view.h already have Evas.h. so typedef is not needed.

> Source/WebKit2/UIProcess/API/efl/InputPicker.h:35
> +class InputPicker {

I don't have idea how to implement input picker because we can not use elementary and edj approach is too expensive.

So,  exposing interface as member of smart class looks enough to me.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1146
> +    return EINA_TRUE;
> +#endif // ENABLE(INPUT_TYPE_COLOR)
> +    return EINA_FALSE;

macro is wrong.

And remove // ...

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1163
> +    return EINA_TRUE;
> +#endif // ENABLE(INPUT_TYPE_COLOR)
> +    return EINA_FALSE;

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:75
> +    EWK_INPUT_TYPE_TELEPHONE = 1, 

I am not sure. but webkit does not use = 1 for enum.

please check whether I am right.

[KwangYong Choi]

> Could you explain how to implement color picker by application based on this new APIs ?

There are two ways using input picker interface:

1. Internal implementation

UI implementation in internal webkit will be used by default. The start of implementation location is InputPicker::show().

2. External application implementation

The application can use their own view_add function instead of ewk_view_add(). And it can set the smart class its own using ewk_view_smart_class_set().

Eina_Bool browser_view_smart_class_set(Ewk_View_Smart_Class* api)
{
    if (!ewk_view_smart_class_set(api))
        return false;

    api->input_picker_show = _browser_input_picker_show; // override

    return true;
}

The overrided function will be called when the input picker is required.

[KwangYong Choi]

Comment on attachment 154310 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/InputPicker.h:35
>> +class InputPicker {
> 
> I don't have idea how to implement input picker because we can not use elementary and edj approach is too expensive.
> 
> So,  exposing interface as member of smart class looks enough to me.

I agree with you. I'll remove this.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1146
>> +    return EINA_FALSE;
> 
> macro is wrong.
> 
> And remove // ...

Can you explain why it's wrong?

And "//" is used in many places.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:75
>> +    EWK_INPUT_TYPE_TELEPHONE = 1, 
> 
> I am not sure. but webkit does not use = 1 for enum.
> 
> please check whether I am right.

It can be used, I think.

enum AccessibilityRole {
    AnnotationRole = 1,
    ApplicationRole,
    ApplicationAlertRole,

[Ryuan Choi]

(In reply to comment #13)
> (From update of attachment 154310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154310&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/InputPicker.h:35
> >> +class InputPicker {
> > 
> > I don't have idea how to implement input picker because we can not use elementary and edj approach is too expensive.
> > 
> > So,  exposing interface as member of smart class looks enough to me.
> 
> I agree with you. I'll remove this.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1146
> >> +    return EINA_FALSE;
> > 
> > macro is wrong.
> > 
> > And remove // ...
> 
> Can you explain why it's wrong?

Sure, when enabled INPUT_TYPE_COLOR, this function have two return statement.
Although compile ignore last one, he probably complain it as warning.

> 
> And "//" is used in many places.
I don't know whether we have rules for this.
But, I believe that "// ENABLE(XXX)" is used to reduce confusion from big fence of macro or overlapped macro.

IMO, this is not necessary and bad with readibility for the small guard.

> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:75
> >> +    EWK_INPUT_TYPE_TELEPHONE = 1, 
> > 
> > I am not sure. but webkit does not use = 1 for enum.
> > 
> > please check whether I am right.
> 
> It can be used, I think.
> 
> enum AccessibilityRole {
>     AnnotationRole = 1,
>     ApplicationRole,
>     ApplicationAlertRole,

OK if then, I don't have objection for this.

[KwangYong Choi]

Created attachment 154474 [details]
Patch

1. Removed InputPicker class for internal implementation.

2. Used #else for macro block

3. Removed needless "//"

[Gyuyoung Kim]

Comment on attachment 154474 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1102
> +    EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show);

Don't you need to check if inputValue is null ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1122
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, EINA_FALSE);

Use standard boolean instead of EINA_FALSE.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1123
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, EINA_FALSE);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1126
> +        return EINA_FALSE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1130
> +    return EINA_TRUE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1132
> +    return EINA_FALSE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1139
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, EINA_FALSE);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1140
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, EINA_FALSE);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1143
> +        return EINA_FALSE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1148
> +    return EINA_TRUE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1150
> +    return EINA_FALSE;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:109
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type input_type, const char *input_value);

Whenever you change this public smart class, you should update EWK_VIEW_SMART_CLASS_VERSION.

[KwangYong Choi]

Created attachment 154565 [details]
Patch

1. Modified to use standard boolean value instead of EINA_TRUE/EINA_FALSE.

The input value is color.serialized(). It can not be null, I think.

[Gyuyoung Kim]

Comment on attachment 154565 [details]
Patch

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

Almost looks fine except for my comment.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:70
> +    WebColorChooserProxyEfl* colorChooser;

Don't you need to initialize this variable ?

[KwangYong Choi]

Created attachment 154833 [details]
Patch

Added initialization code.

[Gyuyoung Kim]

Comment on attachment 154833 [details]
Patch

LGTM now, thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 154833 [details]
Patch

Can you make some test or at least show that it is possible to easily inherit to implement the picker.

[KwangYong Choi]

(In reply to comment #21)
> (From update of attachment 154833 [details])
> Can you make some test or at least show that it is possible to easily inherit to implement the picker.

As you know, this patch is an interface from UI to the color chooser on WebCore.

This implementation has 3 interfaces.

1. Show color picker
2. Set color changed
3. Close color picker

The application(browser app, etc) using webkit should provide color picker. It should show color picker dialog or popup when it receives show color picker call. The color picker implementation can be anything including color selector of EFL. I think, it's similar to color dialog on Windows. After showing the color picker, the user can select color from color dialog or popup.

Below is pseudo example for using color picker.

Eina_Bool browser_view_smart_class_set(Ewk_View_Smart_Class* api)
{
    if (!ewk_view_smart_class_set(api))
        return false;

    // register show color picker callback to smart class
    api->input_picker_show = _browser_input_picker_show;

    return true;
}

void _browser_input_picker_show(smartData, type, value)
{
    // display input picker by type and register callback functions
    elm_color_selector_show(); // this is for example
    register_changed_callback(callback_color_selected);
    register_close_callback(callback_color_picker_closed);
}

void callback_color_selected(color)
{
    // set color changed
    ewk_view_color_chooser_color_set(color);
}

void callback_color_picker_closed()
{
    // close color picker
    ewk_view_color_chooser_close();

    // unregister callback functions and destroy color picker
}

[Ryuan Choi]

Comment on attachment 154833 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1105
> +    EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show);

EINA_SAFETY_ON_NULL_RETURN will complain when input_picker_show is null.

please change this as `if () return;`

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.cpp:43
> +WebColorChooserProxyEfl::~WebColorChooserProxyEfl()
> +{
> +}

I am not sure whether we don't need to clear colorChooser of ewk_view.

[KwangYong Choi]

(In reply to comment #23)
> (From update of attachment 154833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154833&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1105
> > +    EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show);
> 
> EINA_SAFETY_ON_NULL_RETURN will complain when input_picker_show is null.
> 
> please change this as `if () return;`

OK.

> 
> > Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.cpp:43
> > +WebColorChooserProxyEfl::~WebColorChooserProxyEfl()
> > +{
> > +}
> 
> I am not sure whether we don't need to clear colorChooser of ewk_view.

I think it should be required. Color chooser can be destroyed before color picker closed.

[KwangYong Choi]

Created attachment 157372 [details]
Patch

Added destroy api to close input picker on ewk_view.

[Ryuan Choi]

Comment on attachment 157372 [details]
Patch

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

I think that patch itself is almost done.
If possible, I want to add test cases for this.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:72
> +#if ENABLE(INPUT_TYPE_COLOR)
> +    WebColorChooserProxyEfl* colorChooser;
> +#endif
> +

I think that you should rebase.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1154
> +    priv->colorChooser->setSelectedColor(Color(color));

You should check whether color is null and valid.
If color is null, it will be crashed.

please fix and make test case.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:75
> +    EWK_INPUT_TYPE_TELEPHONE = 1, 

White space in last letter.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:473
> + * @param color the color value to be set

Could you address which kinds of string is allowed.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:82
> +void ewk_view_input_picker_close(Evas_Object* ewkView, Ewk_Input_Type inputType);
> +#if ENABLE(INPUT_TYPE_COLOR)

I prefer to add empty line.

[KwangYong Choi]

Comment on attachment 157372 [details]
Patch

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

I will find available test for this patch. It changes layout of <input type="color">. So, it will change some test related color type of input.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:72
>> +
> 
> I think that you should rebase.

OK.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1154
>> +    priv->colorChooser->setSelectedColor(Color(color));
> 
> You should check whether color is null and valid.
> If color is null, it will be crashed.
> 
> please fix and make test case.

OK.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:75
>> +    EWK_INPUT_TYPE_TELEPHONE = 1, 
> 
> White space in last letter.

OK.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:473
>> + * @param color the color value to be set
> 
> Could you address which kinds of string is allowed.

It is possible to set color by named color or #000000 style.

Do you think it's better to mention this on parameter description?

>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:82
>> +#if ENABLE(INPUT_TYPE_COLOR)
> 
> I prefer to add empty line.

OK.

[Ryuan Choi]

Comment on attachment 157372 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:473
>>> + * @param color the color value to be set
>> 
>> Could you address which kinds of string is allowed.
> 
> It is possible to set color by named color or #000000 style.
> 
> Do you think it's better to mention this on parameter description?

Yes, I want to update description.

If I am right, named color, #RRGGBB and #RGB are possible.

Can I know whether alpha is also supported for this tag?

[KwangYong Choi]

Comment on attachment 157372 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:473
>>>> + * @param color the color value to be set
>>> 
>>> Could you address which kinds of string is allowed.
>> 
>> It is possible to set color by named color or #000000 style.
>> 
>> Do you think it's better to mention this on parameter description?
> 
> Yes, I want to update description.
> 
> If I am right, named color, #RRGGBB and #RGB are possible.
> 
> Can I know whether alpha is also supported for this tag?

OK. I will update the description.

Color input type does not support alpha value.

[Gyuyoung Kim]

Comment on attachment 157372 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1106
> +    if (!smartData->api->input_picker_show)

I think EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show) is more simple.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1117
> +    if (!smartData->api->input_picker_hide)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1151
> +    if (!priv->colorChooser)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1168
> +    if (!priv->colorChooser)

ditto.

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.cpp:49
> +    if (m_client)

Generally, WebKit prefers to use early return as below,
   if (!m_client)
       return;

If this function will have more function calls, I think early return is better.

See also, WebColorChooserProxyQt.cpp

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.cpp:55
> +    if (m_client)

ditto.

[Gyuyoung Kim]

(In reply to comment #23)
> (From update of attachment 154833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154833&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1105
> > +    EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show);
> 
> EINA_SAFETY_ON_NULL_RETURN will complain when input_picker_show is null.
> 
> please change this as `if () return;

Ryuan, EINA_SAFETY_ON_NULL_RETURN() is already being used by WK2 ewk_view.cpp as below,

 EINA_SAFETY_ON_NULL_RETURN(smartData->api->focus_in);

Is it better to complain when input_picker_show is null ? How do you think about it ?

[Ryuan Choi]

(In reply to comment #31)
> (In reply to comment #23)
> > (From update of attachment 154833 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154833&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1105
> > > +    EINA_SAFETY_ON_NULL_RETURN(smartData->api->input_picker_show);
> > 
> > EINA_SAFETY_ON_NULL_RETURN will complain when input_picker_show is null.
> > 
> > please change this as `if () return;
> 
> Ryuan, EINA_SAFETY_ON_NULL_RETURN() is already being used by WK2 ewk_view.cpp as below,
> 
>  EINA_SAFETY_ON_NULL_RETURN(smartData->api->focus_in);
> 
> Is it better to complain when input_picker_show is null ? How do you think about it ?

OK, I wanted not for minibrowser to complain.
But now, it looks reasonable.
When input_picker_show is null, clicking <input type="color"> will not be working.
So, I agree with you.


KwangYoung, sorry for the confusion.
Could you keep your previous one like gyuyoung mentioned.

[Thiago Marcos P. Santos]

Could you please add some tests (preferable EFL WK2 unit tests) for this API.

These are the instructions of how to write new tests:
http://trac.webkit.org/wiki/EFLWebKitTests

I wrote the QtWebkit2 implementation of ColorPicker. If you are not sure of how to write the tests, you can use as inspiration (see bug 87989).

[KwangYong Choi]

(In reply to comment #33)
> Could you please add some tests (preferable EFL WK2 unit tests) for this API.
> 
> These are the instructions of how to write new tests:
> http://trac.webkit.org/wiki/EFLWebKitTests
> 
> I wrote the QtWebkit2 implementation of ColorPicker. If you are not sure of how to write the tests, you can use as inspiration (see bug 87989).

I have a plan to add a new test for this patch.

Thank you for your comment.

[KwangYong Choi]

Created attachment 159079 [details]
Patch

Applied Gyuyoung mentioned.

I tried to add a test to test_ewk2_view.cpp.

<script>
function show()
{
    document.getElementById("color").click();
}
</script>

<body onload="show()">
<input type="color" id="color">
</body>

But, DOMActivateEvent does not create color chooser because user gesture is missing. I think, an api to control user gesture is required to implement color chooser test on EFL. It's not exist yet.

So, I wrote example code for this patch. I will attach it for reference.

[KwangYong Choi]

Created attachment 159080 [details]
example

Color picker example.

[KwangYong Choi]

Created attachment 159081 [details]
screenshot

Screenshot.

[Thiago Marcos P. Santos]

Comment on attachment 159079 [details]
Patch

Please add API tests like in bug 88101.

[KwangYong Choi]

(In reply to comment #38)
> (From update of attachment 159079 [details])
> Please add API tests like in bug 88101.

To implement a test for this patch, it should be required new api to set user gesture. Do you think the new api for test should be implemented?

As you know, EFL port uses gtest. And this test framework can not pass user action like QT or eventSender in layout test, as I know.

With my observation, it's not possible to create color chooser without user gesture.

[Ryuan Choi]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 159079 [details] [details])
> > Please add API tests like in bug 88101.
> 
> To implement a test for this patch, it should be required new api to set user gesture. Do you think the new api for test should be implemented?
> 
> As you know, EFL port uses gtest. And this test framework can not pass user action like QT or eventSender in layout test, as I know.
> 
> With my observation, it's not possible to create color chooser without user gesture.

I believe that test is important, so we'd better to make it.

but because we can not make test case now,
I suggest creating new bug to catch up when it is available.

tmpsantos, kwangyong,
How do you think about it?

[Thiago Marcos P. Santos]

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #38)
> > > (From update of attachment 159079 [details] [details] [details])
> > > Please add API tests like in bug 88101.
> > 
> > To implement a test for this patch, it should be required new api to set user gesture. Do you think the new api for test should be implemented?
> > 
> > As you know, EFL port uses gtest. And this test framework can not pass user action like QT or eventSender in layout test, as I know.
> > 
> > With my observation, it's not possible to create color chooser without user gesture.
> 
> I believe that test is important, so we'd better to make it.
> 
> but because we can not make test case now,
> I suggest creating new bug to catch up when it is available.
> 
> tmpsantos, kwangyong,
> How do you think about it?

Christophe has a patch that adds click support to the utest framework. I added it as dependency of this bug.

[Gyuyoung Kim]

>> Christophe has a patch that adds click support to the utest framework. I added it as dependency of this bug.

It looks this patch can simulate user gesture as Bug 90454. But, I'd like to recommend to support this test in new bug because of review convenience. KwangYoung, could you file a bug for this patch's unit test ?

[Gyuyoung Kim]

Comment on attachment 159079 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:35
> +

Don't you need to declare Color namespace as Qt port ?

http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/WebColorChooserProxyQt.h#L36

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:46
> +    virtual ~WebColorChooserProxyEfl();

I think we don't need to use virtual keyword in derived class. Look at Qt port. 
http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/WebColorChooserProxyQt.h#L50

[Chris Dumez]

(In reply to comment #42)
> >> Christophe has a patch that adds click support to the utest framework. I added it as dependency of this bug.
> 
> It looks this patch can simulate user gesture as Bug 90454. But, I'd like to recommend to support this test in new bug because of review convenience. KwangYoung, could you file a bug for this patch's unit test ?

tmpsantos can confirm but I believe he prefers that the unit tests lands in the same patch and the feature. This avoids forgetting about the utests later and the functionality is tested as soon as it lands and we know it works for sure when it lands.

[Chris Dumez]

Comment on attachment 159079 [details]
Patch

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

>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:46
>> +    virtual ~WebColorChooserProxyEfl();
> 
> I think we don't need to use virtual keyword in derived class. Look at Qt port. 
> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/WebColorChooserProxyQt.h#L50

Yes, it is unlikely that we subclass WebColorChooserProxyEfl so I tend to agree with Gyugyoung (although not really an issue).

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:48
> +    virtual void endChooser();

If you follow Gyuyoung's advice and remove virtual destructor from the destructor then please do the same for this method (or it would be confusing).

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:49
> +    virtual void setSelectedColor(const WebCore::Color&);

Ditto.

[Gyuyoung Kim]

(In reply to comment #44)
> (In reply to comment #42)
> > >> Christophe has a patch that adds click support to the utest framework. I added it as dependency of this bug.
> > 
> > It looks this patch can simulate user gesture as Bug 90454. But, I'd like to recommend to support this test in new bug because of review convenience. KwangYoung, could you file a bug for this patch's unit test ?
> 
> tmpsantos can confirm but I believe he prefers that the unit tests lands in the same patch and the feature. This avoids forgetting about the utests later and the functionality is tested as soon as it lands and we know it works for sure when it lands.

If we link a bug for unit test to this bug, we can remember to add unit test. IIRC, did you add unit tests in new bug ? In addition, I think review thread in this bug is already long. But, if KwangYong want to add unit test to this patch, I don't mind.

[Thiago Marcos P. Santos]

(In reply to comment #46)
> (In reply to comment #44)
> > (In reply to comment #42)
> > > >> Christophe has a patch that adds click support to the utest framework. I added it as dependency of this bug.
> > > 
> > > It looks this patch can simulate user gesture as Bug 90454. But, I'd like to recommend to support this test in new bug because of review convenience. KwangYoung, could you file a bug for this patch's unit test ?
> > 
> > tmpsantos can confirm but I believe he prefers that the unit tests lands in the same patch and the feature. This avoids forgetting about the utests later and the functionality is tested as soon as it lands and we know it works for sure when it lands.
> 
> If we link a bug for unit test to this bug, we can remember to add unit test. IIRC, did you add unit tests in new bug ? In addition, I think review thread in this bug is already long. But, if KwangYong want to add unit test to this patch, I don't mind.

When developers adds new features to WebCore, they have to write layout tests at the same patch. With EFL API, we should do the same. The only exception would be if, before landing this patch, the bug for the utests has a patch with r+. That was my case when I was writing ColorPicker for Qt and the reason I did that was because the test was QML and had to be reviewed by a QML expert.

New EFL features without tests should not be accepted anymore. We have a pretty decent test framework, bots are running the tests for every commit, documentation about how to write tests, etc.

Untested features are a nightmare when things start to fail, you are completely clueless about where to start looking. EFL WK2 is new, we have unique opportunity to have an API with 100% of test coverage.

[Gyuyoung Kim]

(In reply to comment #47)
> When developers adds new features to WebCore, they have to write layout tests at the same patch. With EFL API, we should do the same. The only exception would be if, before landing this patch, the bug for the utests has a patch with r+. That was my case when I was writing ColorPicker for Qt and the reason I did that was because the test was QML and had to be reviewed by a QML expert.
> 
> New EFL features without tests should not be accepted anymore. We have a pretty decent test framework, bots are running the tests for every commit, documentation about how to write tests, etc.
> 
> Untested features are a nightmare when things start to fail, you are completely clueless about where to start looking. EFL WK2 is new, we have unique opportunity to have an API with 100% of test coverage.

Yes, we should add new feature with test case. I agree with your opinion 100%. But, recently some other patches weren't added with test cases AFAIK. I just didn't want to add new files because this patch was already reviewed many things. As you guys had added test cases in new bug (For example, Bug 90454), I think this patch can have a new bug for unit test because unit test problem wasn't addressed so far. Of course, I say again, I agree to land new functionality with test cases from now.

Because it looks many efl contributors don't know this issue well yet, please notify this to webkit-efl well.

[KwangYong Choi]

Comment on attachment 159079 [details]
Patch

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

Well, I will apply new patch with a test. I think, it's better to submit a feature with a test at the same time.

>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:35
>> +
> 
> Don't you need to declare Color namespace as Qt port ?
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/WebColorChooserProxyQt.h#L36

#include "Color.h" is enough to use WebCore::Color.

>>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:46
>>> +    virtual ~WebColorChooserProxyEfl();
>> 
>> I think we don't need to use virtual keyword in derived class. Look at Qt port. 
>> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/WebColorChooserProxyQt.h#L50
> 
> Yes, it is unlikely that we subclass WebColorChooserProxyEfl so I tend to agree with Gyugyoung (although not really an issue).

OK. I will remove virtual keyword.

>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:48
>> +    virtual void endChooser();
> 
> If you follow Gyuyoung's advice and remove virtual destructor from the destructor then please do the same for this method (or it would be confusing).

OK

>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:49
>> +    virtual void setSelectedColor(const WebCore::Color&);
> 
> Ditto.

OK

[KwangYong Choi]

Created attachment 159343 [details]
Patch with test

1. Removed virtual keyword.

2. Added "EINA_SAFETY_ON_NULL_RETURN(priv->colorChooser);" in ewk_view_color_chooser_hide_request().

3. Test is implemented.

[Gyuyoung Kim]

Comment on attachment 159343 [details]
Patch with test

Attachment 159343 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13527858

[Kenneth Rohde Christiansen]

Comment on attachment 159343 [details]
Patch with test

General question, the input picker covers a whole lot, time, date, email, etc and is all kept together. Why is color different? and warrants a api on its own? API legacy? Can it be fixed?

Second question, why not make a proper WebKit2 C api for setting these which can be shared across ports and easily wrapped if needed. It feels pretty much like something which should be part of the UIClient. Have you looked into what kind of similar API we might get in the near future? File picker? Media picker?

I think you need to consider all these picker API's and figure out where they differ and which ones deserve an API on their own and which ones can be shared. Also you need to consider future changes in these APIs. How do we make an API that is more or less future proof? Do we need to?

What do we need really?

Media picker? (photos, files, etc)
Date and Time picker (date, time, month, week, etc)
Color picker?

That seems to cover most what is needed now.

telephone, number, email, url, password etc will probably not be implemented using pickers (they might, they might not). As we don't know how to handle these now, maybe we should leave them for later?

Lots of questions which I think should be looked at before diving into the code

[KwangYong Choi]

As I know, there are three kinds of input type. One is file, another is color and the other is date and time related. These types are handled differently in webkit. File type is using file chooser(openPanel), color type is using color chooser(has own proxy) and the others are handled as text field.

So, I think, it's required three kinds of api; the api for file input type, the api for color input type and the api for other types. This patch is one of these apis for color input type.

Date and time picker can be implemented by 1 api which is set the value. But color picker has 2 apis. One is set the value and the other is close the color chooser. It's allowed to call set api multiple times before close the color chooser. I think it's a difference between EFL and QT.

Why I am not make it as WK2 C api is because I'm not sure it's required for all ports. I'm working on EFL port and it's required, so I just implemented it for EFL. Actually, it's been implemented for Tizen first. I think it can be contributed, so I uploaded this patch.

As you mentioned, file picker, color picker and the date and time picker is most what we need for now. File picker and date and time picker should be implemented for EFL soon.

Rest of the types in ewk_view.h like tel, number, email, etc are for the reserved use. These values can be used by application, in the future. I may remove these values for now, and add again when these values are used.

[Kenneth Rohde Christiansen]

(In reply to comment #53)
> As I know, there are three kinds of input type. One is file, another is color and the other is date and time related. These types are handled differently in webkit. File type is using file chooser(openPanel), color type is using color chooser(has own proxy) and the others are handled as text field.
> 
> So, I think, it's required three kinds of api; the api for file input type, the api for color input type and the api for other types. This patch is one of these apis for color input type.

Could we make it only do that then? and file bugs for the others?

> Date and time picker can be implemented by 1 api which is set the value. But color picker has 2 apis. One is set the value and the other is close the color chooser. It's allowed to call set api multiple times before close the color chooser. I think it's a difference between EFL and QT.

Why shouldn't that be possible for a date? Shouldn't it have select and close as well? I think it should.

> Why I am not make it as WK2 C api is because I'm not sure it's required for all ports. I'm working on EFL port and it's required, so I just implemented it for EFL. Actually, it's been implemented for Tizen first. I think it can be contributed, so I uploaded this patch.

I am pretty sure that Qt and GTK would like this as well, it would be cool if you could give it a try

> As you mentioned, file picker, color picker and the date and time picker is most what we need for now. File picker and date and time picker should be implemented for EFL soon.
> 
> Rest of the types in ewk_view.h like tel, number, email, etc are for the reserved use. These values can be used by application, in the future. I may remove these values for now, and add again when these values are used.

I think you should remove them for now, they can be handled in the future.

[KwangYong Choi]

(In reply to comment #54)
> (In reply to comment #53)
> > As I know, there are three kinds of input type. One is file, another is color and the other is date and time related. These types are handled differently in webkit. File type is using file chooser(openPanel), color type is using color chooser(has own proxy) and the others are handled as text field.
> > 
> > So, I think, it's required three kinds of api; the api for file input type, the api for color input type and the api for other types. This patch is one of these apis for color input type.
> 
> Could we make it only do that then? and file bugs for the others?

I think it's possible to file bugs for the others in the future. The features are separately implemented on Tizen.

> > Date and time picker can be implemented by 1 api which is set the value. But color picker has 2 apis. One is set the value and the other is close the color chooser. It's allowed to call set api multiple times before close the color chooser. I think it's a difference between EFL and QT.
> 
> Why shouldn't that be possible for a date? Shouldn't it have select and close as well? I think it should.

You're right. It can be possible. I just think about local implementation. In case of date and time picker, only one api which is set the value is implemented. I think, it's possible to implement for color type as well.

Do you think only one api which is set the value is required for all input pickers? I designed input_picker_show/input_picker_hide function can be used for all input types, but not for the set apis. Is it better to provide one api for the all types?

> > Why I am not make it as WK2 C api is because I'm not sure it's required for all ports. I'm working on EFL port and it's required, so I just implemented it for EFL. Actually, it's been implemented for Tizen first. I think it can be contributed, so I uploaded this patch.
> 
> I am pretty sure that Qt and GTK would like this as well, it would be cool if you could give it a try

Well, at first, I want just to implement color chooser proxy for EFL. For now, apis are added and a test is included. This patch is larger and larger. And it takes long time.

I wish WK2 C api could be handled by separate bug.

> > As you mentioned, file picker, color picker and the date and time picker is most what we need for now. File picker and date and time picker should be implemented for EFL soon.
> > 
> > Rest of the types in ewk_view.h like tel, number, email, etc are for the reserved use. These values can be used by application, in the future. I may remove these values for now, and add again when these values are used.
> 
> I think you should remove them for now, they can be handled in the future.

OK. You're right. I remove them for now.

[Kenneth Rohde Christiansen]

> I think it's possible to file bugs for the others in the future. The features are separately implemented on Tizen.

Please do so and make a master bug, cc me.

> You're right. It can be possible. I just think about local implementation. In case of date and time picker, only one api which is set the value is implemented. I think, it's possible to implement for color type as well.

I think all should have a similar API. It should only differ if there is a valid reason to!
 
> Do you think only one api which is set the value is required for all input pickers? I designed input_picker_show/input_picker_hide function can be used for all input types, but not for the set apis. Is it better to provide one api for the all types?

I think it is more future proof to make it say 3 sets of API calls, like one set for color, one for media (file, photos etc) and one for time and date.

> Well, at first, I want just to implement color chooser proxy for EFL. For now, apis are added and a test is included. This patch is larger and larger. And it takes long time.
> 
> I wish WK2 C api could be handled by separate bug.

It can.

> OK. You're right. I remove them for now.

Good

[KwangYong Choi]

Created attachment 159657 [details]
Patch

This patch has only 1 api named ewk_view_input_picker_color_set() for setting color, now. Similar apis can be implemented for the other types later.

[Kenneth Rohde Christiansen]

Comment on attachment 159657 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:615
> +EAPI Eina_Bool ewk_view_input_picker_color_set(Evas_Object *o, const char *color);

it sounds as you can set color on any input picker which is not the case... wouldn't ewk_view_color_picker_color_set() ont be better?

[Gyuyoung Kim]

Comment on attachment 159657 [details]
Patch

Attachment 159657 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13550297

[KwangYong Choi]

(In reply to comment #58)
> (From update of attachment 159657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159657&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:615
> > +EAPI Eina_Bool ewk_view_input_picker_color_set(Evas_Object *o, const char *color);
> 
> it sounds as you can set color on any input picker which is not the case... wouldn't ewk_view_color_picker_color_set() ont be better?

OK. I will change the api name.

[KwangYong Choi]

(In reply to comment #59)
> (From update of attachment 159657 [details])
> Attachment 159657 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13550297

Gyuyoung, I'm not sure why EWS fails on EFL. It has no problem on my local 32 bit and 64 bit Ubuntu. Can you see anything strange in this patch?

[Thiago Marcos P. Santos]

(In reply to comment #61)
> (In reply to comment #59)
> > (From update of attachment 159657 [details] [details])
> > Attachment 159657 [details] [details] did not pass efl-ews (efl):
> > Output: http://queues.webkit.org/results/13550297
> 
> Gyuyoung, I'm not sure why EWS fails on EFL. It has no problem on my local 32 bit and 64 bit Ubuntu. Can you see anything strange in this patch?

../../lib/libewk2UnitTestUtils.a(EWK2UnitTestBase.cpp.o): In function `EWK2UnitTest::EWK2UnitTestBase::SetUp()':
EWK2UnitTestBase.cpp:(.text+0xef): undefined reference to `ewk_view_smart_add'
collect2: ld returned 1 exit status
make[2]: *** [bin/test_ewk2_cookie_manager] Error 1
make[1]: *** [Source/WebKit2/CMakeFiles/test_ewk2_cookie_manager.dir/all] Error 2
make: *** [all] Error 2

The problem here is:
ewk_view_smart_add is not prefixed with EAPI on WebKit2/UIProcess/API/efl/ewk_view.h

[Thiago Marcos P. Santos]

(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #59)
> > > (From update of attachment 159657 [details] [details] [details])
> > > Attachment 159657 [details] [details] [details] did not pass efl-ews (efl):
> > > Output: http://queues.webkit.org/results/13550297
> > 
> > Gyuyoung, I'm not sure why EWS fails on EFL. It has no problem on my local 32 bit and 64 bit Ubuntu. Can you see anything strange in this patch?
> 
> ../../lib/libewk2UnitTestUtils.a(EWK2UnitTestBase.cpp.o): In function `EWK2UnitTest::EWK2UnitTestBase::SetUp()':
> EWK2UnitTestBase.cpp:(.text+0xef): undefined reference to `ewk_view_smart_add'
> collect2: ld returned 1 exit status
> make[2]: *** [bin/test_ewk2_cookie_manager] Error 1
> make[1]: *** [Source/WebKit2/CMakeFiles/test_ewk2_cookie_manager.dir/all] Error 2
> make: *** [all] Error 2
> 
> The problem here is:
> ewk_view_smart_add is not prefixed with EAPI on WebKit2/UIProcess/API/efl/ewk_view.h

Actually many of the public API's are not. I'm going to fix this in a separated bug.

[KwangYong Choi]

(In reply to comment #63)
> (In reply to comment #62)
> > (In reply to comment #61)
> > > (In reply to comment #59)
> > > > (From update of attachment 159657 [details] [details] [details] [details])
> > > > Attachment 159657 [details] [details] [details] [details] did not pass efl-ews (efl):
> > > > Output: http://queues.webkit.org/results/13550297
> > > 
> > > Gyuyoung, I'm not sure why EWS fails on EFL. It has no problem on my local 32 bit and 64 bit Ubuntu. Can you see anything strange in this patch?
> > 
> > ../../lib/libewk2UnitTestUtils.a(EWK2UnitTestBase.cpp.o): In function `EWK2UnitTest::EWK2UnitTestBase::SetUp()':
> > EWK2UnitTestBase.cpp:(.text+0xef): undefined reference to `ewk_view_smart_add'
> > collect2: ld returned 1 exit status
> > make[2]: *** [bin/test_ewk2_cookie_manager] Error 1
> > make[1]: *** [Source/WebKit2/CMakeFiles/test_ewk2_cookie_manager.dir/all] Error 2
> > make: *** [all] Error 2
> > 
> > The problem here is:
> > ewk_view_smart_add is not prefixed with EAPI on WebKit2/UIProcess/API/efl/ewk_view.h
> 
> Actually many of the public API's are not. I'm going to fix this in a separated bug.

I see now. Thank you for your comment.

[KwangYong Choi]

Created attachment 159817 [details]
Patch

ewk_view_input_picker_color_set() -> ewk_view_color_picker_color_set()

[Chris Dumez]

Comment on attachment 159817 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:246
> +    loadUrlSync(environment->urlForResource("input-type-color.html").data());

Personally, I would have used ewk_view_html_string_load() to avoid adding a input-type-color.html resource just for this.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:257
> +    mouseClick(30, 20);

Shouldn't the input element be positioned absolutely in CSS to ensure this works?

[Kenneth Rohde Christiansen]

Comment on attachment 159817 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/resources/input-type-color.html:4
> +<input type="color" value="#ff0000">

you could load this as simple "data:text/html,<input type='color' value='#ff0000'>" WebKit will fill out the rest

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:246
> +    loadUrlSync(environment->urlForResource("input-type-color.html").data());

loadUrlSync("data:text/html,<input type='color' value='#ff0000'>");

[KwangYong Choi]

Comment on attachment 159817 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:246
>>> +    loadUrlSync(environment->urlForResource("input-type-color.html").data());
>> 
>> Personally, I would have used ewk_view_html_string_load() to avoid adding a input-type-color.html resource just for this.
> 
> loadUrlSync("data:text/html,<input type='color' value='#ff0000'>");

OK. I will change.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:257
>> +    mouseClick(30, 20);
> 
> Shouldn't the input element be positioned absolutely in CSS to ensure this works?

This is a simple html. So, I think, the element can not avoid my click. :)

[Kenneth Rohde Christiansen]

(In reply to comment #68)
> (From update of attachment 159817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159817&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:246
> >>> +    loadUrlSync(environment->urlForResource("input-type-color.html").data());
> >> 
> >> Personally, I would have used ewk_view_html_string_load() to avoid adding a input-type-color.html resource just for this.
> > 
> > loadUrlSync("data:text/html,<input type='color' value='#ff0000'>");
> 
> OK. I will change.
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:257
> >> +    mouseClick(30, 20);
> > 
> > Shouldn't the input element be positioned absolutely in CSS to ensure this works?
> 
> This is a simple html. So, I think, the element can not avoid my click. :)

QtWebKit2 has api for sending events from the page to the ui and the other way around using a window.navigator.qt bridge and also an evaluateJavaScript.

At least evaluateJavaScript would be a nice addition so you can actually query the right coordinates. (That is of course another patch)

[KwangYong Choi]

(In reply to comment #69)
> (In reply to comment #68)
> > (From update of attachment 159817 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159817&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:246
> > >>> +    loadUrlSync(environment->urlForResource("input-type-color.html").data());
> > >> 
> > >> Personally, I would have used ewk_view_html_string_load() to avoid adding a input-type-color.html resource just for this.
> > > 
> > > loadUrlSync("data:text/html,<input type='color' value='#ff0000'>");
> > 
> > OK. I will change.
> > 
> > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:257
> > >> +    mouseClick(30, 20);
> > > 
> > > Shouldn't the input element be positioned absolutely in CSS to ensure this works?
> > 
> > This is a simple html. So, I think, the element can not avoid my click. :)
> 
> QtWebKit2 has api for sending events from the page to the ui and the other way around using a window.navigator.qt bridge and also an evaluateJavaScript.
> 
> At least evaluateJavaScript would be a nice addition so you can actually query the right coordinates. (That is of course another patch)

That's a great idea for choosing the coordinate. I will find the way to use javascript execution. Maybe later?

[Kenneth Rohde Christiansen]

> That's a great idea for choosing the coordinate. I will find the way to use javascript execution. Maybe later?

Yes, later is fine

[KwangYong Choi]

Created attachment 159883 [details]
Patch

Modified to use html string instead of resource html file.

[Kenneth Rohde Christiansen]

Comment on attachment 159883 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:238
> +static Eina_Bool hideColorPicker(Ewk_View_Smart_Data*, Ewk_Input_Type inputType)
> +{
> +    EXPECT_EQ(inputType, EWK_INPUT_TYPE_COLOR);
> +}

Isn't it more important knowing that it was actually asked to hide, than whether it being the right type. Could you test the former?

[KwangYong Choi]

Comment on attachment 159883 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:238
>> +}
> 
> Isn't it more important knowing that it was actually asked to hide, than whether it being the right type. Could you test the former?

Well, may I test the picker status here? I mean, whether showcolorPicker() is called or not.

[KwangYong Choi]

Comment on attachment 159883 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:238
>>> +}
>> 
>> Isn't it more important knowing that it was actually asked to hide, than whether it being the right type. Could you test the former?
> 
> Well, may I test the picker status here? I mean, whether showcolorPicker() is called or not.

Or, I can check ewk_view_color_picker_color_set() returns false here.

The application should do something here, so I'm not making many test here.

[Kenneth Rohde Christiansen]

(In reply to comment #75)
> (From update of attachment 159883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159883&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:238
> >>> +}
> >> 
> >> Isn't it more important knowing that it was actually asked to hide, than whether it being the right type. Could you test the former?
> > 
> > Well, may I test the picker status here? I mean, whether showcolorPicker() is called or not.
> 
> Or, I can check ewk_view_color_picker_color_set() returns false here.
> 
> The application should do something here, so I'm not making many test here.

Why not just add a static bool wasHidden = false; then set it to true and test later

[KwangYong Choi]

Comment on attachment 159883 [details]
Patch

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

>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:238
>>>>> +}
>>>> 
>>>> Isn't it more important knowing that it was actually asked to hide, than whether it being the right type. Could you test the former?
>>> 
>>> Well, may I test the picker status here? I mean, whether showcolorPicker() is called or not.
>> 
>> Or, I can check ewk_view_color_picker_color_set() returns false here.
>> 
>> The application should do something here, so I'm not making many test here.
> 
> Why not just add a static bool wasHidden = false; then set it to true and test later

Actually, this function should be called twice because color picker is shown twice. So, I think, global variable is needed.

[KwangYong Choi]

Created attachment 159889 [details]
Patch

Added a test to check color picker is shown or not when it is closed.

[Simon Hausmann]

(In reply to comment #52)
> Second question, why not make a proper WebKit2 C api for setting these which can be shared across ports and easily wrapped if needed. It feels pretty much like something which should be part of the UIClient. Have you looked into what kind of similar API we might get in the near future? File picker? Media picker?

I agree with Kenneth here. It might be tricky to get it right with regards to the actual picker "values" and how to represent them properly in the C API, but conceptually I think it's definitely worth a try to come up with a C API here. And even then, the C API can consist of common parts and port specific parts (when certain types are needed).

I agree though that this can be a separate patch.

[KwangYong Choi]

(In reply to comment #79)
> (In reply to comment #52)
> > Second question, why not make a proper WebKit2 C api for setting these which can be shared across ports and easily wrapped if needed. It feels pretty much like something which should be part of the UIClient. Have you looked into what kind of similar API we might get in the near future? File picker? Media picker?
> 
> I agree with Kenneth here. It might be tricky to get it right with regards to the actual picker "values" and how to represent them properly in the C API, but conceptually I think it's definitely worth a try to come up with a C API here. And even then, the C API can consist of common parts and port specific parts (when certain types are needed).
> 
> I agree though that this can be a separate patch.

I created bug 95058 for implementing WK2 C API.

[KwangYong Choi]

Created attachment 161452 [details]
Patch

Implemented WK2 C API.

[Early Warning System Bot]

Comment on attachment 161452 [details]
Patch

Attachment 161452 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13685728

[Kenneth Rohde Christiansen]

Comment on attachment 161452 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2716
>      m_colorChooser = m_pageClient->createColorChooserProxy(this, initialColor);
> +
> +    m_uiClient.showColorPicker(this, initialColor.serialized());

So shouldn't it be like if no showColorPicker method exists you use the m_colorChooser line? if not, you use this? So there is some kind of default fallback for platforms that wants that?

[Simon Hausmann]

Comment on attachment 161452 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPage.h:289
> +    WKPageShowColorPickerCallback                                       showColorPicker;
> +    WKPageHideColorPickerCallback                                       hideColorPicker;

These additions will require changing QtWebPageUIClient in order to build the Qt port.

> Source/WebKit2/UIProcess/API/C/WKPage.h:500
> +WK_EXPORT void WKPageSetColorPickerColor(WKPageRef pageRef, WKStringRef selectedColor);

I'm not 1000% sure, but I think a more common pattern would be to give the showColorPicker callback another parameter to use for setting the color instead of adding a function to the page. A bit like with WKFramePolicyListenerRef. What do you think? Maybe that's overkill?

> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:40
> +class WebColorChooserProxyEfl : public WebColorChooserProxy {

I don't understand why you need this *Efl class when you have a C API that is responsible for the entire delegation.

Do you plan to generalize this in a next step, i.e. we could port the Qt implementation to use the C API, too.

[KwangYong Choi]

Comment on attachment 161452 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPage.h:289
>> +    WKPageHideColorPickerCallback                                       hideColorPicker;
> 
> These additions will require changing QtWebPageUIClient in order to build the Qt port.

OK. I will check.

>> Source/WebKit2/UIProcess/API/C/WKPage.h:500
>> +WK_EXPORT void WKPageSetColorPickerColor(WKPageRef pageRef, WKStringRef selectedColor);
> 
> I'm not 1000% sure, but I think a more common pattern would be to give the showColorPicker callback another parameter to use for setting the color instead of adding a function to the page. A bit like with WKFramePolicyListenerRef. What do you think? Maybe that's overkill?

OK. I will check this, too.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:2716
>> +    m_uiClient.showColorPicker(this, initialColor.serialized());
> 
> So shouldn't it be like if no showColorPicker method exists you use the m_colorChooser line? if not, you use this? So there is some kind of default fallback for platforms that wants that?

Use colorChooserProxy if showColorPicker is not exist? These two are not required at once?

>> Source/WebKit2/UIProcess/efl/WebColorChooserProxyEfl.h:40
>> +class WebColorChooserProxyEfl : public WebColorChooserProxy {
> 
> I don't understand why you need this *Efl class when you have a C API that is responsible for the entire delegation.
> 
> Do you plan to generalize this in a next step, i.e. we could port the Qt implementation to use the C API, too.

Well.. I think, the member variable m_webView is not necessary and this class can be generalized.

I tend to keep the previous implementation, so I just added it for EFL. If I use C API, then color picker can be implemented without WebColorChooserProxy.

[Build Bot]

Comment on attachment 161452 [details]
Patch

Attachment 161452 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13695694

[Alexey Proskuryakov]

This touches cross-platform files, so removing [EFL] from title.

[KwangYong Choi]

WK2 C API part is handled on bug 95058.

[KwangYong Choi]

Created attachment 164067 [details]
Patch

Implemented color picker API for EFL port with WK2 C API.

[Kenneth Rohde Christiansen]

Comment on attachment 164067 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:137
> +#if ENABLE(INPUT_TYPE_COLOR)

Will we ever allow people to build EFL without color picker? if not im not sure it makes so much sense adding all these

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1657
> +#if ENABLE(INPUT_TYPE_COLOR)
> +void ewk_view_color_picker_show(Evas_Object* ewkView, const String& inputValue, WKColorPickerResultListenerRef listener)

#if outside

If not build with INPUT_TYPE_COLOR you dont have this api

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1670
> +Eina_Bool ewk_view_color_picker_color_set(Evas_Object* ewkView, const char* color)
> +{
> +#if ENABLE(INPUT_TYPE_COLOR)

#if inside

But you have this one??? very inconsistent

Impossible to see from the code what is internal and not. Qt had

/*! \internal
 */

[KwangYong Choi]

Comment on attachment 164067 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:137
>> +#if ENABLE(INPUT_TYPE_COLOR)
> 
> Will we ever allow people to build EFL without color picker? if not im not sure it makes so much sense adding all these

OK. I will check I can remove INPUT_TYPE_COLOR in ewk_* files.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1657
>> +void ewk_view_color_picker_show(Evas_Object* ewkView, const String& inputValue, WKColorPickerResultListenerRef listener)
> 
> #if outside
> 
> If not build with INPUT_TYPE_COLOR you dont have this api

Yes. It's an internal function which is not used without INPUT_TYPE_COLOR.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1670
>> +#if ENABLE(INPUT_TYPE_COLOR)
> 
> #if inside
> 
> But you have this one??? very inconsistent
> 
> Impossible to see from the code what is internal and not. Qt had
> 
> /*! \internal
>  */

This is a public API. I think, it's not possible to add featuring to public API. So I featured inside of the function.

There is no difference between internal and public APIs in ewk_view.cpp.

May I add internal mark to internal function?
Is there any syntax meaning for marking internal like this?

/*!
    \internal
 */

[KwangYong Choi]

Created attachment 164333 [details]
Patch

Removed #if ENABLE(INPUT_TYPE_COLOR).

Added internal comment to internal APIs.

[Gyuyoung Kim]

Comment on attachment 164333 [details]
Patch

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

>> Will we ever allow people to build EFL without color picker? if not im not sure it makes so much sense adding all these

First of all, I don't like to use macro too much personally. However, EFL port has used macros for public function and internal function if the functionality is able to be supported only when feature macro is enabled. In input color picker case, it looks this feature needs to be enabled always. However, IMO, we need to use macro for some functions which I point out at least because this feature can be disabled by user.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1652
> +

We has wrapped internal function with ENABLE(XXX) macro so far.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1692
> +

#endif ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1694
> +{

EFL port has used #if ENABLE() for public function inside. If we don't use this macro, this API can return "true" though WKColorPickerResultListenerSetColor doesn't set color when ENABLE(INPUT_TYPE_COLOR) is disabled.

void WKColorPickerResultListenerSetColor(WKColorPickerResultListenerRef listenerRef, const WKStringRef color)
{
#if ENABLE(INPUT_TYPE_COLOR)
    toImpl(listenerRef)->setColor(toWTFString(color));
#endif
}

[KwangYong Choi]

Created attachment 164348 [details]
Patch

[KwangYong Choi]

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

Added #if ENABLE(INPUT_TYPE_COLOR) to the APIs.

[Gyuyoung Kim]

Comment on attachment 164348 [details]
Patch

Is there any unskip test cases in layout test ?

[KwangYong Choi]

(In reply to comment #96)
> (From update of attachment 164348 [details])
> Is there any unskip test cases in layout test ?

Actually, this patch does not change layout.

The test fast/forms/color/input-appearance-color.html is still failing because of rendering problem. It can be handled with another bug. This test is written in platform/efl/Skipped file.

344 # BUG: rendering problems
345 fast/forms/color/input-appearance-color.html

[Gyuyoung Kim]

Comment on attachment 164348 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:432
> +    evas_object_smart_callback_add(webView(), "input,type,color,request", colorPickerDoneCallback, &handled);

In unit test case, we have used *on* prefix for callback function.

[Kenneth Rohde Christiansen]

Comment on attachment 164348 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:130
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, const char *value);

I guess this isn't so future proof. What is other start values are needed? void* data?

[KwangYong Choi]

Comment on attachment 164348 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:130
>> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, const char *value);
> 
> I guess this isn't so future proof. What is other start values are needed? void* data?

You're right. It's not enough to use 'char*' for initialization in case of file picker. 'void*' looks better.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:432
>> +    evas_object_smart_callback_add(webView(), "input,type,color,request", colorPickerDoneCallback, &handled);
> 
> In unit test case, we have used *on* prefix for callback function.

OK. I will change.

[KwangYong Choi]

Created attachment 164496 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164496 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:131
> +    // Input Picker API
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, const void *data);
> +    Eina_Bool (*input_picker_hide)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type);

Where is the documentation for these? How to use, how to free the data etc?

[KwangYong Choi]

Created attachment 164524 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164524 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:136
> +    //   - For the EWK_INPUT_TYPE_COLOR type, data is the initial color value of char* type.

so is that like rgc(122, 23, 53), ff00ff etc? There are many ways to represent colors. Can it be any of these? Why not actually give a color? How to free this? (// which should be free'ed using free().

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:138
> +    Eina_Bool (*input_picker_show)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, const void *data);
> +    Eina_Bool (*input_picker_hide)(Ewk_View_Smart_Data *sd, Ewk_Input_Type type);

When can the page request hiding it? it would be nice to document? like are we requesting to hide it if the web process crashes for instance? Should we?

Can we ever have more than one input picker open at the same time? I mean is the type in the second function even needed?

Wouldn't multiple methods not be better?

like

Eina_bool (*input_picker_color_request(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, char* current_color_string);
Eina_bool (*input_picker_date_request(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, char* current_date_string);
Eina_Bool (*input_picker_dismiss)(Ewk_View_Smart_Data *sd);

But maybe as an EFL api the below makes more sense

Eina_bool (*input_picker_color_request(Ewk_View_Smart_Data *sd, Ewk_Input_Type type, int r, int g, int b, int a);

[KwangYong Choi]

Multiple methods are looks better.

Each type of input element is handled separately in WebCore. So, I think, request and dismiss functions are required for each type. I will check it more.

[KwangYong Choi]

Created attachment 164669 [details]
Patch

Used request/dismiss pair of smart class API for color picker. The APIs for other pickers may be implemented like this.

Changed color picker request and set APIs to take 4 int parameters instead of char*.

[Kenneth Rohde Christiansen]

Comment on attachment 164669 [details]
Patch

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

Almost there! Getting very nice.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:122
> +    Eina_Bool (*input_picker_color_dismiss)(Ewk_View_Smart_Data *sd);

I think we should call dismiss automatically if the process crashes, in the case it was requested.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:673
> +/*
> + * Sets the color value of color chooser
> + *
> + * @param o view object contains color chooser
> + * @param r red channel value to be set
> + * @param g green channel value to be set
> + * @param b blue channel value to be set
> + * @param a alpha channel value to be set
> + *
> + * @return @c EINA_TRUE on success @c EINA_FALSE otherwise
> + */

I think it should be said that you can only call this when color has been requested and until it is dismissed.

Also don't call it color chooser, the api says color picker

// Sets the user chosen color. To be used when implementing a color picker.


// The function should only be called when a color has been requested by the document.
// If called when this is not the case or when the input picker has been dismissed, this
// function will fail and return EINA_FALSE.

[KwangYong Choi]

Comment on attachment 164669 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:122
>> +    Eina_Bool (*input_picker_color_dismiss)(Ewk_View_Smart_Data *sd);
> 
> I think we should call dismiss automatically if the process crashes, in the case it was requested.

The input element can be removed during the color picker shown. So, I think, the application should know this to close their color picker.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:673
>> + */
> 
> I think it should be said that you can only call this when color has been requested and until it is dismissed.
> 
> Also don't call it color chooser, the api says color picker
> 
> // Sets the user chosen color. To be used when implementing a color picker.
> 
> 
> // The function should only be called when a color has been requested by the document.
> // If called when this is not the case or when the input picker has been dismissed, this
> // function will fail and return EINA_FALSE.

OK. I will update the description.

[KwangYong Choi]

Created attachment 164863 [details]
Patch

[Kenneth Rohde Christiansen]

> The input element can be removed during the color picker shown. So, I think, the application should know this to close their color picker.

So that is no excuse. Either it should be handled automatically or documented.

Also, we need to test this case where the input element gets removed during showing the input picker. Why can't we handle that? Maybe we should suspend javascript for the document while the picker is open?

[KwangYong Choi]

(In reply to comment #110)
> > The input element can be removed during the color picker shown. So, I think, the application should know this to close their color picker.
> 
> So that is no excuse. Either it should be handled automatically or documented.
> 
> Also, we need to test this case where the input element gets removed during showing the input picker. Why can't we handle that? Maybe we should suspend javascript for the document while the picker is open?

Do you think one show request API and one set API are enough to implement this? If the application receives show request then it shows a color picker with suspending javascript. After that set the color and dismiss the picker and resume javascript?

I think, I have to change WK2 C API to suspend javascript for the document. May I change WK2 C API before change this?

[Kenneth Rohde Christiansen]

Lets land this as it is, then you create tests requesting multiple pickers at the same time, one removing the element etc and then we fix those cases together with the tests, ok?

[KwangYong Choi]

(In reply to comment #112)
> Lets land this as it is, then you create tests requesting multiple pickers at the same time, one removing the element etc and then we fix those cases together with the tests, ok?

OK. :)

Thank you Kenneth, I've learned a lot from this bug thread.

[KwangYong Choi]

Unit test test_ewk2_view passed on both Release and Debug build.

[WebKit Review Bot]

Comment on attachment 164863 [details]
Patch

Clearing flags on attachment: 164863

Committed r129121: <http://trac.webkit.org/changeset/129121>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.