Bug 197342 – [iOS] Add a version of viewport shrink-to-fit heuristics that preserves page layout

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/50063091>

[Wenson Hsieh]

Created attachment 368389 [details]
Work in progress

[Wenson Hsieh]

Created attachment 368427 [details]
Patch

[EWS Watchlist]

Comment on attachment 368427 [details]
Patch

Attachment 368427 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12022133

New failing tests:
fast/viewport/ios/shrink-to-fit-content-responsive-viewport-with-horizontal-overflow.html

[EWS Watchlist]

Created attachment 368429 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[Wenson Hsieh]

Created attachment 368436 [details]
Patch

[Brent Fulgham]

Comment on attachment 368436 [details]
Patch

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

> Source/WebCore/page/ViewportConfiguration.cpp:591
> +    if (m_minimumEffectiveDeviceWidth == width)

Do we need to do floating point equality comparison here? I'm never sure when rounding errors are significant for this kind of test (areEssentiallyEqual)

> Source/WebCore/page/ViewportConfiguration.h:110
> +        if ((m_viewportArguments.zoom == 1. || m_viewportArguments.width == ViewportArguments::ValueDeviceWidth) && !m_isKnownToLayOutWiderThanViewport)

Again, we are testing against a specific floating point value. Should this be 'areEssentiallyEqual'?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3095
> +    // FIXME: Consider additionally logging an error message to the console if a responsive meta viewport tag was used.

Please include a Bugzilla or radar # so we actually do consider doing so! :-)

[Simon Fraser (smfr)]

Comment on attachment 368436 [details]
Patch

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

> Source/WebCore/page/ViewportConfiguration.cpp:609
> +    m_isKnownToLayOutWiderThanViewport = value;

I don't really like ViewportConfiguration being more stateful now.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5742
> +void WebPage::didFinishLoad(WebFrame& frame)

How is this different from didFinishDocumentLoad?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5759
> +#if PLATFORM(IOS_FAMILY)
> +    scheduleShrinkToFitContent();
> +#endif

Platform #ifdefs are icky. You should #define VIEWPORT_RESIZING or something.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1819
>      bool m_keyboardIsAttached { false };
> +    RunLoop::Timer<WebPage> m_shrinkToFitContentTimer;
>      bool m_inDynamicSizeUpdate { false };

You just introduced 7 bytes of padding :(

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3036
> +    m_shrinkToFitContentTimer.stop();
> +    m_shrinkToFitContentTimer.startOneShot(0_s);

I don't think you need a stop before a start.

Also, should this be a DeferrableOneShotTimer?

[Wenson Hsieh]

Comment on attachment 368436 [details]
Patch

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

Thanks, Brent and Simon for taking a look! I'll post an updated version soon.

>> Source/WebCore/page/ViewportConfiguration.cpp:591
>> +    if (m_minimumEffectiveDeviceWidth == width)
> 
> Do we need to do floating point equality comparison here? I'm never sure when rounding errors are significant for this kind of test (areEssentiallyEqual)

Sure, we could just bail if the new and old values areEssentiallyEqual.

>> Source/WebCore/page/ViewportConfiguration.cpp:609
>> +    m_isKnownToLayOutWiderThanViewport = value;
> 
> I don't really like ViewportConfiguration being more stateful now.

It's...not great. But AFAICT, everything ViewportConfiguration knows about is state that is pushed from WebPage (e.g. content size, preferences like user scalability/scaling constraints, layout size, etc.).

I think an alternative to this model is introducing a ViewportConfigurationClient that WebPage (or Page) would implement, so that ViewportConfiguration simply asks the page for any information it needs.

>> Source/WebCore/page/ViewportConfiguration.h:110
>> +        if ((m_viewportArguments.zoom == 1. || m_viewportArguments.width == ViewportArguments::ValueDeviceWidth) && !m_isKnownToLayOutWiderThanViewport)
> 
> Again, we are testing against a specific floating point value. Should this be 'areEssentiallyEqual'?

This is the same as it was before (see below), and I think in this case it's fine, since the zoom in the viewport argument comes straight from parsing viewport arguments.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5742
>> +void WebPage::didFinishLoad(WebFrame& frame)
> 
> How is this different from didFinishDocumentLoad?

didFinishLoad is page load. didFinishDocumentLoad is DOMContentLoaded.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5759
>> +#endif
> 
> Platform #ifdefs are icky. You should #define VIEWPORT_RESIZING or something.

Good call.

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1819
>>      bool m_inDynamicSizeUpdate { false };
> 
> You just introduced 7 bytes of padding :(

(OOPS!)

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3036
>> +    m_shrinkToFitContentTimer.startOneShot(0_s);
> 
> I don't think you need a stop before a start.
> 
> Also, should this be a DeferrableOneShotTimer?

Ah, right. Changed to DeferrableOneShotTimer and changed to use restart().

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3095
>> +    // FIXME: Consider additionally logging an error message to the console if a responsive meta viewport tag was used.
> 
> Please include a Bugzilla or radar # so we actually do consider doing so! :-)

👍🏻

https://bugs.webkit.org/show_bug.cgi?id=197429

[Wenson Hsieh]

Created attachment 368604 [details]
Patch

[Tim Horton]

Comment on attachment 368604 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:427
> +#if ENABLE_VIEWPORT_RESIZING

These should be ENABLE(), not ENABLE_

> Source/WebKit/WebProcess/WebPage/WebPage.h:1907
> +    WebCore::DeferrableOneShotTimer m_shrinkToFitContentTimer;

Doesn't even really need to be a timer, but whatever.

[Wenson Hsieh]

Created attachment 368697 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 368697 [details]
Patch for landing

Clearing flags on attachment: 368697

Committed r244849: <https://trac.webkit.org/changeset/244849>