Bug 202214

Created attachment 379562 [details] Patch

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.

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

Comment on attachment 379562 [details] Patch Also, is there a test for this you can enable on iOS now?

Created attachment 379568 [details] Patch

(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?

(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).

Created attachment 379580 [details] Patch

Created attachment 379846 [details] Patch

(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.

Created attachment 379869 [details] Patch

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.

Created attachment 386845 [details] Patch

(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.

Created attachment 387716 [details] Patch

Please check with media folks

(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.

Comment on attachment 387716 [details] Patch Rejecting attachment 387716 [details] from commit-queue. luming_yin@apple.com 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.

The commit-queue encountered the following flaky tests while processing attachment 387716 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 387716 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 387716 [details] Patch Clearing flags on attachment: 387716 Committed r254553: <https://trac.webkit.org/changeset/254553>

All reviewed patches have been landed. Closing bug.