[WK2] Color chooser API missing

• Component: WebKit2
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

This is API allows the embedder to display a custom color chooser for <input type="color">.

[Pierre Rossi]

*** Bug 87496 has been marked as a duplicate of this bug. ***

[Thiago Marcos P. Santos]

Created attachment 145137 [details]
Patch

[Alexis Menard (darktears)]

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

You don't have Xcode to add the files for mac?

[Thiago Marcos P. Santos]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=145137) [details] [details]
> > Patch
> 
> You don't have Xcode to add the files for mac?

Unfortunately I don't.

[Thiago Marcos P. Santos]

Created attachment 145752 [details]
Patch v2

Thanks Alexis for adding the files to xcode buildsystem.

[Thiago Marcos P. Santos]

Created attachment 147284 [details]
Patch

Rebased.

[Anders Carlsson]

Comment on attachment 147284 [details]
Patch

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

According to http://www.webkit.org/coding/contributing.html, new files should have the WebKit license and not LGPL. Patch looks good otherwise.

> Source/WebKit2/UIProcess/PageClient.h:176
> +    virtual PassRefPtr<WebColorChooserProxy> createColorChooserProxy(WebPageProxy*, const WebCore::Color&) = 0;

I think it's good to name this parameter initialColor.

> Source/WebKit2/UIProcess/WebPageProxy.h:794
> +    void showColorChooser(const WebCore::Color&);

I think it's good to name this parameter initialColor.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:891
> +		E131CFA8157D1C9A002A66B3 /* WebColorChooser.cpp in CopyFiles */ = {isa = PBXBuildFile; fileRef = E131CFA6157D1C9A002A66B3 /* WebColorChooser.cpp */; };
> +		E131CFA9157D1C9A002A66B3 /* WebColorChooser.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E131CFA6157D1C9A002A66B3 /* WebColorChooser.cpp */; };
> +		E131CFAA157D1C9A002A66B3 /* WebColorChooser.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = E131CFA7157D1C9A002A66B3 /* WebColorChooser.h */; };
> +		E131CFAD157D1CD7002A66B3 /* WebColorChooserProxy.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = E131CFAC157D1CD7002A66B3 /* WebColorChooserProxy.h */; };

This part is incorrect. You have to add the files using Xcode and not edit the project file manually.

[Thiago Marcos P. Santos]

Created attachment 147418 [details]
Patch v4

Updated patch fixing what was pointed by the review.

[Gyuyoung Kim]

Comment on attachment 147418 [details]
Patch v4

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

[Thiago Marcos P. Santos]

Created attachment 147546 [details]
Patch v5

Now with stubs for all the ports, so it wont break the build for ports like EFL that has colorpicker enabled for wk1 and are building wk2.

[WebKit Review Bot]

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

[Noam Rosenthal]

Shouln't a new feature like this include some tests?

[Kenneth Rohde Christiansen]

It did as a separate bug report

[Noam Rosenthal]

(In reply to comment #13)
> It did as a separate bug report

Right, my oversight.

[Andreas Kling]

Comment on attachment 147546 [details]
Patch v5

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

r=me based on Anders's prior assessment and the patch looking generally fine to me.

> Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.h:45
> +    ~WebColorChooser();

Should mark this explicitly virtual.

[Thiago Marcos P. Santos]

Created attachment 148663 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148663 [details]
Patch

Clearing flags on attachment: 148663

Committed r120890: <http://trac.webkit.org/changeset/120890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.