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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 34263: Patch to add a fourth bunch of Haiku-specfic files to
WebCore.
https://bugs.webkit.org/attachment.cgi?id=34263&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +#include "DocumentFragment.h"
> +#include "Editor.h"
> +#include "Frame.h"
> +#include "KURL.h"
> +#include "NotImplemented.h"
> +#include "markup.h"
> +
> +#include <support/Locker.h>
> +#include <Clipboard.h>
> +#include <Message.h>
> +#include <String.h>

WebKit style is to use a single paragraph for these, not two.

> +#include "KeyboardCodes.h"
> +#include "NotImplemented.h"
> +
> +#include <InterfaceDefs.h>
> +#include <Message.h>
> +#include <String.h>

Ditto.

> +static String keyIdentifierForHaikuKeyCode(char singleByte, int32 keyCode)

Are you sure you want to use the type "int32" here? We'd normally use int, or
perhaps int32_t. Is this something specific to the Haiku platform?

> +{
> +	   switch (singleByte) {

This is indented two many spaces.

> +		   return String::format("U+%04X", toASCIIUpper((int)keyCode));


You should not have to use any type cast with toASCIIUpper. Does this help in
some way?

> +	       };

Extra semicolon here.

> +	       return String::format("U+%04X", toASCIIUpper((int)keyCode));

If you put this after the switch statement then you could share the two cases
of it.

> +    switch (singleByte) {

For some reason this second switch statement is indented differently than the
first. Could you make both consistent, and follow the WebKit coding style
guide?

> +PlatformKeyboardEvent::PlatformKeyboardEvent(BMessage* message)
> +{
> +    BString bytes = message->FindString("bytes");
> +
> +    m_text = String::fromUTF8(bytes.String(), bytes.Length());
> +    m_unmodifiedText = String(bytes.String(), bytes.Length());
> +    m_keyIdentifier = keyIdentifierForHaikuKeyCode(bytes.ByteAt(0),
message->FindInt32("key"));
> +
> +    if (message->what == B_KEY_UP)
> +	   m_type = KeyUp;
> +    else if (message->what == B_KEY_DOWN)
> +	   m_type = KeyDown;
> +
> +    m_autoRepeat = true;
> +    m_ctrlKey = false;
> +    m_altKey = false;
> +    m_metaKey = false;
> +    m_windowsVirtualKeyCode = windowsKeyCodeForKeyEvent(bytes.ByteAt(0));
> +    m_isKeypad = false;
> +    m_shiftKey = false;
> +}

It's better style to use member initialization as much as possible, and only
use assignment when that is impractical. So at least for most of these booleans
you should switch to that.

> +PlatformMouseEvent::PlatformMouseEvent(const BMessage* message)

Same comment about member-wise initialization for this constructor.

> +    m_deltaX *= -static_cast<float>(cScrollbarPixelsPerLineStep);
> +    m_deltaY *= -static_cast<float>(cScrollbarPixelsPerLineStep);

Is the cast to float needed? What effect does it have? I'd expect this to work
perfectly without the cast.

review- because there are enough comments above that could be simply addressed