RESOLVED FIXED 40052
[DRT/Chromium] Upstream test_shell_webthemeengine as WebThemeEngineDRT
https://bugs.webkit.org/show_bug.cgi?id=40052
Summary [DRT/Chromium] Upstream test_shell_webthemeengine as WebThemeEngineDRT
Roland Steiner
Reported 2010-06-02 03:52:08 PDT
Upstream test_shell_webthemeengine for DRT
Attachments
patch - WebKit version of WebThemeEngine (58.70 KB, patch)
2010-06-07 04:25 PDT, Roland Steiner
tkent: review-
patch - WebKit version of WebThemeEngine v2 (57.48 KB, patch)
2010-06-07 19:38 PDT, Roland Steiner
tkent: review-
patch - WebKit version of WebThemeEngine v2 (57.53 KB, patch)
2010-06-08 00:00 PDT, Roland Steiner
tkent: review-
patch - WebKit version of WebThemeEngine v2 (57.31 KB, patch)
2010-06-08 20:59 PDT, Roland Steiner
tkent: review+
tkent: commit-queue-
Roland Steiner
Comment 1 2010-06-07 04:25:24 PDT
Created attachment 58013 [details] patch - WebKit version of WebThemeEngine Initial version of WebThemeEngine for DRT Also rolling Chromium DEPS since it requires new functionality from webkit_support included in Chromium rev. 48907 Note: A few cases of expected style "violations" due to aligned comments (better readability this way IMHO).
WebKit Review Bot
Comment 2 2010-06-07 04:28:25 PDT
Attachment 58013 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:57: One space before end of line comments [whitespace/comments] [5] WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:65: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 3 2010-06-07 05:11:24 PDT
Kent Tamura
Comment 4 2010-06-07 06:49:27 PDT
Comment on attachment 58013 [details] patch - WebKit version of WebThemeEngine WebKitTools/ChangeLog:8 + Add WebThemeEngineDRT and WebThemeControlDRT Could you write what Chromium revision you ported please? WebKitTools/DumpRenderTree/DumpRenderTree.gypi:37 + 'chromium/WebThemeEngineDRT.h', We need to add these files in a conditional block for OS=win. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:50 + namespace { We prefer static variables/functions to anonymous namespace in WebKit. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:52 + const SkColor kEdgeColor = SK_ColorBLACK; We don't prepend "k" to constant values in WebKit. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:68 + SkIRect Validate(const SkIRect& rect, WebThemeControlDRT::Type ctype) Function names should start with lowercase letter. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:75 + { A block starting after a case label should be: case XXX: { SkIRect ... } case ... WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:81 + int controlSize = std::min(rect.width(), rect.height()); We should use "using namespace std;" and remove std:: in code. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:99 + WebThemeControlDRT::WebThemeControlDRT(skia::PlatformCanvas* canvas, ditto. We should use "using namespace skia;" WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:146 + int x1, int y1, Indentation looks wrong. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:360 + shortOffset = m_width - kNotchShortOffset; The multiple space seems to make no sense. "-" is not aligned to other operators. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:367 + shortOffset = m_height - kNotchShortOffset; ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:374 + shortOffset = m_height - kNotchShortOffset; ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:396 + line(m_left + kGripLongIndent, m_top + halfHeight, ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:407 + line(m_left + halfWidth, m_top + kGripLongIndent, ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:498 + void We don't need to fold the line after "void". WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:31 + // TestShellWebTheme::Control implements the generic rendering of controls Need to update the comment. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:54 + // third_party/WebKit/WebCore/platform/ThemeTypes.h but is maintained "third_party/WebKit/" is not needed. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:76 + kUnknownState = 0, Enum values should not be prefixed with "k", and indentation is wrong. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:98 + kUnknownType = 0, ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:136 + // If draw_edges is true, draw an edge around the control. If Need to update the comment. WebKitTools/DumpRenderTree/chromium/WebThemeEngineDRT.cpp:47 + #define CHECK_EQ(a, b) \ WebKit ASSERT() is not enough? WebKitTools/DumpRenderTree/chromium/WebThemeEngineDRT.cpp:56 + #define NOT_REACHED_CRASH() \ Just using ASSERT_NOT_REACHED() looks enough. WebKitTools/DumpRenderTree/chromium/WebThemeEngineDRT.cpp:63 + namespace { We prefer static variables/functions in WebKit.
Tony Chang
Comment 5 2010-06-07 17:12:36 PDT
BTW, as the cr-linux bot shows, you're going to have problems rolling DEPS. I think I have a DEPS roll ready for today, I'll upload that in a separate bug.
Roland Steiner
Comment 6 2010-06-07 19:38:17 PDT
Created attachment 58103 [details] patch - WebKit version of WebThemeEngine v2 Applied changes requested in review. Removed changes to DEPS file - this will be handled by another bug entry (see dependencies)
WebKit Review Bot
Comment 7 2010-06-07 19:39:40 PDT
Attachment 58103 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:57: One space before end of line comments [whitespace/comments] [5] WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:65: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kent Tamura
Comment 8 2010-06-07 21:14:36 PDT
Comment on attachment 58103 [details] patch - WebKit version of WebThemeEngine v2 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:31 + // This file implements a simple generic version of the WebKitThemeEngine, WebKitThemeEngine -> WebThemeEngine WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:272 + triangle(m_left, m_bottom, Should m_bottom be aligned to m_bottom in the above triangle() call? WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:292 + const int kCheckIndent = 3; We don't add "k" prefix to a constant values in WebKit. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:49 + namespace WebKit { DRT classes should not be in WebKit namespace. It doesn't need any namespace. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:76 + unknownState = 0, Enum values should start with a capital letter. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.h:98 + unknownType = 0, ditto. WebKitTools/DumpRenderTree/chromium/WebThemeEngineDRT.h:53 + namespace WebKit { DRT classes should not have WebKit namespace.
Roland Steiner
Comment 9 2010-06-08 00:00:04 PDT
Created attachment 58118 [details] patch - WebKit version of WebThemeEngine v2 New version, new luck
WebKit Review Bot
Comment 10 2010-06-08 00:01:04 PDT
Attachment 58118 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:55: One space before end of line comments [whitespace/comments] [5] WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:63: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kent Tamura
Comment 11 2010-06-08 01:22:28 PDT
Comment on attachment 58118 [details] patch - WebKit version of WebThemeEngine v2 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:73 + SkIRect retval = rect; Indentation should be 8 space from the beginning of the line. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:138 + SkIntToScalar(x1), SkIntToScalar(y1), Indentation looks wrong. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:345 + // Draw a box with a notch at the left. Indentation should be 8 spaces from the beginning of the line, and "break;" should be inside of {}. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:353 + // Draw a box with a notch at the right. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:361 + // Draw a box with a notch at the top. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:369 + // Draw a box with a notch at the bottom. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:392 + // Draw a horizontal crosshatch for the grip. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:404 + // Draw a vertical crosshatch for the grip. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:448 + // Draw a narrow rect for the track plus box hatches on the ends. ditto. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:453 + } Close the block after "break;"
Roland Steiner
Comment 12 2010-06-08 20:59:15 PDT
Created attachment 58207 [details] patch - WebKit version of WebThemeEngine v2 changed the switch - case identation as requested
WebKit Review Bot
Comment 13 2010-06-08 21:00:37 PDT
Attachment 58207 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:55: One space before end of line comments [whitespace/comments] [5] WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:63: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kent Tamura
Comment 14 2010-06-08 21:04:17 PDT
Comment on attachment 58207 [details] patch - WebKit version of WebThemeEngine v2 WebKitTools/ChangeLog:8 + Add WebThemeEngineDRT and WebThemeControlDRT ported from Chrome rev. 48907 nit: Chrome -> Chromium WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:243 + line(m_left + 1, i, m_right - 1, i, readOnlyColor); Indentation looks worng. WebKitTools/DumpRenderTree/chromium/WebThemeControlDRT.cpp:265 + break; ditto.
Roland Steiner
Comment 15 2010-06-08 23:57:01 PDT
Committed as r60883 (incl. fixes for the last style issues)
Note You need to log in before you can comment on or make changes to this bug.