Bug 189621 – It should be possible to get the mouse event modifiers from an injected bundle.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Per Arne Vollan]

Currently, event modifiers are not available through the API.

[Per Arne Vollan]

Created attachment 349766 [details]
Patch

[Per Arne Vollan]

rdar://problem/44437279

[Anders Carlsson]

I don't think this is the right approach - it would be better to add new client callbacks that include modifiers.

[Per Arne Vollan]

Created attachment 350029 [details]
Patch

[Per Arne Vollan]

(In reply to Anders Carlsson from comment #3)
> I don't think this is the right approach - it would be better to add new
> client callbacks that include modifiers.

Thanks for reviewing :) I have updated the patch.

[Brent Fulgham]

Comment on attachment 350029 [details]
Patch

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

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=189621

<Radar #?>

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116
> +} WKBundlePageOverlayClientV2;

It seems like this change requires clients to adopt a new Client API. Since this is a fix for a regression, it seems like this is not the best approach.

Could we do something on the WebKit side (only) to fix this?

[Anders Carlsson]

(In reply to Brent Fulgham from comment #6)
> Comment on attachment 350029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350029&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=189621
> 
> <Radar #?>
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116
> > +} WKBundlePageOverlayClientV2;
> 
> It seems like this change requires clients to adopt a new Client API. Since
> this is a fix for a regression, it seems like this is not the best approach.
> 
> Could we do something on the WebKit side (only) to fix this?

See https://bugs.webkit.org/show_bug.cgi?id=189621#c3 for why I thought a new client API was necessary.

[Brent Fulgham]

(In reply to Anders Carlsson from comment #7)
> (In reply to Brent Fulgham from comment #6)
> > Could we do something on the WebKit side (only) to fix this?
> 
> See https://bugs.webkit.org/show_bug.cgi?id=189621#c3 for why I thought a
> new client API was necessary.

I don't think that comment helps me understand (except for reiterating that you think a new client API is better).

But, if you prefer the API addition version, I'm fine with that. Could you r+ the patch so we could get it into a build soon?

[Per Arne Vollan]

Created attachment 358441 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #6)
> Comment on attachment 350029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350029&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=189621
> 
> <Radar #?>
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116
> > +} WKBundlePageOverlayClientV2;
> 
> It seems like this change requires clients to adopt a new Client API. Since
> this is a fix for a regression, it seems like this is not the best approach.
> 
> Could we do something on the WebKit side (only) to fix this?

I have updated the patch to avoid changing any client code by swizzling [NSEvent modifierFlags].

Thanks for reviewing!

[Brent Fulgham]

Dan, can you look at this change and give us your thoughts?

[Daniel Bates]

Comment on attachment 358441 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:389
> +    // Swizzle [NSEvent modiferFlags], since it always returns 0 when the WindowServer is blocked.

Grep the code to ensure we don’t have an existing Swizzler class or helpers. Maybe we only have for TestWebKitAPI?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390
> +    Method method = class_getClassMethod(objc_getClass("NSEvent"), @selector(modifierFlags));

[NSEvent class]?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:391
> +    method_setImplementation(method, (IMP)currentModifierFlags);

Reinterpret_cast<>?

[Daniel Bates]

Comment on attachment 358441 [details]
Patch

Something about the placement of this code seems a bit weird. Not sure what it is weird though. Grep the code for examples.

[Per Arne Vollan]

(In reply to Daniel Bates from comment #12)
> Comment on attachment 358441 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358441&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:389
> > +    // Swizzle [NSEvent modiferFlags], since it always returns 0 when the WindowServer is blocked.
> 
> Grep the code to ensure we don’t have an existing Swizzler class or helpers.
> Maybe we only have for TestWebKitAPI?
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390
> > +    Method method = class_getClassMethod(objc_getClass("NSEvent"), @selector(modifierFlags));
> 
> [NSEvent class]?
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:391
> > +    method_setImplementation(method, (IMP)currentModifierFlags);
> 
> Reinterpret_cast<>?

Thanks for reviewing, Dan! I will update the patch.

[Per Arne Vollan]

Created attachment 362309 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #15)
> Created attachment 362309 [details]
> Patch

I moved the swizzling to InjectedBundle::initialize, since [NSEvent currentModifiers] is only used by injected bundles.

[WebKit Commit Bot]

Comment on attachment 362309 [details]
Patch

Clearing flags on attachment: 362309

Committed r241737: <https://trac.webkit.org/changeset/241737>