[Webkit-unassigned] [Bug 48510] [GTK] Implement WebEventFactory, WebErrors classes for WebKit2

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #22 from Martin Robinson <mrobinson at webkit.org>  2011-02-15 09:44:35 PST ---
(From update of attachment 82468)
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.

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