Implement <input type=color> UI WebKit chromium part

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

implement ChromeClient::openColorChooser, closeColorChooser, and setSelectedColorInColorChooser for WebKit chromium

[Keishi Hattori]

Created attachment 103436 [details]
patch

[WebKit Review Bot]

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]

(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 on attachment 103436 [details]
patch

Attachment 103436 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9337487

[Keishi Hattori]

Created attachment 103442 [details]
wrapped in flag

[Kent Tamura]

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]

BTW, will we have multiple color chooser dialogs if multiple tabs have color input fields?

[Kent Tamura]

Adding fishd for WebKit API change.

[Keishi Hattori]

Created attachment 103454 [details]
added comments

[Keishi Hattori]

(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]

(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]

(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]

LGTM.

fishd,
Would you review WebKit API changes please?

[Keishi Hattori]

Created attachment 103723 [details]
changed selectedColor to colorSelected

[Keishi Hattori]

(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]

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]

(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 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]

Created attachment 105949 [details]
fixed patch

[Keishi Hattori]

(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]

Created attachment 108838 [details]
rebased

[Darin Fisher (:fishd, Google)]

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]

Created attachment 111393 [details]
Patch

[Keishi Hattori]

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]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Keishi Hattori]

>   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]

Created attachment 113826 [details]
Patch

[Keishi Hattori]

This new patch is for when the patch for Bug 71644  lands.
WebCore::ColorChooser will no longer be a singleton.

[Keishi Hattori]

Created attachment 113841 [details]
Patch

[Keishi Hattori]

I updated the chromium side to reflect this change
http://codereview.chromium.org/7685006/

[Darin Fisher (:fishd, Google)]

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]

(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)]

(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]

Created attachment 117562 [details]
patch using WebColorChooserSession

[Keishi Hattori]

(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 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 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]

Created attachment 118768 [details]
new patch

[Keishi Hattori]

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 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 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]

Let's split the patch into the WebCore part and the WebKit part.

[Keishi Hattori]

Created attachment 119399 [details]
new patch(webcore side moved to separate patch)

[WebKit Review Bot]

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 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 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 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]

Created attachment 119410 [details]
fixed

[Keishi Hattori]

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 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 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 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 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 on attachment 119410 [details]
fixed

Clearing flags on attachment: 119410

Committed r103169: <http://trac.webkit.org/changeset/103169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Reopened because it was rolled out.

[Keishi Hattori]

Created attachment 119803 [details]
updated pbxproj and GNUmakefile

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.