Bug 209268

Summary: Implement support for cursor interactions on iPad
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, benjamin, cdumez, cmarcelo, darin, ews-watchlist, megan_gardner, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tim Horton 2020-03-18 19:46:22 PDT
Implement support for cursor interactions on iPad
Comment 1 Tim Horton 2020-03-18 19:47:18 PDT
Created attachment 393938 [details]
Patch
Comment 2 Tim Horton 2020-03-18 19:54:06 PDT
(AFAIK the style bot complaint is only true for macOS, not iOS, so I'm ignoring it)
Comment 3 Darin Adler 2020-03-18 20:45:36 PDT
Comment on attachment 393938 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8122
> +    static dispatch_once_t onceToken;
> +    dispatch_once(&onceToken, ^{
> +        // System apps will always be linked on the current OS, so
> +        // check them before the linked-on-or-after.
> +
> +        // <rdar://problem/59521967> iAd Video does not respond to mouse events, only touch events
> +        if (WebCore::IOSApplication::isNews() || WebCore::IOSApplication::isStocks())
> +            shouldUseMouseGestureRecognizer = NO;
> +
> +        if (WebKit::linkedOnOrAfter(WebKit::SDKVersion::FirstThatSendsNativeMouseEvents))
> +            return;
> +
> +        // <rdar://problem/59155220> Some Feedly UI does not respond to mouse events, only touch events
> +        if (WebCore::IOSApplication::isFeedly())
> +            shouldUseMouseGestureRecognizer = NO;
> +    });

Is the thread safety really needed here? Can it be called from more than one thread concurrently?

- If so, is everything the dispatch_once block does safe to be called from any thread?

- If not, I suggest instead writing this:

    static const BOOL shouldUseMouseGestureRecognizer = []() -> BOOL {
        ... code here ...
    }();

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8127
> +- (void)setupMouseGestureRecognizer

The verb "set up" is two words, not one. So I would name this setUpMouseGestureRecognizer.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8154
> +- (void)setupCursorInteraction

And name this setUpCursorInteraction.
Comment 4 Tim Horton 2020-03-19 11:35:06 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 393938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393938&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8122
> > +    static dispatch_once_t onceToken;
> > +    dispatch_once(&onceToken, ^{
> > +        // System apps will always be linked on the current OS, so
> > +        // check them before the linked-on-or-after.
> > +
> > +        // <rdar://problem/59521967> iAd Video does not respond to mouse events, only touch events
> > +        if (WebCore::IOSApplication::isNews() || WebCore::IOSApplication::isStocks())
> > +            shouldUseMouseGestureRecognizer = NO;
> > +
> > +        if (WebKit::linkedOnOrAfter(WebKit::SDKVersion::FirstThatSendsNativeMouseEvents))
> > +            return;
> > +
> > +        // <rdar://problem/59155220> Some Feedly UI does not respond to mouse events, only touch events
> > +        if (WebCore::IOSApplication::isFeedly())
> > +            shouldUseMouseGestureRecognizer = NO;
> > +    });
> 
> Is the thread safety really needed here? Can it be called from more than one
> thread concurrently?
> 
> - If so, is everything the dispatch_once block does safe to be called from
> any thread?
> 
> - If not, I suggest instead writing this:
> 
>     static const BOOL shouldUseMouseGestureRecognizer = []() -> BOOL {
>         ... code here ...
>     }();

... good point.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8127
> > +- (void)setupMouseGestureRecognizer
> 
> The verb "set up" is two words, not one. So I would name this
> setUpMouseGestureRecognizer.

This file is alllll mixed up about the capitalization of setUp, so I'll be bold and fix the whole thing :)
Comment 5 Tim Horton 2020-03-19 12:06:17 PDT
Created attachment 394003 [details]
Patch
Comment 6 Tim Horton 2020-03-19 12:14:00 PDT
Created attachment 394004 [details]
Patch
Comment 7 Tim Horton 2020-03-19 12:20:08 PDT
https://trac.webkit.org/changeset/258721/webkit
Comment 8 Radar WebKit Bug Importer 2020-03-19 12:21:15 PDT
<rdar://problem/60645044>