<rdar://problem/50063091>
Created attachment 368389 [details] Work in progress
Created attachment 368427 [details] Patch
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
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
Created attachment 368436 [details] Patch
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 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 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
Created attachment 368604 [details] Patch
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.
Created attachment 368697 [details] Patch for landing
Comment on attachment 368697 [details] Patch for landing Clearing flags on attachment: 368697 Committed r244849: <https://trac.webkit.org/changeset/244849>