Bug 62619 - Implement <input type=color> UI behavior WebCore part
Summary: Implement <input type=color> UI behavior WebCore part
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 61276
  Show dependency treegraph
 
Reported: 2011-06-13 20:42 PDT by Keishi Hattori
Modified: 2011-08-08 05:44 PDT (History)
6 users (show)

See Also:


Attachments
patch (17.44 KB, patch)
2011-06-13 20:54 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
updated test results (3.83 KB, patch)
2011-06-19 20:30 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
new patch (28.64 KB, patch)
2011-08-03 04:34 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed patch (27.43 KB, patch)
2011-08-04 05:41 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
new fixed patch (23.13 KB, patch)
2011-08-05 03:53 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
new new fixed patch (28.37 KB, patch)
2011-08-05 04:40 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
updated changelog and pbxproj (28.71 KB, patch)
2011-08-05 05:35 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fix layering violation (27.87 KB, patch)
2011-08-07 22:14 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
new patch (27.88 KB, patch)
2011-08-07 23:57 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-06-13 20:42:57 PDT
Patch to implement WebCore part of <input type=color> UI behavior.
Comment 1 Keishi Hattori 2011-06-13 20:54:55 PDT
Created attachment 97063 [details]
patch
Comment 2 Kent Tamura 2011-06-13 21:33:11 PDT
Comment on attachment 97063 [details]
patch

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

> Source/WebCore/html/ColorInputType.cpp:116
> +    element()->setValue(color.serialized(), true);

Because this line dispatches a 'change' event and a JavaScript code can run, it's possible that the JavaScript code change the input type and this ColorInputType instance might be deleted. We must not access 'this' after this line.

> Source/WebCore/html/ColorInputType.cpp:120
> +        element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value()));

Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it?

> Source/WebCore/page/Chrome.cpp:461
> +void Chrome::runUpdateColorPanel(const Color color)
> +{
> +    m_client->runUpdateColorPanel(color);

The name 'runUpdateColorPanel' sounds curious.
'updateSelectedColorOfColorPanel'?

> Source/WebCore/page/ChromeClient.h:228
> +        virtual void runColorPanel() = 0;

You need to add a comment of the function behavior.
What should we do if runColorPanel() is called twice?
When should we close the color panel opened by runColorPanel()?
How to obtain ColorChooser instance to which the color panel send the selected color?
Comment 3 Alexis Menard (darktears) 2011-06-17 06:36:15 PDT
Comment on attachment 97063 [details]
patch

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

>> Source/WebCore/html/ColorInputType.cpp:120
>> +        element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value()));
> 
> Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it?

Well I believe in JS you can change the color, and therefore the panel should update itself no? Though I'm not very sure if the JS events ends up in colorChanged.

> Source/WebCore/page/Chrome.cpp:456
> +    m_client->runColorPanel();

What about showColorPanel()? At the end we "show" a popup.

To create the <select> popup the method's name is createSelectPopup(), perhaps we should stick with createColorChooserPanel()

>> Source/WebCore/page/Chrome.cpp:461
>> +    m_client->runUpdateColorPanel(color);
> 
> The name 'runUpdateColorPanel' sounds curious.
> 'updateSelectedColorOfColorPanel'?

updatePanelCurrentColor()?

>> Source/WebCore/page/ChromeClient.h:228
>> +        virtual void runColorPanel() = 0;
> 
> You need to add a comment of the function behavior.
> What should we do if runColorPanel() is called twice?
> When should we close the color panel opened by runColorPanel()?
> How to obtain ColorChooser instance to which the color panel send the selected color?

I believe if it's run twice, it doesn't matter the popup will still be shown no?

I think when a color is selected we should close the panel no?
http://www.devbeginners.com/index.php/html5/108-html5-color-picker-input-type

has an example. I couldn't find in the spec *how* the popup should look like, is there some kind of guidance for that?

> Source/WebCore/platform/ColorChooser.h:46
> +

I believe ports will inherits from that?
Comment 4 Keishi Hattori 2011-06-19 18:29:00 PDT
Thanks for taking a look!

(In reply to comment #3)
> (From update of attachment 97063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97063&action=review
> 
> >> Source/WebCore/html/ColorInputType.cpp:120
> >> +        element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->value()));
> > 
> > Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged() is called from the color panel, isn't it?
> Well I believe in JS you can change the color, and therefore the panel should update itself no? Though I'm not very sure if the JS events ends up in colorChanged.

Sorry, I got mixed up when dividing the patch. I will fix it.

> > Source/WebCore/page/Chrome.cpp:456
> > +    m_client->runColorPanel();
> 
> What about showColorPanel()? At the end we "show" a popup.
> 
> To create the <select> popup the method's name is createSelectPopup(), perhaps we should stick with createColorChooserPanel()

I tried to match the name with Chrome::runOpenPanel, but I think showColorPanel is much better. I'd gladly change it.

> >> Source/WebCore/page/Chrome.cpp:461
> >> +    m_client->runUpdateColorPanel(color);
> > 
> > The name 'runUpdateColorPanel' sounds curious.
> > 'updateSelectedColorOfColorPanel'?
> 
> updatePanelCurrentColor()?

Ditto.

> >> Source/WebCore/page/ChromeClient.h:228
> >> +        virtual void runColorPanel() = 0;
> > 
> > You need to add a comment of the function behavior.
> > What should we do if runColorPanel() is called twice?
> > When should we close the color panel opened by runColorPanel()?
> > How to obtain ColorChooser instance to which the color panel send the selected color?
> 
> I believe if it's run twice, it doesn't matter the popup will still be shown no?

I don't think we should ever show multiple color choose dialogs. It is impossible to tell which dialog is from which input element. 

Therefore I think it is safe to implement it for only one instance of ColorChooser.

> I think when a color is selected we should close the panel no?
> http://www.devbeginners.com/index.php/html5/108-html5-color-picker-input-type

Mac OS color picker is usually persistant. However now that I think about it, it probably should close when the page changes or something.

> has an example. I couldn't find in the spec *how* the popup should look like, is there some kind of guidance for that?

I don't think there is. 

> > Source/WebCore/platform/ColorChooser.h:46
> 
> I believe ports will inherits from that?

Hopefully they wouldn't have to. I think implementing their UI stuff in ChromeClient::runColorPanel/runUpdateColorPanel should be enough.
Comment 5 Keishi Hattori 2011-06-19 20:30:08 PDT
Created attachment 97735 [details]
updated test results
Comment 6 Keishi Hattori 2011-06-19 20:31:03 PDT
Comment on attachment 97735 [details]
updated test results

Sorry. Wrong bug.
Comment 7 Alexander Pavlov (apavlov) 2011-07-05 08:17:57 PDT
Hey folks,

Is there any estimate of when the patch can be landed? Web Inspector could make a good use of this feature...
Comment 8 Keishi Hattori 2011-07-05 20:53:06 PDT
(In reply to comment #7)
> Is there any estimate of when the patch can be landed? Web Inspector could make a good use of this feature...

This is my top priority but because there are UIs to decide on and many platforms to implement, it will take at least a few more months.
Comment 9 Keishi Hattori 2011-08-03 04:34:36 PDT
Created attachment 102767 [details]
new patch
Comment 10 Keishi Hattori 2011-08-03 04:36:47 PDT
(In reply to comment #9)
> Created an attachment (id=102767)(apply) [details]
> new patch

Added method to close color chooser.
Better method names.
Comment 11 Kent Tamura 2011-08-04 01:30:45 PDT
Comment on attachment 102767 [details]
new patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19091
>  				E1E1BEFF115FF6FB006F52CA /* WindowsKeyboardCodes.h */,
> +				C3EEE07713E7F9DE005F7460 /* ColorChooser.h */,
> +				C3EEE07813E7F9DE005F7460 /* ColorChooser.cpp */,

They should be in alphabetical order.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20281
>  				41E1B1D10FF5986900576B3B /* AbstractWorker.h in Headers */,
> +				C3EEE07913E7F9DE005F7460 /* ColorChooser.h in Headers */,
>  				29A8122E0FBB9C1D00510293 /* AccessibilityARIAGridCell.h in Headers */,

ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26071
>  				BCAB418113E356E800D8AAF3 /* Region.cpp in Sources */,
> +				C3EEE07A13E7F9DE005F7460 /* ColorChooser.cpp in Sources */,

ditto.

> Source/WebCore/css/html.css:640
>      width: 100%;
> -    height: 100%
> +    height: 100%;

This change is unrelated to this bug.

> Source/WebCore/html/ColorInputType.cpp:147
> +    ExceptionCode ec;
> +    setValueAsColor(color, ec);
> +}

We shouldn't ignore the resultant ExceptionCode.
Please see the comment about setValueAsColor() in ColorInputType.h

> Source/WebCore/html/ColorInputType.h:53
> +    virtual void setValueAsColor(const Color&, ExceptionCode&) const;

We should have the default argument for ExceptionCode&.
  virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const;
Then, you can omit the ExceptionCode argument for call sites.

> Source/WebCore/html/ColorInputType.h:60
> +
> +    virtual void colorSelected(const Color&);

Please add a comment that this function is for ColorChooserClient.

> Source/WebCore/html/HTMLInputElement.h:157
> +    Color valueAsColor() const;
> +    void setValueAsColor(const Color&, ExceptionCode&);

Why we need them in HTMLInputElement?

> Source/WebCore/page/Chrome.h:163
> +        void runOpenColorChooser(ColorChooser*, const Color&);
> +        void runCloseColorChooser();
> +        void runSetSelectedColorInColorChooser(const Color&);

I still don't like these function names.  'run' + verb sounds very strange.
I guess "runOpenPanel" means "run an OpenPanel dialog".  So we don't need to follow it.

> Source/WebCore/platform/ColorChooser.h:60
> +    ColorChooser()
> +    : m_client(0)

Indentation is wrong.
Comment 12 Kent Tamura 2011-08-04 01:33:39 PDT
(In reply to comment #11)
> I still don't like these function names.  'run' + verb sounds very strange.
> I guess "runOpenPanel" means "run an OpenPanel dialog".  So we don't need to follow it.

I found the names of the file chooser dialog in OS X is NSOpenPanel.  So "OpenPanel" is a noun.
Comment 13 Keishi Hattori 2011-08-04 05:41:42 PDT
Created attachment 102900 [details]
fixed patch
Comment 14 Keishi Hattori 2011-08-04 05:45:41 PDT
Comment on attachment 102767 [details]
new patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj(edit):19091
>> +				C3EEE07813E7F9DE005F7460 /* ColorChooser.cpp */,
> 
> They should be in alphabetical order.

Done.

>> Source/WebCore/css/html.css(edit):640
>> +    height: 100%;
> 
> This change is unrelated to this bug.

Done.

>> Source/WebCore/html/ColorInputType.cpp(edit):147
>> +}
> 
> We shouldn't ignore the resultant ExceptionCode.
> Please see the comment about setValueAsColor() in ColorInputType.h

Removed ec.

>> Source/WebCore/html/ColorInputType.h(edit):53
>> +    virtual void setValueAsColor(const Color&, ExceptionCode&) const;
> 
> We should have the default argument for ExceptionCode&.
>   virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const;
> Then, you can omit the ExceptionCode argument for call sites.

Done.

>> Source/WebCore/html/ColorInputType.h(edit):60
>> +    virtual void colorSelected(const Color&);
> 
> Please add a comment that this function is for ColorChooserClient.

Done.

>> Source/WebCore/html/HTMLInputElement.h(edit):157
>> +    void setValueAsColor(const Color&, ExceptionCode&);
> 
> Why we need them in HTMLInputElement?

Done.
Is HTMLInputElement::valueAsDate() something left over from the past?

>> Source/WebCore/page/Chrome.h(edit):163
>> +        void runSetSelectedColorInColorChooser(const Color&);
> 
> I still don't like these function names.  'run' + verb sounds very strange.
> I guess "runOpenPanel" means "run an OpenPanel dialog".  So we don't need to follow it.

Removed run part. 
I happened to be looking at parts of the code in WebKit and Chromium with lots of run*.

>> Source/WebCore/platform/ColorChooser.h(edit):60
>> +    : m_client(0)
> 
> Indentation is wrong.

Done.
Comment 15 Kent Tamura 2011-08-04 06:47:35 PDT
(In reply to comment #14)
> > Why we need them in HTMLInputElement?
> 
> Done.
> Is HTMLInputElement::valueAsDate() something left over from the past?

valueAsDate() is exposed to JavaScript.
Comment 16 Kent Tamura 2011-08-04 06:58:34 PDT
Comment on attachment 102900 [details]
fixed patch

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

> Source/WebCore/html/ColorInputType.cpp:128
> +void ColorInputType::willRemove()
> +{
> +    if (ColorChooser::chooser()->client() == this) {
> +        if (Chrome* chrome = this->chrome())
> +            chrome->closeColorChooser();

I think Node::willRemove() is wrong for this task.
We should close the chooser when the element disappears.  e.g. display:none;

> Source/WebCore/html/ColorInputType.h:53
> +    virtual Color valueAsColor() const;
> +    virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const;

I realized that valueAsColor() and setValueAsColor() were used only by ColorInputType.
So,
 - we dont need InputType::valueAsColor() and setValueAsColor().
 - They should not be virtual.
 - They should be private.
 - setValueAsColor() doesn't need the ExceptionCode& argument at all.
Comment 17 Keishi Hattori 2011-08-05 03:53:12 PDT
Created attachment 103057 [details]
new fixed patch
Comment 18 Keishi Hattori 2011-08-05 03:57:18 PDT
Comment on attachment 102900 [details]
fixed patch

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

>> Source/WebCore/html/ColorInputType.cpp(edit):128
>> +            chrome->closeColorChooser();
> 
> I think Node::willRemove() is wrong for this task.
> We should close the chooser when the element disappears.  e.g. display:none;

Done. Moved "close color chooser" call to ColorInputType::detach.

>> Source/WebCore/html/ColorInputType.h(edit):53
>> +    virtual void setValueAsColor(const Color&, ExceptionCode& = ASSERT_NO_EXCEPTION) const;
> 
> I realized that valueAsColor() and setValueAsColor() were used only by ColorInputType.
> So,
>  - we dont need InputType::valueAsColor() and setValueAsColor().
>  - They should not be virtual.
>  - They should be private.
>  - setValueAsColor() doesn't need the ExceptionCode& argument at all.

Done.


Also added ColorChooser::closeColorChooserIfClientInDocument(). It is called from WebCore::FrameLoader::transitionToCommitted so it closes the color chooser when the another page is loaded in the frame.
Comment 19 Kent Tamura 2011-08-05 04:03:32 PDT
Comment on attachment 103057 [details]
new fixed patch

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

> Source/WebCore/html/HTMLInputElement.cpp:890
> +#if ENABLE(INPUT_COLOR)
> +    if (ColorChooser::chooser()->client() == m_inputType) {
> +        if (m_inputType->isColorControl) {
> +            if (Page* page = frame->page())
> +                page->chrome()->closeColorChooser();
> +        }
> +    }
> +#endif

We should not add type-specific code into HTMLInputElement.
Please add InputType::elementWasDetached().

> Source/WebCore/platform/ColorChooser.h:46
> +    virtual void closeColorChooserIfClientInDocument(Document*) = 0;

IfClientInDocument -> IfClientIsInDocument
Comment 20 Keishi Hattori 2011-08-05 04:40:11 PDT
Created attachment 103060 [details]
new new fixed patch
Comment 21 Keishi Hattori 2011-08-05 04:42:06 PDT
(In reply to comment #19)
> (From update of attachment 103057 [details](apply) [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103057&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:890
> > +#if ENABLE(INPUT_COLOR)
> > +    if (ColorChooser::chooser()->client() == m_inputType) {
> > +        if (m_inputType->isColorControl) {
> > +            if (Page* page = frame->page())
> > +                page->chrome()->closeColorChooser();
> > +        }
> > +    }
> > +#endif
> 
> We should not add type-specific code into HTMLInputElement.
> Please add InputType::elementWasDetached().

This shouldn't have been there. I got my git branches mixed up. 
I added InputType::detach to be in line with the existing InputType::attach, but elementWasDetached would be better. Should I rename InputType::attach too?

> > Source/WebCore/platform/ColorChooser.h:46
> > +    virtual void closeColorChooserIfClientInDocument(Document*) = 0;
> 
> IfClientInDocument -> IfClientIsInDocument

Done.
Comment 22 Kent Tamura 2011-08-05 04:42:35 PDT
Comment on attachment 103060 [details]
new new fixed patch

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

> Source/WebCore/ChangeLog:27
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::detach): Close color chooser. Called when input element or its ancestors have "display:none"
> +        or is removed from DOM.

ChangeLog is out-of-sync.
Comment 23 Keishi Hattori 2011-08-05 05:35:37 PDT
Created attachment 103063 [details]
updated changelog and pbxproj
Comment 24 Kent Tamura 2011-08-05 05:42:43 PDT
Comment on attachment 103063 [details]
updated changelog and pbxproj

ok!
Comment 25 WebKit Review Bot 2011-08-05 06:45:37 PDT
Comment on attachment 103063 [details]
updated changelog and pbxproj

Clearing flags on attachment: 103063

Committed r92477: <http://trac.webkit.org/changeset/92477>
Comment 26 WebKit Review Bot 2011-08-05 06:45:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Sam Weinig 2011-08-05 09:48:52 PDT
This patch introduced a layering violation by using Document.h from the platform directory.  Please consider reverting and fixing this issue.
Comment 28 Kent Tamura 2011-08-05 18:05:17 PDT
(In reply to comment #27)
> This patch introduced a layering violation by using Document.h from the platform directory.  Please consider reverting and fixing this issue.

You're right!  Thank you for pointing it out.
I'll roll out this change.
Comment 29 Kent Tamura 2011-08-05 18:07:03 PDT
Reverted r92477 for reason:

Layering violation. We should not use WebCore/dom/ in WebCore/platform/.

Committed r92533: <http://trac.webkit.org/changeset/92533>
Comment 30 Kent Tamura 2011-08-05 19:06:40 PDT
Comment on attachment 103063 [details]
updated changelog and pbxproj

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

> Source/WebCore/loader/FrameLoader.cpp:1825
> +    if (m_frame->document())
> +        ColorChooser::chooser()->closeColorChooserIfClientIsInDocument(m_frame->document());

Are there no other ways to close the color chooser when the document is navigated out?  Is not the document detached?

Some ideas to remove the layering violation:
* Introduce ColorChooserContext interface class, and Document inherits it, or
* Register/unregister unload(?) callback to Document or Frame.  A ColorChooserClient registers itself to it.

> Source/WebCore/platform/ColorChooser.cpp:47
> +        staticChooser = new ColorChooser();

We should use
   adoptPtr(new ColorChooser()).leakPtr()
to inform no one will delete this object.
Comment 31 Keishi Hattori 2011-08-07 22:06:23 PDT
(In reply to comment #30)
> (From update of attachment 103063 [details](apply) [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103063&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1825
> > +    if (m_frame->document())
> > +        ColorChooser::chooser()->closeColorChooserIfClientIsInDocument(m_frame->document());
> 
> Are there no other ways to close the color chooser when the document is navigated out?  Is not the document detached?

Document::detach and ColorInputType::detach do get called when navigating to another page, but it is like 10 seconds after the new page is displayed. It seems to be called when a timer is fired and PageCache::releaseAutoreleasedPagesNowOrReschedule is called.

We can close the color chooser much faster if we call from FrameLoader.

> Some ideas to remove the layering violation:
> * Introduce ColorChooserContext interface class, and Document inherits it, or
> * Register/unregister unload(?) callback to Document or Frame.  A ColorChooserClient registers itself to it.

I tried to make the change minimal in my next patch. However it still uses the word "ColorInputType" inside a method name so I think it is still layering violation.

> We should use
>    adoptPtr(new ColorChooser()).leakPtr()
> to inform no one will delete this object.
Done.
Comment 32 Keishi Hattori 2011-08-07 22:14:33 PDT
Created attachment 103201 [details]
fix layering violation
Comment 33 Kent Tamura 2011-08-07 22:26:19 PDT
Comment on attachment 103201 [details]
fix layering violation

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

> Source/WebCore/ChangeLog:39
> +        (WebCore::FrameLoader::transitionToCommitted): Close color chooser when
> +        navigating away from the page.

Adding the reason that detach() doesn't work would be helpful.

> Source/WebCore/html/ColorInputType.cpp:203
> +    if (ColorChooser::chooser()->client() == this) {

We prefer early exit.

if (ColorChooser::chooser()->client() != this)
    return;

> Source/WebCore/loader/FrameLoader.cpp:1828
> +        ColorInputType* colorInputType = (ColorInputType*)colorChooserClient;
> +        colorInputType->closeColorChooserIfCurrentClient();

Should use C++-style cast instead of C-style cast.

static_cast<ColorInputType*>(colorChooserClient)->closeColorChooserIfCurrenClient();
Comment 34 Keishi Hattori 2011-08-07 23:57:24 PDT
Created attachment 103210 [details]
new patch
Comment 35 Keishi Hattori 2011-08-07 23:58:13 PDT
Comment on attachment 103201 [details]
fix layering violation

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

>> Source/WebCore/ChangeLog(edit):39
>> +        navigating away from the page.
> 
> Adding the reason that detach() doesn't work would be helpful.

Done.

>> Source/WebCore/html/ColorInputType.cpp(edit):203
>> +    if (ColorChooser::chooser()->client() == this) {
> 
> We prefer early exit.
> 
> if (ColorChooser::chooser()->client() != this)
>     return;

Done.

>> Source/WebCore/loader/FrameLoader.cpp(edit):1828
>> +        colorInputType->closeColorChooserIfCurrentClient();
> 
> Should use C++-style cast instead of C-style cast.
> 
> static_cast<ColorInputType*>(colorChooserClient)->closeColorChooserIfCurrenClient();

Done.
Comment 36 Kent Tamura 2011-08-08 00:04:54 PDT
Comment on attachment 103210 [details]
new patch

ok
Comment 37 WebKit Review Bot 2011-08-08 05:44:49 PDT
Comment on attachment 103210 [details]
new patch

Clearing flags on attachment: 103210

Committed r92592: <http://trac.webkit.org/changeset/92592>
Comment 38 WebKit Review Bot 2011-08-08 05:44:56 PDT
All reviewed patches have been landed.  Closing bug.