Bug 237979 – [iOS] Add `WKWebView` API to control CSS "small viewport" `sv*` and "large viewport" `lv*` units

• Component: WebKit API
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/89434696>

[Devin Rousso]

Created attachment 454894 [details]
[fast-cq] Patch

[Tim Horton]

Comment on attachment 454894 [details]
[fast-cq] Patch

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

> Source/WebCore/page/FrameView.cpp:349
> +void FrameView::willReplaceMainFrameView(FrameView& oldMainFrameView)

This seems reminiscent of the propagation that usually happens in e.g. WebFrameLoaderClient::transitionToCommittedForNewPage; is there are reason you're doing it a different way than everything else?

[Devin Rousso]

Created attachment 455257 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 455305 [details]
[fast-cq] Patch

[Tim Horton]

Comment on attachment 455305 [details]
[fast-cq] Patch

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

> Source/WebCore/page/FrameView.h:245
> +    WEBCORE_EXPORT void copyCSSViewportUnits(FrameView&);

Not sure about "copy" (would expect a method named "copy" to return a copy of something, but it does not). Also "copy CSS Viewport units" isn't quite right either, it's copying the viewport sizes for the units. IDK, I feel like the name could use a re-think.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:626
> + @discussion For apps with size-changing UI around the @link WKWebView @/link, specify minimumViewportInset and

"size-changing UI" reads a little weird, but I assume you'll review this text elsewhere

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271
> -    _lastSentMinimumUnobscuredSize = newMinimumUnobscuredSize;
> -    _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize;

I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444
> +    maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize());

I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes?

[Wenson Hsieh]

Comment on attachment 455305 [details]
[fast-cq] Patch

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

> Source/WebCore/page/FrameView.cpp:5636
> +    if (m_defaultViewportSizeOverride && *m_defaultViewportSizeOverride == size)

Nit - I think just `m_defaultViewportSizeOverride == size` should work here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1869
> +#if PLATFORM(IOS_FAMILY)
> +

Should this go in `WKWebViewIOS.mm`?

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444
>> +    maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize());
> 
> I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes?

I _believe_ the rationale for using `initialScaleIgnoringContentSize()` here is that it's what (regular) viewport units use, in order to avoid an ever-changing initial scale when there's content that's sized using viewport units.

[Tim Horton]

Comment on attachment 455305 [details]
[fast-cq] Patch

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

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271
>> -    _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize;
> 
> I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful.

Specifically and more concretely, my worry is about https://trac.webkit.org/changeset/221153/webkit and whether you have lost the fix for it...

[Tim Nguyen (:ntim)]

Comment on attachment 455305 [details]
[fast-cq] Patch

style bot seems unhappy

[Devin Rousso]

Comment on attachment 455305 [details]
[fast-cq] Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:626
>> + @discussion For apps with size-changing UI around the @link WKWebView @/link, specify minimumViewportInset and
> 
> "size-changing UI" reads a little weird, but I assume you'll review this text elsewhere

I did have this reviewed, and nobody else mentioned this 😅  Do you have a suggestion for an alternative?

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1869
>> +
> 
> Should this go in `WKWebViewIOS.mm`?

I wanted to leave this here because I have a followup bug to add this API to macOS <https://webkit.org/b/238173> and figured it'd make things a bit easier/cleaner.

>>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271
>>> -    _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize;
>> 
>> I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful.
> 
> Specifically and more concretely, my worry is about https://trac.webkit.org/changeset/221153/webkit and whether you have lost the fix for it...

I just moved these checks to `WebPageProxy::setDefaultUnobscuredSize`/`WebPageProxy::setMinimumUnobscuredSize`/`WebPageProxy::setMaximumUnobscuredSize` instead of having them be iOS only.  This is because it's also used by the followup <https://webkit.org/b/238173>.  I will test again just in case :)

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444
>>> +    maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize());
>> 
>> I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes?
> 
> I _believe_ the rationale for using `initialScaleIgnoringContentSize()` here is that it's what (regular) viewport units use, in order to avoid an ever-changing initial scale when there's content that's sized using viewport units.

FWIW this is existing code that was moved from `Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm` to support the followup <https://webkit.org/b/238173>.

[Devin Rousso]

Created attachment 455926 [details]
[fast-cq] Patch

[EWS]

Committed r291980 (248939@main): <https://commits.webkit.org/248939@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455926 [details].

[Rob Buis]

Comment on attachment 455926 [details]
[fast-cq] Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527
> +    auto oldView = m_frame->coreFrame()->view();

This probably needs to be a RefPtr, right?

[Devin Rousso]

Comment on attachment 455926 [details]
[fast-cq] Patch

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

>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527
>> +    auto oldView = m_frame->coreFrame()->view();
> 
> This probably needs to be a RefPtr, right?

yyyuuuppp oops

i'll fix this asap

[Devin Rousso]

Created attachment 456037 [details]
[fast-cq] [Patch] fix UAF

[Devin Rousso]

Comment on attachment 455926 [details]
[fast-cq] Patch

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

>>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527
>>> +    auto oldView = m_frame->coreFrame()->view();
>> 
>> This probably needs to be a RefPtr, right?
> 
> yyyuuuppp oops
> 
> i'll fix this asap

r292042