Bug 87495

Summary: [WK2] Color chooser API missing
Product: WebKit Reporter: Thiago Marcos P. Santos <tmpsantos>
Component: WebKit2Assignee: Thiago Marcos P. Santos <tmpsantos>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cgarcia, cmarcelo, dinu.jacob, gustavo, hausmann, kenneth, menard, mrobinson, noam, pierre.rossi, rakuco, sam, vestbo, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87496, 87749, 87989    
Attachments:
Description Flags
Patch
none
Patch v2
none
Patch
none
Patch v4
none
Patch v5
none
Patch none

Thiago Marcos P. Santos
Reported 2012-05-25 06:04:36 PDT
This is API allows the embedder to display a custom color chooser for <input type="color">.
Attachments
Patch (25.55 KB, patch)
2012-05-31 12:48 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch v2 (30.78 KB, patch)
2012-06-05 03:58 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch (30.81 KB, patch)
2012-06-13 04:11 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch v4 (32.29 KB, patch)
2012-06-13 14:57 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch v5 (41.77 KB, patch)
2012-06-14 03:37 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch (41.79 KB, patch)
2012-06-20 15:09 PDT, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Pierre Rossi
Comment 1 2012-05-29 08:23:48 PDT
*** Bug 87496 has been marked as a duplicate of this bug. ***
Thiago Marcos P. Santos
Comment 2 2012-05-31 12:48:08 PDT
Created attachment 145137 [details] Patch
Alexis Menard (darktears)
Comment 3 2012-06-04 05:40:42 PDT
(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
Comment 4 2012-06-04 05:50:52 PDT
(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
Comment 5 2012-06-05 03:58:10 PDT
Created attachment 145752 [details] Patch v2 Thanks Alexis for adding the files to xcode buildsystem.
Thiago Marcos P. Santos
Comment 6 2012-06-13 04:11:33 PDT
Created attachment 147284 [details] Patch Rebased.
Anders Carlsson
Comment 7 2012-06-13 09:12:21 PDT
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
Comment 8 2012-06-13 14:57:01 PDT
Created attachment 147418 [details] Patch v4 Updated patch fixing what was pointed by the review.
Gyuyoung Kim
Comment 9 2012-06-13 17:21:13 PDT
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
Comment 10 2012-06-14 03:37:00 PDT
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
Comment 11 2012-06-14 03:38:36 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
Noam Rosenthal
Comment 12 2012-06-15 06:42:21 PDT
Shouln't a new feature like this include some tests?
Kenneth Rohde Christiansen
Comment 13 2012-06-15 06:46:10 PDT
It did as a separate bug report
Noam Rosenthal
Comment 14 2012-06-15 07:09:19 PDT
(In reply to comment #13) > It did as a separate bug report Right, my oversight.
Andreas Kling
Comment 15 2012-06-20 13:02:27 PDT
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
Comment 16 2012-06-20 15:09:08 PDT
Created attachment 148663 [details] Patch
WebKit Review Bot
Comment 17 2012-06-20 17:44:19 PDT
Comment on attachment 148663 [details] Patch Clearing flags on attachment: 148663 Committed r120890: <http://trac.webkit.org/changeset/120890>
WebKit Review Bot
Comment 18 2012-06-20 17:44:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.