[webkit-reviews] review denied: [Bug 26949] WebCore part of the Haiku WebKit port : [Attachment 32867] Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 26949: WebCore part of the Haiku WebKit port
https://bugs.webkit.org/show_bug.cgi?id=26949

Attachment 32867: Patch to add Haiku-specific drawing files for
WebCore/platform/graphics/.
https://bugs.webkit.org/attachment.cgi?id=32867&action=review

------- Additional Comments from David Levin <levin at chromium.org>
You may want to consider using the (alpha version) lint tool for WebKit.

I downloaded this patch and ran on your files in this patch like this:
   python WebKitTools/Scripts/modules/cpplint.py
WebCore/platform/graphics/haiku/*.cpp

It found these errors:
WebCore/platform/graphics/haiku/FloatRectHaiku.cpp:44:	Missing space after , 
[whitespace/comma] [3]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:51:  This { should be
at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:229:  Tab found;
better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:495:  Place brace on
its own line for function definitions.	[whitespace/braces] [4]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:495:  Missing space
before {  [whitespace/braces] [5]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:511:  Tab found;
better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:516:  Tab found;
better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:541:  Missing space
after ,  [whitespace/comma] [3]
WebCore/platform/graphics/haiku/ImageHaiku.cpp:109:  Tab found; better to use
spaces	[whitespace/tab] [1]
WebCore/platform/graphics/haiku/ImageHaiku.cpp:127:  Tab found; better to use
spaces	[whitespace/tab] [1]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:78:  Tests for true/false,
zero/non-zero, etc. should be done without equality comparisons. 
[readability/comparison_to_zero] [5]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:91:  Extra space before ) 
[whitespace/parens] [2]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:124:  Missing space before
( in if(  [whitespace/parens] [5]
WebCore/platform/graphics/haiku/IntRectHaiku.cpp:45:  Missing space after , 
[whitespace/comma] [3]

When glancing through the code, I also noticed the following issues.

In WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp

> 56 BView* view;
"Prefix C++ data members with "m_"."So it should be m_view 

> 59 GraphicsContextPlatformPrivate::GraphicsContextPlatformPrivate(BView*
p_view)
p_view is not the correct format for a variable.

> 210	  // creatin the region.
typo: creatin

> 318 void GraphicsContext::setLineJoin(LineJoin lj)
Use full words, except in the rare case where an abbreviation would be more
canonical and easier to understand.
s/lj/lineJoin/


In WebCore/platform/graphics/haiku/ImageBufferData.h
#include "OwnPtr.h" should be #include <wtf/OwnPtr.h>


In WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp
> 65	 if (uContents[0] == 0x89 &&
> 66	     uContents[1] == 0x50 &&
Boolean expressions at the same nesting level that span multiple lines should
have their operators on the left side of the line instead of the right side.

So it should look like this:
     if (uContents[0] == 0x89
	 && uContents[1] == 0x50

> 72	 // if (uContents[0] == 0xFF &&
Commented out code isn't checked in.  If you want to note the need to address
JPEG, you could put a 
  // FIXME: Implement JPEG support.
comment here.