[Webkit-unassigned] [Bug 26988] Haiku-specific files for WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26988


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33093|review?                     |review-
               Flag|                            |




--- Comment #34 from David Levin <levin at chromium.org>  2009-07-20 10:56:04 PDT ---
(From update of attachment 33093)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.