Bug 83692 - [EFL] Add API for color chooser
Summary: [EFL] Add API for color chooser
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: Thiago Marcos P. Santos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-11 08:32 PDT by Thiago Marcos P. Santos
Modified: 2012-04-13 04:44 PDT (History)
7 users (show)

See Also:


Attachments
Adds color chooser API to EFL port (21.18 KB, patch)
2012-04-12 08:41 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Added Ewk_Color structure. (22.04 KB, patch)
2012-04-12 12:22 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Same as before, but fixes a typo on the docs. (22.04 KB, patch)
2012-04-13 01:49 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-04-11 08:32:01 PDT
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
Comment 1 Thiago Marcos P. Santos 2012-04-12 04:03:35 PDT
Depends on this 82867 for testing.
Comment 2 Thiago Marcos P. Santos 2012-04-12 07:22:13 PDT
Not really a dependency. My mistake.
Comment 3 Thiago Marcos P. Santos 2012-04-12 08:41:30 PDT
Created attachment 136918 [details]
Adds color chooser API to EFL port
Comment 4 Thiago Marcos P. Santos 2012-04-12 08:57:29 PDT
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?
Comment 5 Antonio Gomes 2012-04-12 09:07:17 PDT
(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?
Comment 6 Thiago Marcos P. Santos 2012-04-12 09:09:04 PDT
(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)
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-04-12 09:12:56 PDT
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.
Comment 8 Rob Buis 2012-04-12 09:21:44 PDT
(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.
Comment 9 Thiago Marcos P. Santos 2012-04-12 10:01:24 PDT
(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. :)
Comment 10 Thiago Marcos P. Santos 2012-04-12 12:22:30 PDT
Created attachment 136953 [details]
Added Ewk_Color structure.
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-04-12 15:29:17 PDT
Comment on attachment 136953 [details]
Added Ewk_Color structure.

Informal r+, thanks for the patch.
Comment 12 Thiago Marcos P. Santos 2012-04-13 01:49:55 PDT
Created attachment 137061 [details]
Same as before, but fixes a typo on the docs.
Comment 13 WebKit Review Bot 2012-04-13 04:44:41 PDT
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>
Comment 14 WebKit Review Bot 2012-04-13 04:44:46 PDT
All reviewed patches have been landed.  Closing bug.