Bug 237342

Summary: Mousemove events double-firing in Safari
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, kkinnunen, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
Patch
none
Patch none

Description Chris Dumez 2022-03-01 13:34:05 PST 
Mousemove events double-firing in Safari
Comment 1 Chris Dumez 2022-03-01 13:34:16 PST 
<rdar://88025610>
Comment 2 Chris Dumez 2022-03-01 13:35:53 PST 
Created attachment 453536 [details]
WIP Patch
Comment 3 Chris Dumez 2022-03-01 15:50:33 PST 
Created attachment 453544 [details]
WIP Patch
Comment 4 Chris Dumez 2022-03-02 07:57:38 PST 
Created attachment 453612 [details]
Patch
Comment 5 Wenson Hsieh 2022-03-02 08:36:10 PST 
Comment on attachment 453612 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:696
> +- (void)_simulateMouseMove:(NSEvent *)event;

Let's move this to WKWebViewPrivateForTestingMac.h instead.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:185
> +    WebKit::WebViewImpl* _impl;

In modern code, ObjC ivars should go into the @implementation.

I think ideally, this would also be a WeakPtr instead of a raw pointer.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:191
> +- (instancetype)initWithViewImpl:(WebKit::WebViewImpl *)impl

I think we would prefer to take WebKit::WebViewImpl& here if we don't expect to pass `nullptr`.
Comment 6 Chris Dumez 2022-03-02 09:10:46 PST 
Created attachment 453622 [details]
Patch
Comment 7 EWS 2022-03-02 11:41:11 PST 
Committed r290743 (247987@main): <https://commits.webkit.org/247987@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453622 [details].
Comment 8 Darin Adler 2022-03-02 11:42:54 PST 
Comment on attachment 453622 [details]
Patch

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

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:617
> +            [static_cast<WKWebView*>(targetView) _simulateMouseMove:event];

In new code we should make a habit of using checked_objc_cast instead of static_cast.
Comment 9 Chris Dumez 2022-03-02 12:10:53 PST 
(In reply to Darin Adler from comment #8)
> Comment on attachment 453622 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453622&action=review
> 
> > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:617
> > +            [static_cast<WKWebView*>(targetView) _simulateMouseMove:event];
> 
> In new code we should make a habit of using checked_objc_cast instead of
> static_cast.

Nice, I'll fix.
Comment 10 Chris Dumez 2022-03-02 15:09:38 PST 
(In reply to Chris Dumez from comment #9)
> (In reply to Darin Adler from comment #8)
> > Comment on attachment 453622 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=453622&action=review
> > 
> > > Tools/WebKitTestRunner/mac/EventSenderProxy.mm:617
> > > +            [static_cast<WKWebView*>(targetView) _simulateMouseMove:event];
> > 
> > In new code we should make a habit of using checked_objc_cast instead of
> > static_cast.
> 
> Nice, I'll fix.

Fixed in <https://commits.webkit.org/r290757>, thanks!