Bug 147573 – Need WKWebView API to enable/disable link preview

• Component: WebKit2
• Status: RESOLVED FIXED

[Beth Dakin]

Need WKWebView API to enable/disable link preview

rdar://problem/22077836

[Beth Dakin]

Created attachment 258090 [details]
Patch

Comment on attachment 258090 [details]
Patch

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

> Source/WebKit2/ChangeLog:20
> +        (-[WKWebView _ignoresNonWheelEvents]):

I don’t think that one has changed.

> Source/WebKit2/ChangeLog:33
> +        (-[WKView _setIgnoresNonWheelEvents:]):

Nor that one.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:354
> -    _allowsLinkPreview = YES;
> +    _allowsLinkPreview = NO;

ivars are NO by default, so you can just remove this assignment.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1858
> +- (BOOL)allowsLinkPreview
> +{
> +    return [_wkView allowsLinkPreview];
> +}
> +
> +- (void)setAllowsLinkPreview:(BOOL)allowsLinkPreview
> +{
> +    [_wkView setAllowsLinkPreview:allowsLinkPreview];
> +}
> +

It’s confusing to have these methods implemented in two places in the file (although this is not unprecedented). It might be nicer to have one implementation in a cross-platform section of the file, with #if PLATFORM() inside the implementation.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:3810
> +    _data->_allowsLinkPreview = NO;

No need to initialize ivars to NO.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:4246
> +#if HAVE(LINK_PREVIEW)
> +    if (!_allowsLinkPreview)
> +        [self removeGestureRecognizer:_data->_immediateActionGestureRecognizer.get()];
> +    else if (NSGestureRecognizer *immediateActionRecognizer = _data->_immediateActionGestureRecognizer.get())
> +        [self addGestureRecognizer:immediateActionRecognizer];
> +#endif

Is HAVE(LINK_PREVIEW) the right check here in Mac code? Should you just check for >= 101000 like mostly everywhere else?

[Beth Dakin]

(In reply to comment #2)
> Comment on attachment 258090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258090&action=review
> 
> > Source/WebKit2/ChangeLog:20
> > +        (-[WKWebView _ignoresNonWheelEvents]):
> 
> I don’t think that one has changed.
> 
> > Source/WebKit2/ChangeLog:33
> > +        (-[WKView _setIgnoresNonWheelEvents:]):
> 
> Nor that one.
> 

Removed!

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:354
> > -    _allowsLinkPreview = YES;
> > +    _allowsLinkPreview = NO;
> 
> ivars are NO by default, so you can just remove this assignment.
> 

Removed!

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1858
> > +- (BOOL)allowsLinkPreview
> > +{
> > +    return [_wkView allowsLinkPreview];
> > +}
> > +
> > +- (void)setAllowsLinkPreview:(BOOL)allowsLinkPreview
> > +{
> > +    [_wkView setAllowsLinkPreview:allowsLinkPreview];
> > +}
> > +
> 
> It’s confusing to have these methods implemented in two places in the file
> (although this is not unprecedented). It might be nicer to have one
> implementation in a cross-platform section of the file, with #if PLATFORM()
> inside the implementation.
> 

It is confusing. I did as you suggested.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:3810
> > +    _data->_allowsLinkPreview = NO;
> 
> No need to initialize ivars to NO.
> 

Removed!

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:4246
> > +#if HAVE(LINK_PREVIEW)
> > +    if (!_allowsLinkPreview)
> > +        [self removeGestureRecognizer:_data->_immediateActionGestureRecognizer.get()];
> > +    else if (NSGestureRecognizer *immediateActionRecognizer = _data->_immediateActionGestureRecognizer.get())
> > +        [self addGestureRecognizer:immediateActionRecognizer];
> > +#endif
> 
> Is HAVE(LINK_PREVIEW) the right check here in Mac code? Should you just
> check for >= 101000 like mostly everywhere else?

Yes! I'm a little surprised HAVE(LINK_PREVIEW) worked! But I have changed it to >= 101000.

[Beth Dakin]

Forgot to update this with the trac: http://trac.webkit.org/changeset/187764