WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
65897
Implement <input type=color> UI WebKit chromium part
https://bugs.webkit.org/show_bug.cgi?id=65897
Summary
Implement <input type=color> UI WebKit chromium part
Keishi Hattori
Reported
2011-08-08 21:08:31 PDT
implement ChromeClient::openColorChooser, closeColorChooser, and setSelectedColorInColorChooser for WebKit chromium
Attachments
patch
(14.88 KB, patch)
2011-08-09 20:48 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
wrapped in flag
(15.01 KB, patch)
2011-08-09 22:19 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
added comments
(15.88 KB, patch)
2011-08-10 01:54 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
changed selectedColor to colorSelected
(15.88 KB, patch)
2011-08-11 19:12 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
fixed patch
(17.14 KB, patch)
2011-09-01 06:22 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
rebased
(17.14 KB, patch)
2011-09-27 06:33 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
Patch
(19.47 KB, patch)
2011-10-17 23:49 PDT
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
Patch
(20.28 KB, patch)
2011-11-06 23:22 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
Patch
(20.26 KB, patch)
2011-11-07 03:15 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
patch using WebColorChooserSession
(16.10 KB, patch)
2011-12-01 21:54 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
new patch
(39.30 KB, patch)
2011-12-12 03:39 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
new patch(webcore side moved to separate patch)
(26.54 KB, patch)
2011-12-15 01:56 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
fixed
(26.37 KB, patch)
2011-12-15 04:12 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
updated pbxproj and GNUmakefile
(32.10 KB, patch)
2011-12-18 20:18 PST
,
Keishi Hattori
no flags
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Keishi Hattori
Comment 1
2011-08-09 20:48:51 PDT
Created
attachment 103436
[details]
patch
WebKit Review Bot
Comment 2
2011-08-09 20:54:22 PDT
Comment on
attachment 103436
[details]
patch
Attachment 103436
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/9339463
Keishi Hattori
Comment 3
2011-08-09 21:00:44 PDT
(In reply to
comment #2
)
> (From update of
attachment 103436
[details]
(apply) [details]) >
Attachment 103436
[details]
(apply) [details] did not pass chromium-ews (chromium-xvfb): > Output:
http://queues.webkit.org/results/9339463
I forgot to wrap some parts with the flag. Fixing now.
WebKit Review Bot
Comment 4
2011-08-09 21:10:25 PDT
Comment on
attachment 103436
[details]
patch
Attachment 103436
[details]
did not pass cr-mac-ews (chromium): Output:
http://queues.webkit.org/results/9337487
Keishi Hattori
Comment 5
2011-08-09 22:19:58 PDT
Created
attachment 103442
[details]
wrapped in flag
Kent Tamura
Comment 6
2011-08-10 01:14:50 PDT
Comment on
attachment 103442
[details]
wrapped in flag View in context:
https://bugs.webkit.org/attachment.cgi?id=103442&action=review
> Source/WebKit/chromium/public/WebColorChooserListener.h:38 > + > +class WebColorChooserListener {
Would you write the purpose of this class here? A user of Chromium WebKi API needs to know how to use this class.
> Source/WebKit/chromium/public/WebViewClient.h:184 > + > + virtual void openColorChooser(WebColor, WebColorChooserListener*) { } > + virtual void closeColorChooser() { } > + virtual void setSelectedColorInColorChooser(WebColor) { }
Would you write descriptions what should be done in these functions?
Kent Tamura
Comment 7
2011-08-10 01:28:30 PDT
BTW, will we have multiple color chooser dialogs if multiple tabs have color input fields?
Kent Tamura
Comment 8
2011-08-10 01:28:55 PDT
Adding fishd for WebKit API change.
Keishi Hattori
Comment 9
2011-08-10 01:54:38 PDT
Created
attachment 103454
[details]
added comments
Keishi Hattori
Comment 10
2011-08-10 01:55:36 PDT
(In reply to
comment #7
)
> BTW, will we have multiple color chooser dialogs if multiple tabs have color input fields?
No. If the color chooser is already open because of a color input from another tab, the color chooser's listener is updated to the new one and the color chooser window regains focus.
Kent Tamura
Comment 11
2011-08-10 02:06:44 PDT
(In reply to
comment #10
)
> (In reply to
comment #7
) > > BTW, will we have multiple color chooser dialogs if multiple tabs have color input fields? > > No. If the color chooser is already open because of a color input from another tab, the color chooser's listener is updated to the new one and the color chooser window regains focus.
So, a color chooser dialog implementation in the browser process counts the number of WebViews which are asking to open it, and doesn't close the dialog if a WebView asked to close it but another WebView doesn't ask to close it yet. Right?
Keishi Hattori
Comment 12
2011-08-10 02:24:30 PDT
(In reply to
comment #11
)
> So, a color chooser dialog implementation in the browser process counts the number of WebViews which are asking to open it, and doesn't close the dialog if a WebView asked to close it but another WebView doesn't ask to close it yet. Right?
Not quite. The dialog closes if the web view containing the "last clicked color input element" asks. I think what users expect when they click is for the old color chooser to close and the new color chooser to open.
Kent Tamura
Comment 13
2011-08-11 19:05:14 PDT
LGTM. fishd, Would you review WebKit API changes please?
Keishi Hattori
Comment 14
2011-08-11 19:12:20 PDT
Created
attachment 103723
[details]
changed selectedColor to colorSelected
Keishi Hattori
Comment 15
2011-08-11 19:13:22 PDT
(In reply to
comment #14
)
> Created an attachment (id=103723)(apply) [details] > changed selectedColor to colorSelected
Sorry. WebColorChooserListener::selectedColor should have been colorSelected.
Keishi Hattori
Comment 16
2011-08-12 06:02:05 PDT
Here are the patches on the chromium side. common for every platform part:
http://codereview.chromium.org/7633013
mac specific part:
http://codereview.chromium.org/7628016
Keishi Hattori
Comment 17
2011-08-21 19:45:17 PDT
(In reply to
comment #16
)
> Here are the patches on the chromium side.
Here are the new URLs. Implement input type=color UI (common part)
http://codereview.chromium.org/7685006/
Implement input type=color UI (mac part)
http://codereview.chromium.org/7687006/
Implement input type=color UI (gtk part)
http://codereview.chromium.org/7696018/
Implement input type=color UI (win part)
http://codereview.chromium.org/7690004/
Darin Fisher (:fishd, Google)
Comment 18
2011-08-30 00:35:15 PDT
Comment on
attachment 103723
[details]
changed selectedColor to colorSelected View in context:
https://bugs.webkit.org/attachment.cgi?id=103723&action=review
> Source/WebKit/chromium/public/WebColorChooserListener.h:39 > +class WebColorChooserListener {
please add a protected destructor, just like WebFileChooserCompletion. also, i think it would be helpful to use the same naming convention as other interfaces. instead of a Listener, call this a Completion. WebColorChooserCompletion
> Source/WebKit/chromium/public/WebColorChooserListener.h:41 > + // Called when user selects a color. On the Mac this would be called every
nit: "Called when [the] user selects a color." I would be careful about documenting platform specific behavior here. That can change as new versions of the OS are released. I wouldn't be surprised to find Linux desktops behaving like Macs in the future. Maybe this comment should just say something more generic, like "Depending on the platform, this method may either be called multiple times to preview the selected color, or just once when the user has made their final color choice."
> Source/WebKit/chromium/public/WebColorChooserListener.h:44 > + virtual void colorSelected(WebColor) = 0;
nit: we use the convention of prefix-naming event methods either did* or will*, and so in this case I think didSelectColor would be best. actually, to be like WebFileChooserCompletion, I'd call this didChooseColor.
> Source/WebKit/chromium/public/WebViewClient.h:179 >
nit: please preserve the two new line whitespace gap above section separators.
> Source/WebKit/chromium/public/WebViewClient.h:185 > + virtual void openColorChooser(WebColor, WebColorChooserListener*) { }
nit: I think you should name the first parameter defaultColor. That should make the code more self-documenting. nit: call this runColorChooser, just like runFileChooser
> Source/WebKit/chromium/public/WebViewClient.h:187 > + // This method tries to closes the color chooser interface. The color
nit: "tries to closes" -> "tries to close" no need to say "interface" here. it is sufficient to refer to it as a "color chooser".
> Source/WebKit/chromium/public/WebViewClient.h:188 > + // chooser would not close if the color chooser's listener is is tied to
nit: "is is" I'm having a hard time understanding this last sentence. It seems like this method can only refer to a color chooser that was created by this WebView. So, how can "the color chooser's listener" be tied to another tab? I'm very confused.
> Source/WebKit/chromium/public/WebViewClient.h:197 > // Dialogs -------------------------------------------------------------
nit: please preserve the two new line whitespace gap above section separators.
Keishi Hattori
Comment 19
2011-09-01 06:22:25 PDT
Created
attachment 105949
[details]
fixed patch
Keishi Hattori
Comment 20
2011-09-01 06:22:44 PDT
(In reply to
comment #18
)
> (From update of
attachment 103723
[details]
(apply) [details]) > View in context:
https://bugs.webkit.org/attachment.cgi?id=103723&action=review
> > > Source/WebKit/chromium/public/WebColorChooserListener.h:39 > > +class WebColorChooserListener { > > please add a protected destructor, just like WebFileChooserCompletion.
Done.
> also, i think it would be helpful to use the same naming convention as > other interfaces. instead of a Listener, call this a Completion. > > WebColorChooserCompletion
Done.
> > Source/WebKit/chromium/public/WebColorChooserListener.h:41 > > + // Called when user selects a color. On the Mac this would be called every > > nit: "Called when [the] user selects a color."
Done.
> I would be careful about documenting platform specific behavior here. That > can change as new versions of the OS are released. I wouldn't be surprised > to find Linux desktops behaving like Macs in the future. Maybe this comment > should just say something more generic, like "Depending on the platform, this > method may either be called multiple times to preview the selected color, or > just once when the user has made their final color choice."
Done.
> > Source/WebKit/chromium/public/WebColorChooserListener.h:44 > > + virtual void colorSelected(WebColor) = 0; > > nit: we use the convention of prefix-naming event methods either did* or will*, and > so in this case I think didSelectColor would be best. > > actually, to be like WebFileChooserCompletion, I'd call this didChooseColor.
Done.
> > Source/WebKit/chromium/public/WebViewClient.h:179 > > > > nit: please preserve the two new line whitespace gap above section separators.
Done.
> > Source/WebKit/chromium/public/WebViewClient.h:185 > > + virtual void openColorChooser(WebColor, WebColorChooserListener*) { } > > nit: I think you should name the first parameter defaultColor. That should make > the code more self-documenting.
Done.
> nit: call this runColorChooser, just like runFileChooser
I didn't do this because "open" would match all the method names up to ColorChooserImple::open() in Chromium.
> > Source/WebKit/chromium/public/WebViewClient.h:187 > > + // This method tries to closes the color chooser interface. The color > > nit: "tries to closes" -> "tries to close"
Done.
> no need to say "interface" here. it is sufficient to refer to it as a "color chooser".
Done.
> > Source/WebKit/chromium/public/WebViewClient.h:188 > > + // chooser would not close if the color chooser's listener is is tied to > > nit: "is is"
Done.
> I'm having a hard time understanding this last sentence. It seems like this > method can only refer to a color chooser that was created by this WebView. > So, how can "the color chooser's listener" be tied to another tab? I'm very > confused.
There will only be one color chooser in chromium. But each tab has RenderViews. If
> > Source/WebKit/chromium/public/WebViewClient.h:197 > > // Dialogs ------------------------------------------------------------- > > nit: please preserve the two new line whitespace gap above section separators.
Done.
Keishi Hattori
Comment 21
2011-09-27 06:33:04 PDT
Created
attachment 108838
[details]
rebased
Darin Fisher (:fishd, Google)
Comment 22
2011-10-05 09:52:12 PDT
Comment on
attachment 108838
[details]
rebased View in context:
https://bugs.webkit.org/attachment.cgi?id=108838&action=review
> Source/WebKit/chromium/public/WebColorChooserCompletion.h:44 > + virtual void didChooseColor(WebColor) = 0;
How do we know when we can delete the WebColorChooserCompletion instance? In the case of WebFileChooserCompletion, we know that we can delete the instance when didChooseFile is called because it will only be called once. Since you need to support many calls to didChooseColor, it seems like you need a finalization method. You seem to be getting around this by making the WebColorChooserCompletion be allocated as a singleton.
> Source/WebKit/chromium/public/WebViewClient.h:188 > + // This method does cleanup on the color chooser interface.
I think it would be much cleaner to define a WebColorChooser interface, and on that interface you could put methods like "close()" and "setSelectedColor()". I think you should hide the detail that Chrome will only show one color chooser globally for all tabs at any given time. The API here should not mandate such an implementation. How about a design like this: class WebViewClient { public: ... virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } }; class WebColorChooser { public: virtual ~WebColorChooser() {} // Destroys the color chooser. virtual void setSelectedColor(WebColor) = 0; // Updates the color shown in the chooser. }; class WebColorChooserClient { public: virtual void didChooseColor(WebColor) = 0; // Can be called multiple times to preview the chosen color. Can be reverted via didCancel. virtual void didAccept() = 0; // Done choosing color, and user accepts chosen color. virtual void didCancel() = 0; // Done choosing color, and user wants to revert back to the original color. protected: ~WebColorChooserClient() {} }; In this design, the WebColorChooser should be destroyed by the client once didAccept or didCancel is called, and at that point it is safe to destroy the WebColorChooserClient instance. If setSelectedColor is called after didAccept/didCancel, then it will be ignored. An alternative design might add methods on WebColorChooser to show and hide the dialog, and then the Client interface might have didShow and didHide methods. It might also need a didRevert method to indicate when the previewed color choice should be reverted.
Keishi Hattori
Comment 23
2011-10-17 23:49:10 PDT
Created
attachment 111393
[details]
Patch
Keishi Hattori
Comment 24
2011-10-17 23:49:39 PDT
I agree with your design and I have decided to go with WebColorChooser+WebColorChooserClient and stopped using singletons inside of WebKit. I do not need to revert the selected color for any of the platforms, so the methods are different from what you suggested. The WebColorChooserClient instance and WebColorChooser instance will be destroyed when didCleanup is called.
WebKit Review Bot
Comment 25
2011-10-17 23:51:52 PDT
Please wait for approval from
fishd@chromium.org
before submitting because this patch contains changes to the Chromium public API.
Keishi Hattori
Comment 26
2011-10-18 00:46:12 PDT
> class WebViewClient { > public: > ... > virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } > };
I implemented WebViewClient::colorChooser() so that it creates a new color chooser if it doesn't exist already. Maybe I should rename it to WebViewClient::createColorChooserIfNecessary ?
Keishi Hattori
Comment 27
2011-11-06 23:22:25 PST
Created
attachment 113826
[details]
Patch
Keishi Hattori
Comment 28
2011-11-06 23:25:09 PST
This new patch is for when the patch for
Bug 71644
lands. WebCore::ColorChooser will no longer be a singleton.
Keishi Hattori
Comment 29
2011-11-07 03:15:11 PST
Created
attachment 113841
[details]
Patch
Keishi Hattori
Comment 30
2011-11-07 17:38:44 PST
I updated the chromium side to reflect this change
http://codereview.chromium.org/7685006/
Darin Fisher (:fishd, Google)
Comment 31
2011-11-14 23:20:33 PST
Comment on
attachment 113841
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=113841&action=review
> Source/WebKit/chromium/public/WebColorChooserClient.h:51 > + virtual void didCleanup() = 0;
nit: I think using a term like "Close" here might be a bit clearer. It seems like there are two events that you can receive from the color chooser. Either, the color was changed or the dialog was closed.
> Source/WebKit/chromium/src/ChromeClientImpl.cpp:673 > + client->colorChooser()->setClient(colorChooserClient);
It seems like it would be better to create a new WebColorChooser for use by WebKit. It may be the case that you only ever want to make one color chooser visible to the user at a time, but by allocating a separate WebColorChooser for each openColorChooser call, you simplify some of the state management. For example, your checks for "colorChooser == client->colorChooser()->client()->colorChooser()" go away. Instead, when WebKit wants to close the WebColorChooser instance, it just deletes it. When WebKit wants to change the color shown in its WebColorChooser instance, it just calls a method on it. So, I'd go with: class WebViewClient { ... virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } ... };
Keishi Hattori
Comment 32
2011-11-15 00:48:51 PST
(In reply to
comment #31
)
> (From update of
attachment 113841
[details]
(apply) [details]) > View in context:
https://bugs.webkit.org/attachment.cgi?id=113841&action=review
> > > Source/WebKit/chromium/public/WebColorChooserClient.h:51 > > + virtual void didCleanup() = 0; > > nit: I think using a term like "Close" here might be a bit clearer. It seems like > there are two events that you can receive from the color chooser. Either, the > color was changed or the dialog was closed.
I prefer "Close" too. But calling this won't close the color chooser panel on OS X. Darin Adler said that Apple's UI guideline says that an application should never close the color chooser panel(only the user can). And we will follow this guideline for OS X. I couldn't come up with a suitable term. Do you still think that "Close" is better?
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:673 > > + client->colorChooser()->setClient(colorChooserClient); > > It seems like it would be better to create a new WebColorChooser for use by WebKit. > It may be the case that you only ever want to make one color chooser visible to the > user at a time, but by allocating a separate WebColorChooser for each openColorChooser > call, you simplify some of the state management. For example, your checks for > "colorChooser == client->colorChooser()->client()->colorChooser()" go away. Instead, > when WebKit wants to close the WebColorChooser instance, it just deletes it. When > WebKit wants to change the color shown in its WebColorChooser instance, it just calls > a method on it. > > So, I'd go with: > > class WebViewClient { > ... > virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } > ... > };
This won't work because WebCore::ColorChooser is no longer a singleton
http://trac.webkit.org/changeset/99403
There may be multiple input color elements on the page and they can call ChromeClientImpl::cleanupColorChooser even when it is not the one that opened the color picker.
Darin Fisher (:fishd, Google)
Comment 33
2011-11-29 22:13:15 PST
(In reply to
comment #32
)
> (In reply to
comment #31
) > > (From update of
attachment 113841
[details]
(apply) [details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=113841&action=review
> > > > > Source/WebKit/chromium/public/WebColorChooserClient.h:51 > > > + virtual void didCleanup() = 0; > > > > nit: I think using a term like "Close" here might be a bit clearer. It seems like > > there are two events that you can receive from the color chooser. Either, the > > color was changed or the dialog was closed. > > I prefer "Close" too. But calling this won't close the color chooser panel on OS X. > > Darin Adler said that Apple's UI guideline says that an application should never close the color chooser panel(only the user can). And we will follow this guideline for OS X. > > I couldn't come up with a suitable term. Do you still think that "Close" is better?
I think this API should be about a web page expressing interest or disinterest in the color chooser, and allowing the color chooser to inform the web page of color changes. I was thinking of WebColorChooser as a "color chooser connection" or "color choosing session" object. Maybe using a name like that would help since the lifetime of the color chooser dialog can be independent of a web page's interest in the color chooser? Hmm, WebColorChooserSession: class WebViewClient { ... virtual WebColorChooserSession* createColorChooserSession(WebColor initialColor, WebColorChooserSessionClient*) { ... } }; class WebColorChooserSession { public: virtual ~WebColorChooserSession() { } virtual void setColor(WebColor) { } }; class WebColorChooserSessionClient { public: virtual void didChangeColor(WebColor) = 0; virtual void didEndSession() = 0; protected: virtual ~WebColorChooserSessionClient() { } }; The caller of createColorChooserSession is responsible for deleting the WebColorChooserSession object when they are no longer interested in the color chooser. The color chooser can also force the session to end (via didEndSession), in which case it is still the responsibility of the caller of createColorChooserSession to delete the WebColorChooserSession object. Given a session object, it is possible to force the color chooser to show a different color by calling setColor. Also, while the session is active, it is possible for the color chooser to inform the web page that the color selection changed (didChangeColor).
> > So, I'd go with: > > > > class WebViewClient { > > ... > > virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } > > ... > > }; > > This won't work because WebCore::ColorChooser is no longer a singleton
http://trac.webkit.org/changeset/99403
> > There may be multiple input color elements on the page and they can call ChromeClientImpl::cleanupColorChooser even when it is not the one that opened the color picker.
^^^ I'm having a hard time understanding these issues.
Keishi Hattori
Comment 34
2011-12-01 21:54:46 PST
Created
attachment 117562
[details]
patch using WebColorChooserSession
Keishi Hattori
Comment 35
2011-12-01 21:55:36 PST
(In reply to
comment #33
)
> I think this API should be about a web page expressing interest or disinterest in the > color chooser, and allowing the color chooser to inform the web page of color changes.
Great idea! I've made a new patch using WebColorChooserSession.
> I was thinking of WebColorChooser as a "color chooser connection" or "color choosing > session" object. Maybe using a name like that would help since the lifetime of the > color chooser dialog can be independent of a web page's interest in the color chooser?
After implementing this I realized that WebColorChooserSessionClient wasn't doing much and was just making it complicated so I decided to make ChromeClientImpl be a WebColorChooserSessionClient.
> Given a session object, it is possible to force the color chooser to show a different color > by calling setColor. Also, while the session is active, it is possible for the color chooser > to inform the web page that the color selection changed (didChangeColor).
If we don't separate color being changed from the web page and color being changed by the user from the color picker, it will cause an infinite loop. Therefore I kept the method name didChooseColor, because it will only be called when the user changes color. Not when the web page changes the element value.
> > > > So, I'd go with: > > > > > > class WebViewClient { > > > ... > > > virtual WebColorChooser* createColorChooser(WebColor initialColor, WebColorChooserClient*) { return 0; } > > > ... > > > }; > > > > This won't work because WebCore::ColorChooser is no longer a singleton
http://trac.webkit.org/changeset/99403
> > > > There may be multiple input color elements on the page and they can call ChromeClientImpl::cleanupColorChooser even when it is not the one that opened the color picker. > > ^^^ I'm having a hard time understanding these issues.
Sorry. What I wanted to say was, Because the WebCore::ColorChooser is no longer a singleton, the checks "colorChooser == client->colorChooser()->client()->colorChooser()" (in my new patch they are colorChooser == m_colorChooser) are necessary. Each input color element now has a WebCore::ColorChooser instance, so it needs to check if the set color/close call is coming from the one that opened the color chooser.
WebKit Review Bot
Comment 36
2011-12-01 22:37:53 PST
Comment on
attachment 117562
[details]
patch using WebColorChooserSession
Attachment 117562
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10725219
Darin Fisher (:fishd, Google)
Comment 37
2011-12-02 16:13:52 PST
Comment on
attachment 117562
[details]
patch using WebColorChooserSession View in context:
https://bugs.webkit.org/attachment.cgi?id=117562&action=review
> Source/WebKit/chromium/public/WebColorChooserSession.h:49 > + virtual void didChooseColor(WebColor) = 0;
I think it is very confusing that you have didChooserColor on both WebColorChooserSession and WebColorChooserSessionClient. Looking at the WebCore equivalents of these classes (ColorChooser and ColorChooserClient), I see similarly confusing method names. I'm suspicious of that design. I'm also not sure that it makes sense to use the term Session here but not in WebCore. Maybe the Session naming is not helpful.
Keishi Hattori
Comment 38
2011-12-12 03:39:14 PST
Created
attachment 118768
[details]
new patch
Keishi Hattori
Comment 39
2011-12-12 03:41:42 PST
This is a new patch with the new architecture. WebColorChooserSession no longer has a method named didChooserColor. I removed any traces of "cleanup" and there is no more ColorChooser object in WebCore.
Darin Fisher (:fishd, Google)
Comment 40
2011-12-13 23:42:49 PST
Comment on
attachment 118768
[details]
new patch This design makes a lot more sense to me. Thank you for reconsidering the WebCore side. I will try to do a more thorough code review shortly.
Darin Fisher (:fishd, Google)
Comment 41
2011-12-14 11:09:40 PST
Comment on
attachment 118768
[details]
new patch View in context:
https://bugs.webkit.org/attachment.cgi?id=118768&action=review
I'm going to make the radical suggestion that given this new architecture, you could probably drop the word "Session" from everything, and it would all still make sense. You might need to change "endSession" to "endChooser"
> Source/WebCore/html/ColorInputType.cpp:130 > + session()->setSelectedColor(valueAsColor());
nit: indentation
> Source/WebCore/html/ColorInputType.cpp:150 > +void ColorInputType::newColorChooserSession(const Color& initialColor)
since this method returns early if there is already a session, perhaps it's name should have the IfNecessary suffix? also, I think 'create' would be a more canonical prefix: createColorChooserSessionIfNecessary
> Source/WebCore/loader/EmptyClients.h:195 > + ColorChooserSession* createColorChooserSession(ColorChooserSessionClient*, const Color&) { return 0; }
perhaps this should return PassOwnPtr?
> Source/WebCore/platform/ColorChooserSessionClient.h:18 > + ColorChooserSession* session() { return m_session.get(); }
Why does the ColorChooserSession need to be stored on the *Client? That is a very uncommon design. It seems like ColorInputType should have a m_session instead.
> Source/WebCore/platform/ColorChooserSessionClient.h:19 > + void setSession(ColorChooserSession* session) { m_session = adoptPtr(session); }
nit: parameter should be PassOwnPtr<ColorChooserSession>
Kent Tamura
Comment 42
2011-12-14 16:36:44 PST
Let's split the patch into the WebCore part and the WebKit part.
Keishi Hattori
Comment 43
2011-12-15 01:56:07 PST
Created
attachment 119399
[details]
new patch(webcore side moved to separate patch)
WebKit Review Bot
Comment 44
2011-12-15 01:58:12 PST
Attachment 119399
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/src/ColorChooserProxy.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kent Tamura
Comment 45
2011-12-15 02:11:48 PST
Comment on
attachment 119399
[details]
new patch(webcore side moved to separate patch) View in context:
https://bugs.webkit.org/attachment.cgi?id=119399&action=review
> Source/WebKit/chromium/src/ChromeClientImpl.h:142 > + virtual PassOwnPtr<WebCore::ColorChooser> createColorChooser(WebCore::ColorChooserClient*, const WebCore::Color&);
nit: append OVERRIDE.
> Source/WebKit/chromium/src/ColorChooserProxy.h:54 > +#endif // GeolocationClientProxy_h
False comment.
> Source/WebKit/chromium/src/WebColorChooserClientImpl.h:45 > + virtual void didChooseColor(const WebColor&); > + virtual void didEndChooser();
nit: OVERRIDE
> Source/WebKit/chromium/src/WebColorChooserClientImpl.h:55 > +#endif // WebColorChooserClient_h
False comment
WebKit Review Bot
Comment 46
2011-12-15 02:13:56 PST
Comment on
attachment 119399
[details]
new patch(webcore side moved to separate patch)
Attachment 119399
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10904166
Gustavo Noronha (kov)
Comment 47
2011-12-15 02:37:04 PST
Comment on
attachment 119399
[details]
new patch(webcore side moved to separate patch)
Attachment 119399
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10905209
Keishi Hattori
Comment 48
2011-12-15 04:12:47 PST
Created
attachment 119410
[details]
fixed
Keishi Hattori
Comment 49
2011-12-15 04:20:24 PST
Comment on
attachment 118768
[details]
new patch View in context:
https://bugs.webkit.org/attachment.cgi?id=118768&action=review
>> Source/WebCore/html/ColorInputType.cpp:130 >> + session()->setSelectedColor(valueAsColor()); > > nit: indentation
done.
>> Source/WebCore/html/ColorInputType.cpp:150 >> +void ColorInputType::newColorChooserSession(const Color& initialColor) > > since this method returns early if there is already a session, perhaps it's name > should have the IfNecessary suffix? also, I think 'create' would be a more > canonical prefix: createColorChooserSessionIfNecessary
Removed since its only called from one place.
>> Source/WebCore/loader/EmptyClients.h:195 >> + ColorChooserSession* createColorChooserSession(ColorChooserSessionClient*, const Color&) { return 0; } > > perhaps this should return PassOwnPtr?
done.
>> Source/WebCore/platform/ColorChooserSessionClient.h:18 >> + ColorChooserSession* session() { return m_session.get(); } > > Why does the ColorChooserSession need to be stored on the *Client? That is a very > uncommon design. It seems like ColorInputType should have a m_session instead.
done.
>> Source/WebCore/platform/ColorChooserSessionClient.h:19 >> + void setSession(ColorChooserSession* session) { m_session = adoptPtr(session); } > > nit: parameter should be PassOwnPtr<ColorChooserSession>
Removed.
WebKit Review Bot
Comment 50
2011-12-15 04:53:18 PST
Comment on
attachment 119410
[details]
fixed
Attachment 119410
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10904223
Gustavo Noronha (kov)
Comment 51
2011-12-15 05:16:51 PST
Comment on
attachment 119410
[details]
fixed
Attachment 119410
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10904235
Collabora GTK+ EWS bot
Comment 52
2011-12-15 06:17:57 PST
Comment on
attachment 119410
[details]
fixed
Attachment 119410
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10904234
WebKit Review Bot
Comment 53
2011-12-16 00:46:29 PST
Comment on
attachment 119410
[details]
fixed Rejecting
attachment 119410
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: WebKit/chromium/src/WebColor.o CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebColorChooserClientImpl.o CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebCommon.o Source/WebKit/chromium/src/WebColorChooserClientImpl.cpp:30: fatal error: ColorChooserClient.h: No such file or directory compilation terminated. make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebColorChooserClientImpl.o] Error 1 make: *** Waiting for unfinished jobs.... Full output:
http://queues.webkit.org/results/10900596
WebKit Review Bot
Comment 54
2011-12-18 02:37:27 PST
Comment on
attachment 119410
[details]
fixed Clearing flags on attachment: 119410 Committed
r103169
: <
http://trac.webkit.org/changeset/103169
>
WebKit Review Bot
Comment 55
2011-12-18 02:37:35 PST
All reviewed patches have been landed. Closing bug.
Kent Tamura
Comment 56
2011-12-18 18:19:28 PST
Reopened because it was rolled out.
Keishi Hattori
Comment 57
2011-12-18 20:18:33 PST
Created
attachment 119803
[details]
updated pbxproj and GNUmakefile
WebKit Review Bot
Comment 58
2011-12-18 21:23:10 PST
Comment on
attachment 119803
[details]
updated pbxproj and GNUmakefile Clearing flags on attachment: 119803 Committed
r103215
: <
http://trac.webkit.org/changeset/103215
>
WebKit Review Bot
Comment 59
2011-12-18 21:23:18 PST
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.
Top of Page
Format For Printing
XML
Clone This Bug