Bug 119025

Summary: Rename WebColorChooserProxy
Product: WebKit Reporter: Ruth Fong <ruthiecftg>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, beidson, cdumez, cgarcia, cmarcelo, commit-queue, eflews.bot, gustavo, gyuyoung.kim, jonlee, luiz, menard, mrobinson, noam, rakuco, ruthiecftg, webkit-bug-importer, webkit-ews, zeno
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61276    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Ruth Fong 2013-07-23 16:52:09 PDT
WebColorChooserProxy, which represents the color picker UI for an <input type='color'> element, suggests a one-to-one relationship with the WebColorChooser object. 

In bug 61276, the architecture will change so that only one  WebColorChooserProxy object exists for a given WebPageProxy, while there can be multiple WebColorChooser objects living in the WebProcess.

WebColorChooserProxy should be renamed to something like WebColorPickerUI that reflects its representation of the color picker UI.
Comment 1 Radar WebKit Bug Importer 2013-07-23 16:53:07 PDT
<rdar://problem/14528039>
Comment 2 Ruth Fong 2013-07-24 14:08:22 PDT
Created attachment 207410 [details]
Patch
Comment 3 Early Warning System Bot 2013-07-24 14:12:56 PDT
Comment on attachment 207410 [details]
Patch

Attachment 207410 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1200027
Comment 4 EFL EWS Bot 2013-07-24 14:15:04 PDT
Comment on attachment 207410 [details]
Patch

Attachment 207410 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1205034
Comment 5 Brady Eidson 2013-07-24 14:16:18 PDT
Comment on attachment 207410 [details]
Patch

Should change it in all the build systems.

`grep -r WebColorChooserProxy.cpp <path/to/WebKit2>` should find everywhere you need to replace it.
Comment 6 Tim Horton 2013-07-24 14:17:11 PDT
Comment on attachment 207410 [details]
Patch

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

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

This method is poorly named now, isn't it?
Comment 7 Brady Eidson 2013-07-24 14:25:06 PDT
(In reply to comment #6)
> (From update of attachment 207410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207410&action=review
> 
> > Source/WebKit2/UIProcess/PageClient.h:190
> > +    virtual PassRefPtr<WebColorPickerUI> createColorChooserProxy(WebPageProxy*, const WebCore::Color& initialColor, const WebCore::IntRect&) = 0;
> 
> This method is poorly named now, isn't it?


And is there any advantage to calling the new object WebColorPickerUI instead of WebColorPicker, considering that the abstract concept of a WebColorPicker encapsulates more than just its UI?
Comment 8 Ruth Fong 2013-07-24 14:42:05 PDT
Comment on attachment 207410 [details]
Patch

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

>>> Source/WebKit2/UIProcess/PageClient.h:190
>>> +    virtual PassRefPtr<WebColorPickerUI> createColorChooserProxy(WebPageProxy*, const WebCore::Color& initialColor, const WebCore::IntRect&) = 0;
>> 
>> This method is poorly named now, isn't it?
> 
> And is there any advantage to calling the new object WebColorPickerUI instead of WebColorPicker, considering that the abstract concept of a WebColorPicker encapsulates more than just its UI?

I think that the WebColorPicker(UI) object does represent UI representation of the color picker, whereas the WebColorChooser is the abstract representation for the <input type='color'> object.
Comment 9 Ruth Fong 2013-07-24 14:52:56 PDT
Created attachment 207412 [details]
Patch
Comment 10 WebKit Commit Bot 2013-07-24 14:56:06 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 11 Ruth Fong 2013-07-24 16:07:03 PDT
Created attachment 207417 [details]
Patch
Comment 12 Brady Eidson 2013-07-24 16:55:55 PDT
Comment on attachment 207417 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        Rename WebColorChooserProxy to WebColorPicker since the name WebColorChoooserProxy
> +        implies a one-to-one relationship with WebColorChooser, which will not be true when
> +        the patch for bug 61276 lands.

Nitpick - It's not even true *now*

Yes, the WebProcess has 1 WebColorChooser, and the UIProcess has 1 WebColorChooserProxy...  but they aren't actually connected.

The Object/ObjectProxy naming scheme is for a pair of objects that represent the identical object/concept on both sides of the IPC connection, so tightly intertwined that they message each other directly.
Comment 13 Ruth Fong 2013-07-24 17:02:10 PDT
Created attachment 207422 [details]
Patch
Comment 14 WebKit Commit Bot 2013-07-24 17:03:46 PDT
Comment on attachment 207422 [details]
Patch

Rejecting attachment 207422 [details] from commit-queue.

ruthiecftg@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 15 WebKit Commit Bot 2013-07-24 18:07:17 PDT
Comment on attachment 207422 [details]
Patch

Clearing flags on attachment: 207422

Committed r153108: <http://trac.webkit.org/changeset/153108>
Comment 16 WebKit Commit Bot 2013-07-24 18:07:23 PDT
All reviewed patches have been landed.  Closing bug.