[webkit-reviews] review denied: [Bug 26988] Haiku-specific files for WebCore : [Attachment 32868] Patch to add a first bunch of Haiku-specific files for WebCore.

[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 26988: Haiku-specific files for WebCore
https://bugs.webkit.org/show_bug.cgi?id=26988

Attachment 32868: Patch to add a first bunch of Haiku-specific files for
WebCore.
https://bugs.webkit.org/attachment.cgi?id=32868&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> diff --git a/WebCore/platform/haiku/ClipboardHaiku.cpp
b/WebCore/platform/haiku/ClipboardHaiku.cpp

> +#include "HashTable.h"

Should be "#include <wtf/HashTable.h>"

> +
> +
> +namespace WebCore {
> +
> +ClipboardHaiku::ClipboardHaiku(ClipboardAccessPolicy policy, bool
forDragging) 
> +    : Clipboard(policy, forDragging)
> +{
> +}
> +
> +void ClipboardHaiku::clearData(const String& type)
> +{
> +    BMessage *data = (BMessage *) NULL;
1. Star in wrong place.
BMessage* data

2. Use c++ style casts. (BMessage *)  (but I don't think the cast is necessary.


3. Use 0 instead of NULL.

This line is repeated a lot in this patch, but I'm only flagging this first
instace.


> +
> +    if (be_clipboard->Lock()) {
> +	   if ((data = be_clipboard->Data())) {
Since there doesn't seem to be any benefit of putting this in the if other than
saving a line.	Why not write it as two lines (which in my opinion makes it
more readable)?

  data = be_clipboard->Data();
  if (data) {

It isn't necessary to be this but it would be nice imo.



> +// extensions beyond IE's API
Format comments like sentences (begin with a capital and end with a period).

> +IntPoint ClipboardHaiku::dragLocation() const 
> +{ 
> +    notImplemented();
> +    return IntPoint(0,0);
Add a space after the comma.


> +DragImageRef ClipboardHaiku::createDragImage(IntPoint& dragLoc) const
Avoid abbreviations: dragloc -> dragLocation

> diff --git a/WebCore/platform/haiku/ClipboardHaiku.h
b/WebCore/platform/haiku/ClipboardHaiku.h
> +    // State available during IE's events for drag and drop and copy/paste
> +    class ClipboardHaiku : public Clipboard {
> +    public:
> +	   ClipboardHaiku(ClipboardAccessPolicy policy, bool forDragging);

"bool forDragging"
Please use an enum instead of a bool. (See
http://trac.webkit.org/browser/trunk/WebCore/workers/WorkerScriptLoader.h for
an example of where this was done.)



> diff --git a/WebCore/platform/haiku/CursorHaiku.cpp
b/WebCore/platform/haiku/CursorHaiku.cpp

> +namespace WebCore {
> +
> +Cursor::Cursor(PlatformCursor p)
"p" -- Use words instead f abbreviations for variable names.


> +static Cursor global_cursor((PlatformCursor)B_CURSOR_SYSTEM_DEFAULT);

Improper casing global_cursor -> globalCursor
Use C++ style casting (static_cast/reinterpret_cast, etc.) instead of C style
casting.


> +static Cursor ibeam_cursor((PlatformCursor)B_CURSOR_I_BEAM);

Same comments as above.