Bug 74591

Summary: Refactor input type color WebCore part
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, fishd, japhet, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 65897    
Attachments:
Description Flags
patch
none
removed WebCore.gypi to move to the WebKit patch
none
fixed nits
none
use ASSERT_NO_EXCEPTION none

Keishi Hattori
Reported 2011-12-14 23:26:07 PST
See Bug 65897 Refactoring ColorChooser.
Attachments
patch (13.55 KB, patch)
2011-12-14 23:48 PST, Keishi Hattori 		no flags
DetailsFormatted DiffDiff
removed WebCore.gypi to move to the WebKit patch (12.99 KB, patch)
2011-12-14 23:56 PST, Keishi Hattori 		no flags
DetailsFormatted DiffDiff
fixed nits (13.36 KB, patch)
2011-12-15 01:01 PST, Keishi Hattori 		no flags
DetailsFormatted DiffDiff
use ASSERT_NO_EXCEPTION (13.39 KB, patch)
2011-12-15 01:16 PST, Keishi Hattori 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Keishi Hattori
Comment 1 2011-12-14 23:48:12 PST
Created attachment 119384 [details] patch
Keishi Hattori
Comment 2 2011-12-14 23:56:26 PST
Created attachment 119385 [details] removed WebCore.gypi to move to the WebKit patch
Kent Tamura
Comment 3 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?
Keishi Hattori
Comment 4 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.
Keishi Hattori
Comment 5 2011-12-15 01:01:31 PST
Created attachment 119393 [details] fixed nits
Kent Tamura
Comment 6 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.
Keishi Hattori
Comment 7 2011-12-15 01:16:44 PST
Created attachment 119396 [details] use ASSERT_NO_EXCEPTION
WebKit Review Bot
Comment 8 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
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-12-17 23:03:43 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.