[webkit-reviews] review denied: [Bug 26988] Haiku-specific files for WebCore : [Attachment 33093] 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 33093: Patch to add a first bunch of Haiku-specific files for
WebCore.
https://bugs.webkit.org/attachment.cgi?id=33093&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few last things to address.

> Other ports designed this class with the constructor as private and have a
static method to create the
> clipboard.
Yep, that is a standard pattern. My mistake in missing it :)


> > "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.)

> On this point I don't really agree. I know what you mean but, as I looked in
> other ports, none of them used an enum but a boolean.

This is a new style that WebKit is moving to that likely post dates the other
implementations, so that's why they use an enum.

For example see https://bugs.webkit.org/show_bug.cgi?id=26695, where this was
said when someone was adding a new bool parameter:
  We're trying to avoid adding boolean parameters, because they are not very
  clear (when reading code like "hitTestResultAtPoint(point, true, false,
true)",
  what do the booleans mean?). One strategy to replace a boolean parameter is
to
  use a two-value enum, like this:

  enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
 
Although in this case, it does just pass it along to the base class which takes
bool, so perhaps it really isn't a new bool parameter.

Thus, it seems ok in this instance.  Please keep this in mind when doing other
apis though.


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

> +#include "IntPoint.h"
> +#include "FileList.h"
> +#include "PlatformString.h"
> +#include "StringHash.h"
> +
> +#include "NotImplemented.h"

Please sort this with the other "" headers.

> +
> +#include <wtf/HashTable.h>

Please sort this with the other #include <> headers.

> +
> +#include <app/Clipboard.h>
> +#include <Message.h>
> +#include <String.h>



> +void ClipboardHaiku::clearData(const String& type)
> +{
> +    BMessage* data;

It would be better if you declared this where it was used and initialized.

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

It would look like this:
  BMessage* data = be_clipboard->Data();

This same pattern is throughout this code.

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

> +
> +#include <support/Locker.h>
> +#include <app/Clipboard.h>

Please sort these two.


> diff --git a/WebCore/platform/haiku/CursorHaiku.cpp
b/WebCore/platform/haiku/CursorHaiku.cpp
> +static Cursor
global_cursor(static_cast<PlatformCursor>(B_CURSOR_SYSTEM_DEFAULT));
> +static Cursor ibeam_cursor(static_cast<PlatformCursor>(B_CURSOR_I_BEAM));

As noted before: Improper casing global_cursor -> globalCursor