Bug 189621 - It should be possible to get the mouse event modifiers from an injected bundle.
Summary: It should be possible to get the mouse event modifiers from an injected bundle.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-14 09:28 PDT by Per Arne Vollan
Modified: 2019-02-18 14:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.29 KB, patch)
2018-09-14 09:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2018-09-18 10:08 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.77 KB, patch)
2019-01-05 12:13 PST, Per Arne Vollan
dbates: review+
Details | Formatted Diff | Diff
Patch (3.22 KB, patch)
2019-02-18 12:09 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-09-14 09:28:04 PDT
Currently, event modifiers are not available through the API.
Comment 1 Per Arne Vollan 2018-09-14 09:30:20 PDT
Created attachment 349766 [details]
Patch
Comment 2 Per Arne Vollan 2018-09-14 09:34:13 PDT
rdar://problem/44437279
Comment 3 Anders Carlsson 2018-09-18 08:01:42 PDT
I don't think this is the right approach - it would be better to add new client callbacks that include modifiers.
Comment 4 Per Arne Vollan 2018-09-18 10:08:11 PDT
Created attachment 350029 [details]
Patch
Comment 5 Per Arne Vollan 2018-09-18 10:08:59 PDT
(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.
Comment 6 Brent Fulgham 2019-01-04 10:15:37 PST
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?
Comment 7 Anders Carlsson 2019-01-04 11:48:38 PST
(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.
Comment 8 Brent Fulgham 2019-01-04 12:10:23 PST
(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?
Comment 9 Per Arne Vollan 2019-01-05 12:13:07 PST
Created attachment 358441 [details]
Patch
Comment 10 Per Arne Vollan 2019-01-05 12:15:06 PST
(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!
Comment 11 Brent Fulgham 2019-02-14 09:12:47 PST
Dan, can you look at this change and give us your thoughts?
Comment 12 Daniel Bates 2019-02-14 09:30:28 PST
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<>?
Comment 13 Daniel Bates 2019-02-14 09:36:44 PST
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.
Comment 14 Per Arne Vollan 2019-02-14 09:41:03 PST
(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.
Comment 15 Per Arne Vollan 2019-02-18 12:09:18 PST
Created attachment 362309 [details]
Patch
Comment 16 Per Arne Vollan 2019-02-18 12:12:45 PST
(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.
Comment 17 WebKit Commit Bot 2019-02-18 14:43:13 PST
Comment on attachment 362309 [details]
Patch

Clearing flags on attachment: 362309

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