Bug 95058

Summary: [WK2] Add color picker API support for WebKit2
Product: WebKit Reporter: KwangYong Choi <ky0.choi>
Component: WebKit2Assignee: KwangYong Choi <ky0.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, andersca, ap, cdumez, cgarcia, cmarcelo, gns, gyuyoung.kim, hausmann, kenneth, menard, mrobinson, rakuco, tmpsantos, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 91832    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
gyuyoung.kim: review+
Patch
none
Patch none

Description KwangYong Choi 2012-08-27 01:53:18 PDT
Add color picker API support for WebKit2
Comment 1 KwangYong Choi 2012-08-30 05:47:41 PDT
I implemented this feature to bug 91832.
Comment 2 KwangYong Choi 2012-09-03 05:13:02 PDT
Created attachment 161901 [details]
Patch

I think, it can be handled separately from bug 91832.

In this patch, there are two UI client callbacks for show/hide color picker. And one set API for setting selected color.
Comment 3 WebKit Review Bot 2012-09-03 05:16:30 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 WebKit Review Bot 2012-09-03 05:16:52 PDT
Attachment 161901 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/UIProcess/WebColorChooserProxy.h:65:  The parameter name "client" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 KwangYong Choi 2012-09-03 05:27:55 PDT
Created attachment 161904 [details]
Patch

Fixed style.
Comment 6 Gyuyoung Kim 2012-09-03 06:09:33 PDT
Comment on attachment 161904 [details]
Patch

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

> Source/WebKit2/Shared/APIObject.h:111
> +        TypeColorPickerResultListener,

Wrong alphabetical order.

> Source/WebKit2/UIProcess/WebColorChooserProxy.h:65
> +    WebColorChooserProxy(Client*);

Missing explicit keyword.

> Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h:53
> +    WebColorPickerResultListenerProxy(WebPageProxy*);

ditto.
Comment 7 KwangYong Choi 2012-09-03 17:36:05 PDT
Created attachment 161950 [details]
Patch

Fixed coding style and added explicit keyword.
Comment 8 Chris Dumez 2012-09-03 23:15:38 PDT
Comment on attachment 161950 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKColorPickerResultListener.cpp:39
> +void WKColorPickerResultListenerColorSet(WKColorPickerResultListenerRef listenerRef, const WKStringRef color)

WKColorPickerResultListenerColorSet -> WKColorPickerResultListenerSetColor ? We shouldn't use Yoda style here, this isn't EFL :)
Comment 9 KwangYong Choi 2012-09-03 23:48:17 PDT
Created attachment 161973 [details]
Patch

WKColorPickerResultListenerColorSet -> WKColorPickerResultListenerSetColor
Comment 10 Gyuyoung Kim 2012-09-07 04:33:38 PDT
Comment on attachment 161973 [details]
Patch

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

It looks showColorPicker and hideColorPicker are duplicate with Bug 91832. Do you explain what is difference between this bug and Bug 91832 ?

> Source/WebKit2/UIProcess/API/C/WKColorPickerResultListener.cpp:35
> +{

Missing #if ENABLE() macro.
Comment 11 KwangYong Choi 2012-09-07 05:19:57 PDT
(In reply to comment #10)
> (From update of attachment 161973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161973&action=review
> 
> It looks showColorPicker and hideColorPicker are duplicate with Bug 91832. Do you explain what is difference between this bug and Bug 91832 ?

This patch has only WK2 C API part. I will change bug 91832 to use this patch. It's EFL API implementation.

> 
> > Source/WebKit2/UIProcess/API/C/WKColorPickerResultListener.cpp:35
> > +{
> 
> Missing #if ENABLE() macro.

I think, WKColorPickerResultListenerGetTypeID() should return TypeColorPickerResultListener always even though ENABLE(COLOR_INPUT_TYPE) is not defined. So I'm not using ENABLE() macro.

WKColorPickerResultListenerGetTypeID() may return 0 instead of TypeColorPickerResultListener?
Comment 12 KwangYong Choi 2012-09-07 05:49:52 PDT
Created attachment 162751 [details]
Patch

Added #if ENABLE()
Comment 13 Gyuyoung Kim 2012-09-09 23:00:48 PDT
Looks find on my side. Simon, could you take a look this patch ?
Comment 14 Gyuyoung Kim 2012-09-12 01:49:29 PDT
If there is no comment by tomorrow, I'd like to set r+.
Comment 15 WebKit Review Bot 2012-09-13 18:33:36 PDT
Comment on attachment 162751 [details]
Patch

Rejecting attachment 162751 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
hing file Source/WebKit2/win/WebKit2.vcproj
patching file Source/WebKit2/win/WebKit2Generated.make
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/MiniBrowser/mac/BrowserWindowController.m
patching file Tools/MiniBrowser/win/BrowserView.cpp
patching file Tools/WebKitTestRunner/TestController.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gyuyoung K..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13845413
Comment 16 KwangYong Choi 2012-09-13 19:52:23 PDT
Created attachment 164032 [details]
Patch

Rebased.
Comment 17 Gyuyoung Kim 2012-09-13 19:54:39 PDT
Comment on attachment 164032 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKBase.h:127
> +typedef const struct OpaqueWKColorPickerResultListener* WKColorPickerResultListenerRef;

Nit: Wrong alphabetical order.
Comment 18 KwangYong Choi 2012-09-13 20:14:30 PDT
Created attachment 164035 [details]
Patch

Fixed alphabetical order.
Comment 19 WebKit Review Bot 2012-09-13 22:08:23 PDT
Comment on attachment 164035 [details]
Patch

Clearing flags on attachment: 164035

Committed r128553: <http://trac.webkit.org/changeset/128553>
Comment 20 WebKit Review Bot 2012-09-13 22:08:29 PDT
All reviewed patches have been landed.  Closing bug.