Bug 201371 – Provide a prototype for AR QuickLook to trigger processing in the originating page

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Provide a prototype for AR QuickLook to trigger processing in the originating page

[Dean Jackson]

<rdar://54904781>

[Dean Jackson]

Created attachment 377776 [details]
Patch

[Anders Carlsson]

(In reply to Dean Jackson from comment #2)
> Created attachment 377776 [details]
> Patch

Looks like iOS is broken.

[Dean Jackson]

Created attachment 377804 [details]
Patch

[Dean Jackson]

Created attachment 377806 [details]
Patch

[Sam Weinig]

Comment on attachment 377806 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        Provide a way for a Web page to know if a particular action in the AR scene
> +        was performed, if and only if the system AR library calls a delegate with
> +        particular parameters. Post a message to the window on the main frame
> +        to indicate the action was triggered.

Why the mainframe? Can a AR QuickLook be triggered only from a mainframe?

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:161
> +        NSString *javaScriptString = [NSString stringWithFormat:@"window.postMessage({_origin: '_apple_ar_quicklook' , action: 'buttonTapped'}, window.location.origin);"];
> +        _previewController->page().runJavaScriptInMainFrame(javaScriptString, true, [](API::SerializedScriptValue*, bool, const WebCore::ExceptionDetails&, WebKit::ScriptValueCallback::Error) { });
> +    }

Is there any chance the page has navigated since quicklook started? It would be good to either defend agains that or assert that it didn't happen.

[Wenson Hsieh]

Comment on attachment 377806 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:73
> +- (instancetype)initWithSystemPreviewController:(WebKit::SystemPreviewController*)previewController MIMEType:(NSString*)mimeType originatingPageURL:(URL)url

Nit - do we ever expect the previewController to be null? If not, perhaps this should take a reference.

[Simon Fraser (smfr)]

Comment on attachment 377806 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:158
> +    if ([[message[@"callToAction"] description] isEqualToString:@"buttonTapped"]) {

-description is generally used for debugging and might contain all kinds of junk (addresses, class names).

[Dean Jackson]

Created attachment 378606 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 378606 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:489
> +        systemPreviewInfo.globalFrameID.frameID = document().frame()->loader().client().frameID().valueOr(FrameIdentifier { });
> +        systemPreviewInfo.globalFrameID.pageID = document().frame()->loader().client().pageID().valueOr(PageIdentifier { });

Weird flex but OK. Not sure why the PageIdentifier and FrameIdentifiers aren't on Page and Frame. Chris?

> Source/WebCore/loader/FrameLoaderTypes.h:185
> +    IntRect previewRect;

What coordinate system is this rect in?

> Source/WebKit/UIProcess/SystemPreviewController.h:65
> +    void triggerSystemPreviewActionWithTarget(uint64_t frameID, uint64_t pageID);

Maybe put "ForTesting" in the name

[Dean Jackson]

Committed r249855: <https://trac.webkit.org/changeset/249855>

[Ryan Haddad]

(In reply to Dean Jackson from comment #11)
> Committed r249855: <https://trac.webkit.org/changeset/249855>
TestWebKitAPI.WebKit.SystemPreviewTriggered (added with this change) is timing out on the iOS Simulator bots:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/builds/6364

[Jonathan Bedard]

This breaks the iOS 13 OpenSource build...perhaps it needs <https://trac.webkit.org/changeset/249859/webkit>? Or an SPI declaration for ARQuickLookWebKitItemDelegate?

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 378969 [details]
Patch

[Jonathan Bedard]

Committed r249958: <https://trac.webkit.org/changeset/249958>

[Wenson Hsieh]

Comment on attachment 378606 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:508
> +- (void)_triggerSystemPreviewActionOnFrame:(uint64_t)frameID page:(uint64_t)pageID WK_API_AVAILABLE(ios(13.0));

Should this be WK_IOS_TBA?