[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





--- Comment #23 from Alejandro G. Castro <alex at igalia.com>  2011-02-16 06:16:09 PST ---
Thanks for the review.

(In reply to comment #22)
> (From update of attachment 82468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82468&action=review
> 
> > 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.
>

I did not check the code when reviewing, apparently it was the bug 51587 that added that code to the original file this code comes from.

> > 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.
> 

I agree, I'll change this code.

> > 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}?
>

I agree, I'm going to review it. This code comes from the old WheelEventEvent, I guess that is the reason for most of the decisions there.

> > 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.

Yep, good point.

Thanks again.

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