Bug 74591 - Refactor input type color WebCore part
Summary: Refactor input type color 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: 65897
  Show dependency tree / graph
 
Reported: 2011-12-14 23:26 PST by Keishi Hattori
Modified: 2011-12-17 23:03 PST (History)
5 users (show)

See Also:

Attachments
patch (13.55 KB, patch)
2011-12-14 23:48 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
removed WebCore.gypi to move to the WebKit patch (12.99 KB, patch)
2011-12-14 23:56 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
fixed nits (13.36 KB, patch)
2011-12-15 01:01 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
use ASSERT_NO_EXCEPTION (13.39 KB, patch)
2011-12-15 01:16 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-12-14 23:26:07 PST 
See Bug 65897
Refactoring ColorChooser.
Comment 1 Keishi Hattori 2011-12-14 23:48:12 PST 
Created attachment 119384 [details]
patch
Comment 2 Keishi Hattori 2011-12-14 23:56:26 PST 
Created attachment 119385 [details]
removed WebCore.gypi to move to the WebKit patch
Comment 3 Kent Tamura 2011-12-15 00:08:01 PST 
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?
Comment 4 Keishi Hattori 2011-12-15 00:59:41 PST 
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.
Comment 5 Keishi Hattori 2011-12-15 01:01:31 PST 
Created attachment 119393 [details]
fixed nits
Comment 6 Kent Tamura 2011-12-15 01:04:53 PST 
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.
Comment 7 Keishi Hattori 2011-12-15 01:16:44 PST 
Created attachment 119396 [details]
use ASSERT_NO_EXCEPTION
Comment 8 WebKit Review Bot 2011-12-15 23:47:54 PST 
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
Comment 9 WebKit Review Bot 2011-12-17 23:03:38 PST 
Comment on attachment 119396 [details]
use ASSERT_NO_EXCEPTION

Clearing flags on attachment: 119396

Committed r103168: <http://trac.webkit.org/changeset/103168>
Comment 10 WebKit Review Bot 2011-12-17 23:03:43 PST 
All reviewed patches have been landed.  Closing bug.