Bug 119030

Summary: Move platform-specific implementation of the color picker
Product: WebKit Reporter: Ruth Fong <ruthiecftg>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WORKSFORME    
Severity: Normal CC: beidson, gyuyoung.kim, jonlee, ky0.choi, ruthiecftg, ryuan.choi, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61276    
Attachments:
Description Flags
Patch ap: review-

Ruth Fong
Reported 2013-07-23 17:04:38 PDT
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.
Attachments
Patch (8.91 KB, patch)
2013-07-24 18:22 PDT, Ruth Fong 		ap: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-07-23 17:06:01 PDT
<rdar://problem/14528215>
Ruth Fong
Comment 2 2013-07-24 18:22:03 PDT
Created attachment 207425 [details] Patch
Alexey Proskuryakov
Comment 3 2013-07-24 18:58:32 PDT
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
Comment 4 2013-07-24 18:58:53 PDT
latter *case*
Ruth Fong
Comment 5 2013-07-24 21:43:36 PDT
(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
Comment 6 2013-07-25 00:12:01 PDT
(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
Comment 7 2013-07-25 09:25:35 PDT
(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. :)
Note You need to log in before you can comment on or make changes to this bug.