[webkit-reviews] review denied: [Bug 8515] Linux porting compile bug : [Attachment 8761] Gdk port specific sources

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 8515: Linux porting compile bug
http://bugzilla.opendarwin.org/show_bug.cgi?id=8515

Attachment 8761: Gdk port specific sources
http://bugzilla.opendarwin.org/attachment.cgi?id=8761&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The .cpp files are missing includes of "config.h". Every .cpp file needs to
include "config.h". Also, each .cpp file should includes its own .h file first,
right after "config.h".

I don't think the file "PlatformKeyboardCodes.h" should have the name
"Platform" in its name. The only reason the event files have Platform in their
name is that they conflict with DOM classes. I think "KeyboardCodes.h" would be
a fine name for the file.

Headers must not include "config.h". That's included by every .cpp file
instead.

    return IntPoint(contentsPoint) + scrollOffset();

Why the cast to IntPoint here?

    m_isKeyUp = bool( event->type == GDK_KEY_RELEASE);
    m_shiftKey = bool(event->key.state & GDK_SHIFT_MASK != 0);
    m_ctrlKey = bool(event->key.state & GDK_CONTROL_MASK != 0);
    m_altKey = bool(event->key.state & GDK_MOD1_MASK != 0); /*alt ?*/
    m_metaKey = bool(event->key.state & GDK_MOD2_MASK != 0); /*meta ?*/

What are all those "bool" casts about? Explicit casts should not be needed.

    m_text = String(event->key.string);
    m_unmodifiedText = String(event->key.string);

And these String casts also should not be needed.

Lots of these files say Copyright 2005, 2006 in them. Did you really publish
the code you are contributing in 2005 (publish roughly meaning something like
"make available to others")? If not, then you shouldn't list 2005.

I see a mix of coding and formatting styles in this code. Most is not formatted
according to the style we use in WebCore, for example there's a lot of
8-character indenting, spaces inside parentheses, braces around single-line if
statements, return types on a separate line from function names, empty
functions are {} on one line, etc. I presume you are planning to use astyle to
fix some of the formatting issues. It would be good to fix the style to match
other WebCore code and current gudelines as much as possible before landing all
these new files, although not mandatory.