Update API availability for autoplay event

• Component: WebKit2
• Status: RESOLVED FIXED

[Luming Yin]

<rdar://problem/55710395>

[Luming Yin]

Created attachment 379562 [details]
Patch

[Wenson Hsieh]

Comment on attachment 379562 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:213
> +- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags WK_API_AVAILABLE(macos(10.13.4), ios(WK_IOS_TBA));

It looks like this should be moved into a cross-platform part of the header as well.

[Tim Horton]

Comment on attachment 379562 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:213
> +- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags WK_API_AVAILABLE(macos(10.13.4), ios(WK_IOS_TBA));

I think this is in the wrong section? it's in an #else of a #if TARGET_OS_IPHONE

[Tim Horton]

Comment on attachment 379562 [details]
Patch

Also, is there a test for this you can enable on iOS now?

[Luming Yin]

Created attachment 379568 [details]
Patch

[Luming Yin]

(In reply to Wenson Hsieh from comment #2)
> Comment on attachment 379562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379562&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:213
> > +- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags WK_API_AVAILABLE(macos(10.13.4), ios(WK_IOS_TBA));
> 
> It looks like this should be moved into a cross-platform part of the header
> as well.

(In reply to Tim Horton from comment #3)
> Comment on attachment 379562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379562&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:213
> > +- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags WK_API_AVAILABLE(macos(10.13.4), ios(WK_IOS_TBA));
> 
> I think this is in the wrong section? it's in an #else of a #if
> TARGET_OS_IPHONE

Oops, it's indeed in the wrong section! Updated my patch to move it to cross-platform part of the header.

(In reply to Tim Horton from comment #4)
> Comment on attachment 379562 [details]
> Patch
> 
> Also, is there a test for this you can enable on iOS now?

It doesn't seem like there's any test(s) for this API. Are WebKit tests in a separate repository?

[Wenson Hsieh]

(In reply to Luming Yin from comment #6)
> (In reply to Tim Horton from comment #4)
> > Comment on attachment 379562 [details]
> > Patch
> > 
> > Also, is there a test for this you can enable on iOS now?
> 
> It doesn't seem like there's any test(s) for this API. Are WebKit tests in a
> separate repository?

In a separate project, Tools/TestWebKitAPI. You may want to take a look at WebsitePolicies.mm.

I think you might need some additional changes to get this working for iOS though (e.g., UIClient::handleAutoplayEvent is macOS-only right now).

[Luming Yin]

Created attachment 379580 [details]
Patch

[Luming Yin]

Created attachment 379846 [details]
Patch

[Luming Yin]

(In reply to Tim Horton from comment #4)
> Comment on attachment 379562 [details]
> Patch
> 
> Also, is there a test for this you can enable on iOS now?

Patch updated to enable relevant API tests on iOS.

[Luming Yin]

Created attachment 379869 [details]
Patch

[Wenson Hsieh]

Comment on attachment 379869 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:-109
> -    auto host = m_document->topDocument().url().host();
> -    return equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com");

Just to double check — would we not need this quirk on iOS anymore because it will be handled by Safari?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:848
> +void WebPage::simulateTap(const IntPoint& point, OptionSet<WebEvent::Modifier> modifiers)
> +{
> +    FloatPoint adjustedPoint;
> +    Node* nodeRespondingToClick = m_page->mainFrame().nodeRespondingToClickEvents(point, adjustedPoint);
> +    if (nodeRespondingToClick)
> +        handleSyntheticClick(*nodeRespondingToClick, adjustedPoint, modifiers);
> +}

Hm…I think we generally try to avoid deep testing hooks like this if we can, and mostly limit interaction-related testing hooks such as this to the UI process (commonly, WKContentViewInteraction and WKWebView).

That said, I recall that this was difficult in API tests due to how there are `lastLayerTreeTransactionId` checks throughout WebPageIOS that fail due to `lastLayerTreeTransactionId ` being 0. If that’s the case, could we additionally synthesize touch events by calling into -_webTouchEventsRecognized: (or add a simple hook to do so) before sending the click? I think that /might/ be slightly cleaner than adding a different testing codepath in the web process for simulating clicks.

[Luming Yin]

Created attachment 386845 [details]
Patch

[Luming Yin]

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 379869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379869&action=review
> 
> > Source/WebCore/page/Quirks.cpp:-109
> > -    auto host = m_document->topDocument().url().host();
> > -    return equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com");
> 
> Just to double check — would we not need this quirk on iOS anymore because
> it will be handled by Safari?
Oops, I just double checked, and we do in fact still need this quirk on iOS. I've Added the quirk back.

> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:848
> > +void WebPage::simulateTap(const IntPoint& point, OptionSet<WebEvent::Modifier> modifiers)
> > +{
> > +    FloatPoint adjustedPoint;
> > +    Node* nodeRespondingToClick = m_page->mainFrame().nodeRespondingToClickEvents(point, adjustedPoint);
> > +    if (nodeRespondingToClick)
> > +        handleSyntheticClick(*nodeRespondingToClick, adjustedPoint, modifiers);
> > +}
> 
> Hm…I think we generally try to avoid deep testing hooks like this if we can,
> and mostly limit interaction-related testing hooks such as this to the UI
> process (commonly, WKContentViewInteraction and WKWebView).
> 
> That said, I recall that this was difficult in API tests due to how there
> are `lastLayerTreeTransactionId` checks throughout WebPageIOS that fail due
> to `lastLayerTreeTransactionId ` being 0. If that’s the case, could we
> additionally synthesize touch events by calling into
> -_webTouchEventsRecognized: (or add a simple hook to do so) before sending
> the click? I think that /might/ be slightly cleaner than adding a different
> testing codepath in the web process for simulating clicks.

This is indeed the case! Generating synthetic taps in the UI process is difficult due to various `lastLayerTreeTransactionId` checks. I also tried to use _WKTouchEventGenerator to generate touches, but when running the headless WebKit API tests, [UIApplication sharedApplication] returns nil, so there’s no way to get the associated contextID, meaning this also isn't a valid option.

I've updated the patch to synthesize touch events by calling into -_webTouchEventsRecognized: before sending the click.

[Luming Yin]

Created attachment 387716 [details]
Patch

[Tim Horton]

Please check with media folks

[Jer Noble]

(In reply to Tim Horton from comment #16)
> Please check with media folks

No problem from the media folks. Fewer unnecessary platform-specific blocks of code is definitely better.

[EWS]

Comment on attachment 387716 [details]
Patch

Rejecting attachment 387716 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387716 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387716 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387716 [details]
Patch

Clearing flags on attachment: 387716

Committed r254553: <https://trac.webkit.org/changeset/254553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.