[webkit-reviews] review denied: [Bug 48510] [GTK] Implement WebEventFactory, WebErrors classes for WebKit2 : [Attachment 82468] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 15 09:44:34 PST 2011
Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 48510: [GTK] Implement WebEventFactory, WebErrors classes for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48510
Attachment 82468: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=82468&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82468&action=review
> Source/WebCore/platform/PlatformKeyboardEvent.h:182
> + static String keyIdentifierForGdkKeyCode(guint keyCode);
> + static int windowsKeyCodeForKeyEvent(unsigned int keycode);
> + static String singleCharacterString(guint val);
I'd remove the argument names here. Shouldn't windowsKeyCodeForKeyEvent be
called windowsKeyCodeForKeyCode?
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:34
> +#include "WindowsKeyboardCodes.h"
> +
> +#include <gdk/gdk.h>
Please remove this extra newline.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:78
> + if ((state & GDK_SHIFT_MASK)
> + || ((event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE)
&& (reinterpret_cast<GdkEventKey*>(event)->keyval == GDK_KEY_3270_BackTab)))
> + modifiers |= WebEvent::ShiftKey;
Why do we special case BackTab? Instead of casting to a GdkEventKey, it's
better to use event->key.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:163
> + float deltaX = 0;
> + float deltaY = 0;
Why have a pair of floats and a FloatSize? It would make more sense just to
keep a FloatSize and fiddle that.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> + float wheelTicksX = 0;
> + float wheelTicksY = 0;
You should be able to remove these, seee below.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:166
> + float scrollbarPixelsPerLine = 40;
This should be static const and there should be a comment explaining where it
comes from, since it's a magic number. I'd move it down closer to where it's
used to. There's no need to declare all the variables at the beginning of the
method.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:187
> + deltaY = delta;
> + break;
> + case GDK_SCROLL_DOWN:
> + deltaY = -delta;
> + break;
> + case GDK_SCROLL_LEFT:
> + deltaX = delta;
> + break;
> + case GDK_SCROLL_RIGHT:
> + deltaX = -delta;
> + break;
If delta is always 1, it's much clearer just to say 1 or -1. Even better, just
say eventDelta = FloatSize(1, 0); etc.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> + FloatSize eventWheelTicks(wheelTicksX, wheelTicksY);
Why not just move this definition above where you adjust deltaX and deltaY so
that you can dispense with wheelTicks{X,Y}?
> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:47
> +#define WebURLErrorDomain "URLErrorDomain"
> +
> +enum {
> + WebURLErrorUnknown = -1,
> + WebURLErrorCancelled = -999,
> + WebURLErrorBadURL = -1000,
> + WebURLErrorTimedOut = -1001,
> + WebURLErrorUnsupportedURL = -1002,
> + WebURLErrorCannotFindHost = -1003,
> + WebURLErrorCannotConnectToHost = -1004,
> + WebURLErrorNetworkConnectionLost = -1005,
I believe this file has totally changed since this patch was written. You
should now be calling WebError::webKitErrorDomain and Windows and Mac do not
define all these enums.
More information about the webkit-reviews
mailing list