Bug 144836

Summary:

Use preview view controller from WebKitSystemInterface.

Product:

WebKit

Reporter:

Yongjun Zhang <yongjun_zhang>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, commit-queue, mitz, mjs, sam

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch.	none
New patch.	mitz: review+
Address review comment for landing.	none

Yongjun Zhang

Reported 2015-05-09 17:55:03 PDT

See title, we should use view controller prepared in WebKitSystemInterface.

Attachments
Patch. (8.41 KB, patch) 2015-05-09 18:16 PDT, Yongjun Zhang	no flags	Details Formatted Diff Diff
New patch. (8.76 KB, patch) 2015-05-10 12:12 PDT, Yongjun Zhang	mitz: review+	Details Formatted Diff Diff
Address review comment for landing. (8.79 KB, patch) 2015-05-10 17:46 PDT, Yongjun Zhang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yongjun Zhang

Comment 1 2015-05-09 17:55:54 PDT

<rdar://problem/20735668>

Yongjun Zhang

Comment 2 2015-05-09 18:16:44 PDT

Created attachment 252791 [details] Patch.

mitz

Comment 3 2015-05-09 19:43:05 PDT

Comment on attachment 252791 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=252791&action=review This looks OK to me. I am surprised that the new property is being introduced as API right away rather than SPI. I’d like someone else from Apple to review this aspect. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:235 > +@property (nonatomic) BOOL allowsLinkPreview; This needs WK_AVAILABLE(NA, WK_IOS_TBA). > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:45 > +#if USE(APPLE_INTERNAL_SDK) > +#import <WebKitAdditions/LinkPreviewDefines.h> > +#endif These lines should move to a separate paragraph, after the non-conditional imports. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3116 > + _previewGestureRecognizer.clear(); We seem to prefer “= nil” to clear().

Yongjun Zhang

Comment 4 2015-05-10 12:12:26 PDT

Created attachment 252821 [details] New patch. Make allowsLinkPreview a private property.

mitz

Comment 5 2015-05-10 12:17:00 PDT

Comment on attachment 252821 [details] New patch. View in context: https://bugs.webkit.org/attachment.cgi?id=252821&action=review > Source/WebKit2/ChangeLog:23 > + (-[WKContentView previewViewControllerForPosition:inSourceView:]): : For client that doesn't provide : : > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:131 > +@property (nonatomic, getter=_allowsLinkPreview, setter=_setAllowsLinkPreview:) BOOL _allowsLinkPreview; No need for a custom getter here since it’s the same as the property name. Needs WK_AVAILABLE(NA, WK_IOS_TBA). > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:211 > + > +- (void)_registerPreviewInWindow:(UIWindow *)window; > +- (void)_unregisterPreviewInWindow:(UIWindow *)window; Weird to have a newline before but not after.

Yongjun Zhang

Comment 6 2015-05-10 17:46:32 PDT

Created attachment 252831 [details] Address review comment for landing.

WebKit Commit Bot

Comment 7 2015-05-10 18:42:38 PDT

Comment on attachment 252831 [details] Address review comment for landing. Clearing flags on attachment: 252831 Committed r184061: <http://trac.webkit.org/changeset/184061>

WebKit Commit Bot

Comment 8 2015-05-10 18:42:43 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.