Bug 119030 – Move platform-specific implementation of the color picker

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Forms

[Ruth Fong]

In bug 95058, a first WK2 implementation for <input type='color'> was outlined. However, in bug 61276, there will be some architectural changes that require changing the assumptions of the previous implementation.

Most notably, bug 61276 does not assume that WebPageProxy::showColorChooser is only called when m_colorChooser is null. showColorChooser will be called anytime an <input type='color'> element is activated (in ColorInputType.cpp).

To keep WebPageProxy cross-platform and to continue supporting the implementation put in place in bug 95058 as best as possible, parts of showColorChooser will be refactored into new private function like boolean useLegacyColorPicker() that will return false in WebPageProxy. A platform-specific WebPageProxySamsung will be created and the refactored useLegacyColorPicker() will be moved here.

[Radar WebKit Bug Importer]

<rdar://problem/14528215>

[Ruth Fong]

Created attachment 207425 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 207425 [details]
Patch

Adding the new file to xcodeproj is not right - I think that it would need be added to Source/WebKit2/PlatformEfl.cmake if we agreed that it's platform specific.

But I don't think that the code is platform specific. It just does things in a way that's either different or plain incorrect. In the former case, we should some up with a better way to describe and conditionalize it. In the latter way, it should be replaced with correct code (this is WebKit2).

[Alexey Proskuryakov]

latter *case*

[Ruth Fong]

(In reply to comment #3)
> (From update of attachment 207425 [details])
> Adding the new file to xcodeproj is not right - I think that it would need be added to Source/WebKit2/PlatformEfl.cmake if we agreed that it's platform specific.
> 
> But I don't think that the code is platform specific. It just does things in a way that's either different or plain incorrect. In the former case, we should some up with a better way to describe and conditionalize it. In the latter way, it should be replaced with correct code (this is WebKit2).

The current implementation assumes that only one <input type='color'> is on a page, which is not a sustainable model (look at WebPageProxy::showColorChooser. This assumes that m_colorPicker is null when it's called. 

However, showColorChooser is called whenever an <input type='color'> object is activated (in ColorInputType::handleDOMActivateEvent). If there are two <input type='color'> elements and one is first clicked on and then the second, then showColorChooser will be called for the second, but m_colorPicker will not be null.

This should be changed to be able to handle multiple <input type='color'> elements, and another patch is tracking that (bug 61276).

Also, it's unnecessary to use the WebColorPickerResultListenerProxy object and m_uiClient. Once m_colorPicker is created and set properly, it should be able to handle receiving user input of color selection and communicating back down to the WebPageProxy level (see the proposed patch for bug 61276).

[Ryuan Choi]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 207425 [details] [details])
> > Adding the new file to xcodeproj is not right - I think that it would need be added to Source/WebKit2/PlatformEfl.cmake if we agreed that it's platform specific.
> > 
> > But I don't think that the code is platform specific. It just does things in a way that's either different or plain incorrect. In the former case, we should some up with a better way to describe and conditionalize it. In the latter way, it should be replaced with correct code (this is WebKit2).
> 
> The current implementation assumes that only one <input type='color'> is on a page, which is not a sustainable model (look at WebPageProxy::showColorChooser. This assumes that m_colorPicker is null when it's called. 
> 
> However, showColorChooser is called whenever an <input type='color'> object is activated (in ColorInputType::handleDOMActivateEvent). If there are two <input type='color'> elements and one is first clicked on and then the second, then showColorChooser will be called for the second, but m_colorPicker will not be null.
> 
> This should be changed to be able to handle multiple <input type='color'> elements, and another patch is tracking that (bug 61276).
> 
> Also, it's unnecessary to use the WebColorPickerResultListenerProxy object and m_uiClient. Once m_colorPicker is created and set properly, it should be able to handle receiving user input of color selection and communicating back down to the WebPageProxy level (see the proposed patch for bug 61276).

If current logic is bad (I think so), probably I can refactor EFL side like bug 61276 within 1~2 days.

Anyway, "Samsung" in folder/file name looks not good to me. :)
Samsung is not a platform.

[Ruth Fong]

(In reply to comment #6)
> 
> If current logic is bad (I think so), probably I can refactor EFL side like bug 61276 within 1~2 days.
> 
> Anyway, "Samsung" in folder/file name looks not good to me. :)
> Samsung is not a platform.

Sounds good. I'll close this bug then and go ahead with 61276. :)