Bug 147573 - Need WKWebView API to enable/disable link preview
Summary: Need WKWebView API to enable/disable link preview
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-03 11:24 PDT by Beth Dakin
Modified: 2015-08-13 11:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.65 KB, patch)
2015-08-03 11:31 PDT, Beth Dakin
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2015-08-03 11:24:19 PDT
Need WKWebView API to enable/disable link preview

rdar://problem/22077836
Comment 1 Beth Dakin 2015-08-03 11:31:18 PDT
Created attachment 258090 [details]
Patch
Comment 2 mitz 2015-08-03 13:53:12 PDT
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?
Comment 3 Beth Dakin 2015-08-03 14:08:00 PDT
(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.
Comment 4 Beth Dakin 2015-08-13 11:42:30 PDT
Forgot to update this with the trac: http://trac.webkit.org/changeset/187764