Bug 197342

Summary: [iOS] Add a version of viewport shrink-to-fit heuristics that preserves page layout
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, bfulgham, commit-queue, ews-watchlist, simon.fraser, thorton, webkit-bug-importer, yongjun_zhang
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Work in progress
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
thorton: review+
Patch for landing none

Description Wenson Hsieh 2019-04-26 22:30:43 PDT
<rdar://problem/50063091>
Comment 1 Wenson Hsieh 2019-04-26 23:24:47 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-04-28 02:51:48 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2019-04-28 04:54:02 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-04-28 04:54:03 PDT Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2019-04-28 14:35:45 PDT
Created attachment 368436 [details]
Patch
Comment 6 Brent Fulgham 2019-04-29 09:41:03 PDT
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! :-)
Comment 7 Simon Fraser (smfr) 2019-04-30 11:48:30 PDT
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?
Comment 8 Wenson Hsieh 2019-04-30 13:06:40 PDT
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
Comment 9 Wenson Hsieh 2019-04-30 13:40:32 PDT
Created attachment 368604 [details]
Patch
Comment 10 Tim Horton 2019-05-01 11:51:03 PDT
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.
Comment 11 Wenson Hsieh 2019-05-01 13:30:10 PDT
Created attachment 368697 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2019-05-01 14:08:43 PDT
Comment on attachment 368697 [details]
Patch for landing

Clearing flags on attachment: 368697

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