Bug 155383 - Make preview inline navigation work API
Summary: Make preview inline navigation work API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-11 14:56 PST by Beth Dakin
Modified: 2016-03-17 06:14 PDT (History)
8 users (show)

See Also:


Attachments
Patch (72.90 KB, patch)
2016-03-11 15:05 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (92.39 KB, patch)
2016-03-11 16:44 PST, Beth Dakin
mitz: review-
Details | Formatted Diff | Diff
Patch that addresses Dan's comments (93.39 KB, patch)
2016-03-12 14:14 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
New patch that addresses more of Dan's comments (94.17 KB, patch)
2016-03-12 14:25 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with WK_API_ENABLED back (93.62 KB, patch)
2016-03-12 14:36 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (93.62 KB, patch)
2016-03-12 14:48 PST, 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 2016-03-11 14:56:34 PST
Make preview inline navigation work API
Comment 1 Beth Dakin 2016-03-11 15:05:11 PST
Created attachment 273773 [details]
Patch
Comment 2 Beth Dakin 2016-03-11 16:44:41 PST
Created attachment 273784 [details]
Patch
Comment 3 WebKit Commit Bot 2016-03-11 16:46:32 PST
Attachment 273784 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewElementInfo.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 mitz 2016-03-11 21:56:00 PST
Comment on attachment 273784 [details]
Patch

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

r- because this breaks the Mac build. Some more comments below.

> Source/WebKit2/ChangeLog:16
> +        _WKPreviewActionItem is now WKPreviewActionItem, but the _WKPreviewAction 
> +        class is still a private implementation detail, so it still has an underscore 
> +        even though the filenames do not.

If it’s an *internal* implementation detail, i.e, the class isn’t available even to SPI clients and isn’t exposed in a framework header (which I think is the case or at least should be the case), then per our convention it should *not* be underscore-prefixed.

> Source/WebKit2/ChangeLog:19
> +        nowWKPreviewActionItemIdentifiers.h/mm and all the the identifiers have been 

nowWK

> Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:39
> +#endif // WK_API_ENABLED

I think we normally don’t // WK_API_ENABLED.

> Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItemInternal.h:32
> +@interface _WKPreviewAction : UIPreviewAction <NSCopying, WKPreviewActionItem>

So the underscore should go.

Not new, but you’re claiming NSCopying conformance here but I don’t see that class overrides -copyWithZone:, so at the very least it fails to copy the identifier (and that’s assuming UIPreviewAction is copyable, in which case, there’s no need to declare that its subclass is NSCopying-conformant).

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:150
> +- (BOOL)webView:(WKWebView *)webView shouldPreviewElement:(WKPreviewElementInfo *)elementInfo WK_AVAILABLE(NA, WK_IOS_TBA);
> +
> +/*! @abstract Allows your app to provide a custom view controller to show when the given element is peeked.
> + @param webView The web view invoking the delegate method.
> + @param elementInfo The elementInfo for the element the user is peeking.
> + @param defaultActions An array of the actions that WebKit would use as previewActionItems for this element by 
> + default. These actions would be used if allowsLinkPreview is YES but these delegate methods have not been 
> + implemented, or if this delegate method returns nil.
> + @discussion Returning a view controller will result in that view controller being displayed as a peek preview.
> + To use the defaultActions, your app is responsible for returning whichever of those actions it wants in your 
> + view controller's implementation of -previewActionItems.
> + 
> + Returning nil will result in WebKit's default preview behavior. webView:commitPreviewingViewController: will only be invoked
> + if a non-nil view controller was returned.
> + */
> +- (nullable UIViewController *)webView:(WKWebView *)webView previewingViewControllerForElement:(WKPreviewElementInfo *)elementInfo defaultActions:(NSArray <id <WKPreviewActionItem>> *)previewActions WK_AVAILABLE(NA, WK_IOS_TBA);
> +
> +/*! @abstract Allows your app to pop to the view controller it created.
> + @param webView The web view invoking the delegate method.
> + @param previewingViewController The view controller that is being popped.
> + */
> +- (void)webView:(WKWebView *)webView commitPreviewingViewController:(UIViewController *)previewingViewController WK_AVAILABLE(NA, WK_IOS_TBA);

These should all be in an #if TARGET_OS_IPHONE block.
Comment 5 mitz 2016-03-12 12:05:11 PST
I filed bug 155395 for the NSCopying issue.
Comment 6 Beth Dakin 2016-03-12 14:11:41 PST
Thanks Dan!!

(In reply to comment #4)
> Comment on attachment 273784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273784&action=review
> 
> r- because this breaks the Mac build. Some more comments below.
> 
> > Source/WebKit2/ChangeLog:16
> > +        _WKPreviewActionItem is now WKPreviewActionItem, but the _WKPreviewAction 
> > +        class is still a private implementation detail, so it still has an underscore 
> > +        even though the filenames do not.
> 
> If it’s an *internal* implementation detail, i.e, the class isn’t available
> even to SPI clients and isn’t exposed in a framework header (which I think
> is the case or at least should be the case), then per our convention it
> should *not* be underscore-prefixed.
> 

Oops! I fixed this.

> > Source/WebKit2/ChangeLog:19
> > +        nowWKPreviewActionItemIdentifiers.h/mm and all the the identifiers have been 
> 
> nowWK
> 

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:39
> > +#endif // WK_API_ENABLED
> 
> I think we normally don’t // WK_API_ENABLED.
> 

I found 173 occurrences of '// WK_API_ENABLED' when I searched for it in the WebKit2 xcoeproj, so I left it in for now. :-O

> > Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItemInternal.h:32
> > +@interface _WKPreviewAction : UIPreviewAction <NSCopying, WKPreviewActionItem>
> 
> So the underscore should go.
> 

Gone!

> Not new, but you’re claiming NSCopying conformance here but I don’t see that
> class overrides -copyWithZone:, so at the very least it fails to copy the
> identifier (and that’s assuming UIPreviewAction is copyable, in which case,
> there’s no need to declare that its subclass is NSCopying-conformant).
> 

I haven't fixed this yet since there is a new bug filed about it. Also since it will be easier to review that bit in the context of a smaller patch.

> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:150
> > +- (BOOL)webView:(WKWebView *)webView shouldPreviewElement:(WKPreviewElementInfo *)elementInfo WK_AVAILABLE(NA, WK_IOS_TBA);
> > +
> > +/*! @abstract Allows your app to provide a custom view controller to show when the given element is peeked.
> > + @param webView The web view invoking the delegate method.
> > + @param elementInfo The elementInfo for the element the user is peeking.
> > + @param defaultActions An array of the actions that WebKit would use as previewActionItems for this element by 
> > + default. These actions would be used if allowsLinkPreview is YES but these delegate methods have not been 
> > + implemented, or if this delegate method returns nil.
> > + @discussion Returning a view controller will result in that view controller being displayed as a peek preview.
> > + To use the defaultActions, your app is responsible for returning whichever of those actions it wants in your 
> > + view controller's implementation of -previewActionItems.
> > + 
> > + Returning nil will result in WebKit's default preview behavior. webView:commitPreviewingViewController: will only be invoked
> > + if a non-nil view controller was returned.
> > + */
> > +- (nullable UIViewController *)webView:(WKWebView *)webView previewingViewControllerForElement:(WKPreviewElementInfo *)elementInfo defaultActions:(NSArray <id <WKPreviewActionItem>> *)previewActions WK_AVAILABLE(NA, WK_IOS_TBA);
> > +
> > +/*! @abstract Allows your app to pop to the view controller it created.
> > + @param webView The web view invoking the delegate method.
> > + @param previewingViewController The view controller that is being popped.
> > + */
> > +- (void)webView:(WKWebView *)webView commitPreviewingViewController:(UIViewController *)previewingViewController WK_AVAILABLE(NA, WK_IOS_TBA);
> 
> These should all be in an #if TARGET_OS_IPHONE block.

Fixed!
Comment 7 Beth Dakin 2016-03-12 14:14:55 PST
Created attachment 273849 [details]
Patch that addresses Dan's comments
Comment 8 mitz 2016-03-12 14:15:17 PST
(In reply to comment #6)

> > > Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:39
> > > +#endif // WK_API_ENABLED
> > 
> > I think we normally don’t // WK_API_ENABLED.
> > 
> 
> I found 173 occurrences of '// WK_API_ENABLED' when I searched for it in the
> WebKit2 xcoeproj, so I left it in for now. :-O

I should have been more specific. I think we normally avoid // WK_API_ENABLED in our API and SPI headers, or at least in our API headers :-P
Comment 9 WebKit Commit Bot 2016-03-12 14:16:47 PST
Attachment 273849 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3951:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewElementInfo.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Beth Dakin 2016-03-12 14:21:34 PST
(In reply to comment #8)
> (In reply to comment #6)
> 
> > > > Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:39
> > > > +#endif // WK_API_ENABLED
> > > 
> > > I think we normally don’t // WK_API_ENABLED.
> > > 
> > 
> > I found 173 occurrences of '// WK_API_ENABLED' when I searched for it in the
> > WebKit2 xcoeproj, so I left it in for now. :-O
> 
> I should have been more specific. I think we normally avoid //
> WK_API_ENABLED in our API and SPI headers, or at least in our API headers :-P

OOoooOooOooOOoO. I have a bunch of them in the API headers. Will remove.
Comment 11 Beth Dakin 2016-03-12 14:23:39 PST
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #6)
> > 
> > > > > Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:39
> > > > > +#endif // WK_API_ENABLED
> > > > 
> > > > I think we normally don’t // WK_API_ENABLED.
> > > > 
> > > 
> > > I found 173 occurrences of '// WK_API_ENABLED' when I searched for it in the
> > > WebKit2 xcoeproj, so I left it in for now. :-O
> > 
> > I should have been more specific. I think we normally avoid //
> > WK_API_ENABLED in our API and SPI headers, or at least in our API headers :-P
> 
> OOoooOooOooOOoO. I have a bunch of them in the API headers. Will remove.

Although we do seem to have one in WKUIDelegate.h, but I will still remove the ones I added.
Comment 12 Beth Dakin 2016-03-12 14:25:22 PST
Created attachment 273850 [details]
New patch that addresses more of Dan's comments
Comment 13 WebKit Commit Bot 2016-03-12 14:27:54 PST
Attachment 273850 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3951:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewElementInfo.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItemIdentifiers.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:30:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Beth Dakin 2016-03-12 14:36:07 PST
Created attachment 273852 [details]
Patch with WK_API_ENABLED back
Comment 15 WebKit Commit Bot 2016-03-12 14:38:59 PST
Attachment 273852 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3951:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewElementInfo.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Beth Dakin 2016-03-12 14:48:40 PST
Created attachment 273854 [details]
Patch
Comment 17 WebKit Commit Bot 2016-03-12 14:50:33 PST
Attachment 273854 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3951:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreviewElementInfo.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKElementInfoInternal.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Beth Dakin 2016-03-12 15:16:28 PST
Thanks Dan! http://trac.webkit.org/changeset/198070
Comment 19 Csaba Osztrogonác 2016-03-17 06:13:03 PDT
(In reply to comment #18)
> Thanks Dan! http://trac.webkit.org/changeset/198070

It broke the Apple Mac cmake build:
CMake Error at Source/cmake/WebKitMacros.cmake:248 (add_library):
  Cannot find source file:

    UIProcess/API/Cocoa/_WKElementInfo.mm
Comment 20 Csaba Osztrogonác 2016-03-17 06:14:00 PDT
fix landed in http://trac.webkit.org/changeset/198329