[webkit-reviews] review denied: [Bug 40052] [DRT/Chromium] Upstream test_shell_webthemeengine as WebThemeEngineDRT : [Attachment 58013] patch - WebKit version of WebThemeEngine

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 40052: [DRT/Chromium] Upstream test_shell_webthemeengine as
WebThemeEngineDRT
https://bugs.webkit.org/show_bug.cgi?id=40052

Attachment 58013: patch - WebKit version of WebThemeEngine
https://bugs.webkit.org/attachment.cgi?id=58013&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.