WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
patch - WebKit version of WebThemeEngine v2
(57.48 KB, patch)
2010-06-07 19:38 PDT
,
Roland Steiner
tkent
: review-
Details
Formatted Diff
Diff
patch - WebKit version of WebThemeEngine v2
(57.53 KB, patch)
2010-06-08 00:00 PDT
,
Roland Steiner
tkent
: review-
Details
Formatted Diff
Diff
patch - WebKit version of WebThemeEngine v2
(57.31 KB, patch)
2010-06-08 20:59 PDT
,
Roland Steiner
tkent
: review+
tkent
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Attachment 58013
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/3163134
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.
Top of Page
Format For Printing
XML
Clone This Bug