[webkit-reviews] review granted: [Bug 220065] REGRESSION (r261157): Crash in WKSelectPopover when running as iPhone app on iPad : [Attachment 416619] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 220065: REGRESSION (r261157): Crash in WKSelectPopover when running as
iPhone app on iPad
https://bugs.webkit.org/show_bug.cgi?id=220065

Attachment 416619: Patch

https://bugs.webkit.org/attachment.cgi?id=416619&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 416619
  --> https://bugs.webkit.org/attachment.cgi?id=416619
Patch

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

I’m doing a review+ here but I have some pretty serious concerns so bordering
on a review- maybe.

> Source/WebKit/UIProcess/ios/WKContentView.mm:218
> +    if (![UIApplication sharedApplication])
> +	   [[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_applicationDidFinishLaunching:)
name:UIApplicationDidFinishLaunchingNotification object:nil];

Does not seem right to register for this separately for each WKContentView.
WebKit only needs to be notified *once*, and we would then probably want to
tell all web *processes* about the observed change, not all web *pages*. Not
even sure that initializing WKContentView is the exact right bottleneck for
this. We need this registration if we ever stored the result of
currentUserInterfaceIdiomIsPadOrMac somewhere while [UIApplication
sharedApplication]) returns nil, which may *currently* always be tied to
allocating WKContentView objects, but that might be an unnecessarily fragile
thing to depend on longer term.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1651
> +    shouldRecommendDesktopClassBrowsing = WTF::nullopt;

This needs to be done once globally, not separately by each web page.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1656
> +    m_preferences->setAllowsInlineMediaPlayback(isPadOrMac);
> +   
m_preferences->setInlineMediaPlaybackRequiresPlaysInlineAttribute(!isPadOrMac);
> +    m_preferences->setAllowsInlineMediaPlaybackAfterFullscreen(!isPadOrMac);

Does this do the right thing for textAutosizingUsesIdempotentMode? I think
likely not.

Seems risky that this code here is far away from both the
WebPreferencesInternal.yaml and -[WKWebViewConfiguration init], since they need
to be kept in sync. At least there should be comments pointing from one to the
other, since two different bits of code are trying to do the same thing.

But also, the fact that this modifies preferences directly and not
WKWebViewConfiguration points to the fact that this is a different semantic
from the existing code. The existing code sets a *default* for a newly created
WKWebViewConfiguration object. But change will *override* the current setting.
So if a program sets the allowsInlineMediaPlayback flag on their
WKWebViewConfiguration explicitly, for example, this will ignore that setting
and override any change they have made. That’s not consistent with what we do
otherwise and not really great; it’s also not great that the configuration will
be "wrong" if they read the WKWebViewConfiguration, so that’s another small our
solution is imperfect.

Ideally we should think bout the desired behavior when someone customizes the
configuration. Or more generally what we’d want here, if we weren’t working
around our ability to function properly before UIApplication is initialized.

I think desired behavior would be to have the correct value even if WebKit code
is called super-early, rather than being wrong for a while and trying to fix
things after the fact, but I suppose that might be difficult. Worth talking
through with UIKit team?

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1659
> +    if (hasRunningProcess())
> +	  
m_process->send(Messages::WebPage::UserInterfaceIdiomDidChange(isPadOrMac),
m_webPageID);

Inelegant to do this for every web page: it needs to be done once for every web
*process*. Ideally could be a WebProcessProxy function instead of a
WebPageProxy function.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1663
> +}
> +
>  
>  } // namespace WebKit

Extra blank line here.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3977
> +    WebKit::setCurrentUserInterfaceIdiomIsPadOrMac(isPadOrMac);

Definitely "once per process" code, not "once per page".

> Tools/TestWebKitAPI/Tests/ios/UserInterfaceIdiomUpdate.mm:51
> +    [webView stringByEvaluatingJavaScript:@"select.focus()"];

This test doesn’t check any of these:

    setAllowsInlineMediaPlayback
    setInlineMediaPlaybackRequiresPlaysInlineAttribute
    setAllowsInlineMediaPlaybackAfterFullscreen

So it would pass even if the code from this patch was wrong. You can delete one
or more of those calls and no test would fail. That means we probably need more
thorough testing.