Bug 40052 - [DRT/Chromium] Upstream test_shell_webthemeengine as WebThemeEngineDRT
Summary: [DRT/Chromium] Upstream test_shell_webthemeengine as WebThemeEngineDRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Roland Steiner
URL:
Keywords:
Depends on: 40277
Blocks: 35902
  Show dependency treegraph
 
Reported: 2010-06-02 03:52 PDT by Roland Steiner
Modified: 2010-06-08 23:57 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2010-06-02 03:52:08 PDT
Upstream test_shell_webthemeengine for DRT
Comment 1 Roland Steiner 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).
Comment 2 WebKit Review Bot 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.
Comment 3 WebKit Review Bot 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
Comment 4 Kent Tamura 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.
Comment 5 Tony Chang 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.
Comment 6 Roland Steiner 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)
Comment 7 WebKit Review Bot 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.
Comment 8 Kent Tamura 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.
Comment 9 Roland Steiner 2010-06-08 00:00:04 PDT
Created attachment 58118 [details]
patch - WebKit version of WebThemeEngine v2

New version, new luck
Comment 10 WebKit Review Bot 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.
Comment 11 Kent Tamura 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;"
Comment 12 Roland Steiner 2010-06-08 20:59:15 PDT
Created attachment 58207 [details]
patch - WebKit version of WebThemeEngine v2

changed the switch - case identation as requested
Comment 13 WebKit Review Bot 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.
Comment 14 Kent Tamura 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.
Comment 15 Roland Steiner 2010-06-08 23:57:01 PDT
Committed as r60883 (incl. fixes for the last style issues)