Bug 144836 - Use preview view controller from WebKitSystemInterface.
Summary: Use preview view controller from WebKitSystemInterface.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-09 17:55 PDT by Yongjun Zhang
Modified: 2015-05-10 18:42 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2015-05-09 17:55:03 PDT
See title, we should use view controller prepared in WebKitSystemInterface.
Comment 1 Yongjun Zhang 2015-05-09 17:55:54 PDT
<rdar://problem/20735668>
Comment 2 Yongjun Zhang 2015-05-09 18:16:44 PDT
Created attachment 252791 [details]
Patch.
Comment 3 mitz 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().
Comment 4 Yongjun Zhang 2015-05-10 12:12:26 PDT
Created attachment 252821 [details]
New patch.

Make allowsLinkPreview a private property.
Comment 5 mitz 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.
Comment 6 Yongjun Zhang 2015-05-10 17:46:32 PDT
Created attachment 252831 [details]
Address review comment for landing.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2015-05-10 18:42:43 PDT
All reviewed patches have been landed.  Closing bug.