Bug 150179 - Add magnify and rotate gesture event support for Mac WebKit2
Summary: Add magnify and rotate gesture event support for Mac WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-15 12:00 PDT by Tim Horton
Modified: 2015-10-19 12:20 PDT (History)
11 users (show)

See Also:


Attachments
patch (94.91 KB, patch)
2015-10-15 12:00 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (98.02 KB, patch)
2015-10-16 12:29 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (98.04 KB, patch)
2015-10-16 15:05 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (96.21 KB, patch)
2015-10-16 15:30 PDT, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-10-15 12:00:17 PDT
Created attachment 263176 [details]
patch

Add magnify and rotate gesture event support for Mac WebKit2
Comment 1 WebKit Commit Bot 2015-10-15 12:02:16 PDT
Attachment 263176 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/ViewGestureController.h:103:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:64:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:65:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:67:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 10 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Tim Horton 2015-10-16 12:29:48 PDT
Created attachment 263311 [details]
Patch
Comment 3 WebKit Commit Bot 2015-10-16 12:31:40 PDT
Attachment 263311 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:64:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:65:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:67:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2015-10-16 15:05:53 PDT
Created attachment 263337 [details]
Patch
Comment 5 WebKit Commit Bot 2015-10-16 15:08:20 PDT
Attachment 263337 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:64:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:65:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:67:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Tim Horton 2015-10-16 15:30:41 PDT
Created attachment 263341 [details]
Patch
Comment 7 WebKit Commit Bot 2015-10-16 15:34:20 PDT
Attachment 263341 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:64:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:65:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/Shared/mac/NativeWebGestureEventMac.mm:67:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 4 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Darin Adler 2015-10-18 16:01:48 PDT
Comment on attachment 263341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263341&action=review

> Source/WebCore/page/EventHandler.h:68
>  #if ENABLE(IOS_TOUCH_EVENTS)
> -#include <wtf/HashSet.h>
>  #include <wtf/Vector.h>
>  #endif
>  
> +#if ENABLE(IOS_TOUCH_EVENTS) || ENABLE(MAC_GESTURE_EVENTS)
> +#include <wtf/HashSet.h>
> +#endif

These kinds of generic WTF includes should probably be included without an #if. Having a precise include set for each combination doesn’t seem helpful.

> Source/WebKit2/Shared/NativeWebGestureEvent.h:38
> +class NativeWebGestureEvent : public WebGestureEvent {

final?

> Source/WebKit2/Shared/NativeWebGestureEvent.h:42
> +    explicit NativeWebGestureEvent(NSEvent *, NSView *);
> +
> +    NSEvent* nativeEvent() const { return m_nativeEvent.get(); }

NSEvent * formatting inconsistent here.

> Source/WebKit2/Shared/mac/WebGestureEvent.h:68
> +    WebCore::IntPoint m_position;

int, not double?

> Source/WebKit2/Shared/mac/WebGestureEvent.h:70
> +    float m_gestureScale;
> +    float m_gestureRotation;

float, not double?

> Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp:150
> +    RefPtr<EventDispatcher> eventDispatcher(this);

I normally like "= this" instead of "(this)"; not sure why. I guess that "()" is my initialization syntax of last resort. I even prefer the "{}" since it’s pickier about types and less likely to look like a function declaration.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2195
> +        CurrentEvent currentEvent(gestureEvent);
> +
> +        handled = handleGestureEvent(gestureEvent, m_page.get());

No need for the blank line. It’s not like local variables need to be separated at the top. Reads better without it.
Comment 9 Tim Horton 2015-10-19 11:01:47 PDT
(In reply to comment #8)
> > Source/WebKit2/Shared/mac/WebGestureEvent.h:68
> > +    WebCore::IntPoint m_position;
> 
> int, not double?

Matches all the other events.

> > Source/WebKit2/Shared/mac/WebGestureEvent.h:70
> > +    float m_gestureScale;
> > +    float m_gestureRotation;
> 
> float, not double?

Matches WebTouchEvent.
Comment 10 Tim Horton 2015-10-19 11:12:11 PDT
http://trac.webkit.org/changeset/191299
Comment 11 Tim Horton 2015-10-19 12:20:20 PDT
http://trac.webkit.org/changeset/191305