Summary: | Need WKWebView API to enable/disable link preview | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||
Component: | WebKit2 | Assignee: | Beth Dakin <bdakin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, bdakin, enrica, mitz, sam, thorton | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Beth Dakin
2015-08-03 11:24:19 PDT
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? (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. Forgot to update this with the trac: http://trac.webkit.org/changeset/187764 |