Bug 188772 – Allow override of viewport configuration

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[James Savage]

<rdar://problem/43538408>

[Radar WebKit Bug Importer]

<rdar://problem/43538892>

[James Savage]

Created attachment 347590 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 347590 [details]
Patch

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

> Source/WebCore/page/ViewportConfiguration.h:105
> +    WEBCORE_EXPORT static Parameters deviceWebpageParameters();

This is a somewhat confusing name. Maybe put it above webpageParameters() and add a comment saying that it uses device width and scale 1.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1803
> +    if (m_page->settings().shouldIgnoreMetaViewport() || m_viewportConfiguration.setViewportArguments(viewportArguments))
>          viewportConfigurationChanged();

Why trigger viewportConfigurationChanged() every time if meta viewport is ignored?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5320
> +    if (!m_page->settings().shouldIgnoreMetaViewport() && m_viewportConfiguration.setViewportArguments(coreFrame->document()->viewportArguments()))
>          viewportChanged = true;

Ditto.

[James Savage]

Comment on attachment 347590 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1803
>>          viewportConfigurationChanged();
> 
> Why trigger viewportConfigurationChanged() every time if meta viewport is ignored?

I could be misremembering, but if I skipped this then web content would not re-layout when the web view size changed.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5320
>>          viewportChanged = true;
> 
> Ditto.

Same for this.

[EWS Watchlist]

Comment on attachment 347590 [details]
Patch

Attachment 347590 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8930504

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 347645 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Simon Fraser (smfr)]

Comment on attachment 347590 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:785
> +#if PLATFORM(IOS)

We don't like the #ifdefs. Maybe just leave this stuff on for all platforms; it will just be a no-op on macOS.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:146
> -#if !TARGET_OS_IPHONE
> +#if TARGET_OS_IPHONE
> +@property (nonatomic, setter=_setShouldIgnoreMetaViewport:) BOOL _shouldIgnoreMetaViewport WK_API_AVAILABLE(ios(WK_IOS_TBA));
> +#else

We don't like the #ifdefs. Maybe just leave this stuff on for all platforms; it will just be a no-op on macOS.

[James Savage]

Created attachment 348329 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 348329 [details]
Patch

Looks good but this needs to have some tests. It should be possible to test with TestWebKitAPI tests.

How does this bug relate to bug 167734? Is it a duplicate? It’s best to not end up with multiple similar but subtly different APIs for doing things in this area.

[James Savage]

Created attachment 349638 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349638 [details]
Patch

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

> LayoutTests/ChangeLog:15
> +        * fast/viewport/ios/ipad/baseline.html: Added.

baseline.html is a confusing name. Maybe just no-viewport.html.

> LayoutTests/ChangeLog:17
> +        * fast/viewport/ios/ipad/meta-viewport-disabled.html: Added.

I would not use  -disabled in a test name; it's too easily confused with the test being disabled. Call it meta-viewport-ignored.html.

> LayoutTests/fast/viewport/ios/ipad/baseline.html:5
> +    <meta name="viewport" content="">

Is this equivalent to no viewport tag at all?

[James Savage]

Comment on attachment 349638 [details]
Patch

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

>> LayoutTests/ChangeLog:15
>> +        * fast/viewport/ios/ipad/baseline.html: Added.
> 
> baseline.html is a confusing name. Maybe just no-viewport.html.

Will change.

>> LayoutTests/ChangeLog:17
>> +        * fast/viewport/ios/ipad/meta-viewport-disabled.html: Added.
> 
> I would not use  -disabled in a test name; it's too easily confused with the test being disabled. Call it meta-viewport-ignored.html.

Will change.

>> LayoutTests/fast/viewport/ios/ipad/baseline.html:5
>> +    <meta name="viewport" content="">
> 
> Is this equivalent to no viewport tag at all?

As best I can tell yes. I chose this route to avoid updating the test javascript, which makes the assumption that there will be a viewport tag and hits an exception if there is not. But if you’d rather I can patch the JS to deal with no meta tag rather than doing this.

[James Savage]

Created attachment 350799 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350799 [details]
Patch

Clearing flags on attachment: 350799

Committed r236530: <https://trac.webkit.org/changeset/236530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.