Bug 74591 – Refactor input type color WebCore part

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

See Bug 65897
Refactoring ColorChooser.

[Keishi Hattori]

Created attachment 119384 [details]
patch

[Keishi Hattori]

Created attachment 119385 [details]
removed WebCore.gypi to move to the WebKit patch

[Kent Tamura]

Comment on attachment 119384 [details]
patch

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

r- because of some nits.

> Source/WebCore/ChangeLog:7
> +        Refactor input type color WebCore part
> +        https://bugs.webkit.org/show_bug.cgi?id=74591
> +
> +        Reviewed by NOBODY (OOPS!).
> +

You had better add overview of the change.

> Source/WebCore/html/ColorInputType.cpp:131
> +    if (m_chooser.get())

You don't need .get().  "if (m_chooser)" is enough.

> Source/WebCore/html/ColorInputType.cpp:144
> +    if (chrome && !m_chooser.get())

ditto.

> Source/WebCore/html/ColorInputType.cpp:171
> +    if (m_chooser.get())

ditto.

> Source/WebCore/html/ColorInputType.cpp:182
>      ExceptionCode ec;
> -    colorSwatch->style()->setProperty("background-color", element()->value(), ec);
> +    colorSwatch->style()->setProperty(CSSPropertyBackgroundColor, element()->value(), false, ec);

You shouldn't ignore ExceptionCode result.

colorSwatch->style()->setProperty(CSSPropertyBackgroundColor, element()->value(), false, ASSERT_NO_EXCEPTION);

> Source/WebCore/platform/ColorChooser.h:34
> +#include <stdio.h>

Is this needed?

[Keishi Hattori]

Comment on attachment 119384 [details]
patch

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> You had better add overview of the change.

Added.

>> Source/WebCore/html/ColorInputType.cpp:131
>> +    if (m_chooser.get())
> 
> You don't need .get().  "if (m_chooser)" is enough.

done.

>> Source/WebCore/html/ColorInputType.cpp:144
>> +    if (chrome && !m_chooser.get())
> 
> ditto.

done.

>> Source/WebCore/html/ColorInputType.cpp:171
>> +    if (m_chooser.get())
> 
> ditto.

done.

>> Source/WebCore/platform/ColorChooser.h:34
>> +#include <stdio.h>
> 
> Is this needed?

Sorry. Removed.

[Keishi Hattori]

Created attachment 119393 [details]
fixed nits

[Kent Tamura]

Comment on attachment 119393 [details]
fixed nits

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

> Source/WebCore/html/ColorInputType.cpp:183
>      ExceptionCode ec;
> -    colorSwatch->style()->setProperty("background-color", element()->value(), ec);
> +    colorSwatch->style()->setProperty(CSSPropertyBackgroundColor, element()->value(), false, ec);
>  }

nit: My comment for this part was not addressed.

[Keishi Hattori]

Created attachment 119396 [details]
use ASSERT_NO_EXCEPTION

[WebKit Review Bot]

Comment on attachment 119396 [details]
use ASSERT_NO_EXCEPTION

Rejecting attachment 119396 [details] from commit-queue.

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html
Full output: http://queues.webkit.org/results/10895620

[WebKit Review Bot]

Comment on attachment 119396 [details]
use ASSERT_NO_EXCEPTION

Clearing flags on attachment: 119396

Committed r103168: <http://trac.webkit.org/changeset/103168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.