Bug 83692 – [EFL] Add API for color chooser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Thiago Marcos P. Santos]

EWK users will get a signal to create a color chooser dialog or whatever interface they find more appropriated when user clicks on a <input type=color>.

Will enable something like this to be implemented by an EFL browser: http://www.youtube.com/watch?v=IPwCH8WQ8Qo

[Thiago Marcos P. Santos]

Depends on this 82867 for testing.

[Thiago Marcos P. Santos]

Not really a dependency. My mistake.

[Thiago Marcos P. Santos]

Created attachment 136918 [details]
Adds color chooser API to EFL port

[Thiago Marcos P. Santos]

Tonikitoo: It might interest you since it is touching your build system. There was a typo on WebKitFeatures.cmake that I fixed. Was it working for you?

[Antonio Gomes]

(In reply to comment #4)
> Tonikitoo: It might interest you since it is touching your build system. There was a typo on WebKitFeatures.cmake that I fixed. Was it working for you?

what was the typo? rob/ming?

[Thiago Marcos P. Santos]

(In reply to comment #5)
> (In reply to comment #4)
> > Tonikitoo: It might interest you since it is touching your build system. There was a typo on WebKitFeatures.cmake that I fixed. Was it working for you?
> 
> what was the typo? rob/ming?

-WEBKIT_OPTION_DEFINE(ENABLE_INPUT_COLOR "Toggle Color Input support" OFF)
+WEBKIT_OPTION_DEFINE(ENABLE_INPUT_TYPE_COLOR "Toggle Color Input support" OFF)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136918 [details]
Adds color chooser API to EFL port

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

Looks mostly fine. I wonder if it wouldn't make sense to pass the color data as a struct to the users; otherwise, the documentation should at least mention how to extract the RGBA values from the integer (and note that it's encoded as ARGB instead of RGBA). Is there any test that can be unskipped?

Last but not least: do you know if the BlackBerry port does something similar to us (ie. relay a signal and leave it up to the user to create the color chooser)? I'm thinking how an actual implementation would work -- one would probably need the position where the event was emitted to create the widget, for example.

> Source/WebCore/platform/efl/ColorChooserEfl.h:51
> +    ChromeClientEfl *m_chromeClient;

Style nitpick: wrong placement of the asterisk.

[Rob Buis]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Tonikitoo: It might interest you since it is touching your build system. There was a typo on WebKitFeatures.cmake that I fixed. Was it working for you?
> > 
> > what was the typo? rob/ming?
> 
> -WEBKIT_OPTION_DEFINE(ENABLE_INPUT_COLOR "Toggle Color Input support" OFF)
> +WEBKIT_OPTION_DEFINE(ENABLE_INPUT_TYPE_COLOR "Toggle Color Input support" OFF)

I agree with this change.

[Thiago Marcos P. Santos]

(In reply to comment #7)
> (From update of attachment 136918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136918&action=review
> 
> Looks mostly fine. I wonder if it wouldn't make sense to pass the color data as a struct to the users; otherwise, the documentation should at least mention how to extract the RGBA values from the integer (and note that it's encoded as ARGB instead of RGBA). 

Hmmm... good point. I'm going to change to structure as you are suggesting since Evas always handles color as 4 separated bytes.

> Is there any test that can be unskipped?

There are some tests related to color input that are already succeeding but are unrelated to this patch. I going to unskip them on a different patch, otherwise is misleading.

> 
> Last but not least: do you know if the BlackBerry port does something similar to us (ie. relay a signal and leave it up to the user to create the color chooser)? I'm thinking how an actual implementation would work -- one would probably need the position where the event was emitted to create the widget, for example.

My implementation is based on the Chromium port, that doesn't delivery any kind of positioning coordinates (IMO because they assume the current mouse coordinates). For BlackBerry port this should not matter, since I guess it will open a fullscreen window on the mobile device.

> 
> > Source/WebCore/platform/efl/ColorChooserEfl.h:51
> > +    ChromeClientEfl *m_chromeClient;
> 
> Style nitpick: wrong placement of the asterisk.

Your eyes are better than the style-checker. Thanks for reviewing. :)

[Thiago Marcos P. Santos]

Created attachment 136953 [details]
Added Ewk_Color structure.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136953 [details]
Added Ewk_Color structure.

Informal r+, thanks for the patch.

[Thiago Marcos P. Santos]

Created attachment 137061 [details]
Same as before, but fixes a typo on the docs.

[WebKit Review Bot]

Comment on attachment 137061 [details]
Same as before, but fixes a typo on the docs.

Clearing flags on attachment: 137061

Committed r114113: <http://trac.webkit.org/changeset/114113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.