Bug 219024

Created attachment 414318 [details]
WIP patch

This is WIP, just to check if it builds in other ports and the result of imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_hoverable_pointers.html

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 414323 [details]
WIP patch

Created attachment 414324 [details]
WIP patch

Created attachment 414334 [details]
WIP patch

I don't understand the build failures on tv and watch platform. I need some help with those.

(In reply to Carlos Garcia Campos from comment #6)
> I don't understand the build failures on tv and watch platform. I need some
> help with those.

It's a classic unified source 'using namespace' conflict. Might be an indirect one (WebCore -> WebKit -> top-level). I've applied your patch and started a build, I'll see if I can find the minimal change required to get things back on track.

(In reply to Tim Horton from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > I don't understand the build failures on tv and watch platform. I need some
> > help with those.
> 
> It's a classic unified source 'using namespace' conflict. Might be an
> indirect one (WebCore -> WebKit -> top-level). I've applied your patch and
> started a build, I'll see if I can find the minimal change required to get
> things back on track.

Thanks!

I landed http://trac.webkit.org/changeset/269901/webkit and hit the retry button here (no idea if I waited long enough between doing those things, though... but it should be OK eventually).

It worked, thank you!

Created attachment 414798 [details]
Patch

Comment on attachment 414798 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Update expectations of
> +        imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_hoverable_pointers.html. The test is now
> +        timing out, but because tesdriver doesn't correctly handle the iframe element.

See bug #219255

<rdar://problem/71747688>

Comment on attachment 414798 [details]
Patch

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

r=me, overall the approach looks straightforward.

Unfortunately, watchOS build is still failing:

In file included from /Volumes/Data/worker/watchOS-7-Build-EWS/build/WebKitBuild/Release-watchos/DerivedSources/WebKit2/unified-sources/UnifiedSource46-mm.mm:8:
/Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/UIProcess/ios/forms/WKFocusedFormControlView.mm:134:27: error: reference to 'UIEvent' is ambiguous
- (BOOL)handleWheelEvent:(UIEvent *)event
                          ^

This looks like another UnifiedSources thing, ugh. I'll try to figure out what the right include is.

> Source/WebKit/Shared/WebMouseEvent.h:73
> +    const String& pointerType() const { return m_pointerType; }

It's a little weird to me that the pointerType is a string that we pass around. Is there a reason to not use an enum?

(In reply to Brian Burg from comment #14)
> Comment on attachment 414798 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414798&action=review
> 
> r=me, overall the approach looks straightforward.
> 
> Unfortunately, watchOS build is still failing:
> 
> In file included from
> /Volumes/Data/worker/watchOS-7-Build-EWS/build/WebKitBuild/Release-watchos/
> DerivedSources/WebKit2/unified-sources/UnifiedSource46-mm.mm:8:
> /Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/UIProcess/ios/
> forms/WKFocusedFormControlView.mm:134:27: error: reference to 'UIEvent' is
> ambiguous
> - (BOOL)handleWheelEvent:(UIEvent *)event
>                           ^
> 
> This looks like another UnifiedSources thing, ugh. I'll try to figure out
> what the right include is.

I suspect this is happening because we're trying to import `PointerEvent.h` in these UI process-side headers, which also pulls in DOM/bindings-related stuff like WindowProxy, FrameView, Node, etc.

Instead, we should probably pull the enum types and declarations we need to reference out into a separate header, and just import that instead of `PointerEvent.h`.

> 
> > Source/WebKit/Shared/WebMouseEvent.h:73
> > +    const String& pointerType() const { return m_pointerType; }
> 
> It's a little weird to me that the pointerType is a string that we pass
> around. Is there a reason to not use an enum?

Comment on attachment 414798 [details]
Patch

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

>>> Source/WebKit/Shared/WebMouseEvent.h:73
>>> +    const String& pointerType() const { return m_pointerType; }
>> 
>> It's a little weird to me that the pointerType is a string that we pass around. Is there a reason to not use an enum?
> 
> 

I guess it's because pointerType is defined in the spec as a DOMString. The spec says it can be mouse, pen and touch, but also other devices that must be prefixed. I guess we could use an enum internally indeed, but it's probably out fo the scope of this bug.

Created attachment 415738 [details]
Patch for landing

EWS is green this time!

Committed r270582: <https://trac.webkit.org/changeset/270582>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415738 [details].