Bug 155056 – AX: Force allow user zoom

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

We should plumb through the accessibility setting to WebKit in order to make user-scalable enable/disable on the fly.

[Radar WebKit Bug Importer]

<rdar://problem/24988193>

[Nan Wang]

Created attachment 273065 [details]
patch

[Nan Wang]

Created attachment 273066 [details]
patch

[chris fleizach]

Comment on attachment 273066 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        No new tests.

we should probably add a test that override force scale (which can be set with Settings in the layout test) also overrides max scale

> Source/WebCore/page/ViewportConfiguration.h:91
> +    double maximumScale() const { return m_forceAlwaysUserScalable ? 10.0 : m_configuration.maximumScale; }

we should probably define this is a const double somewhere

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:214
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000

#if PLATFORM(IOS) &&

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:215
> +#include <WebCore/SoftLinking.h>

i think you need another #if
#if __has_include(<AccessibilitySupport.h>)
#else
extern "C" {
Boolean _AXSForceAllowWebScaling();
CFStringRef kAXSAllowForceWebScalingEnabledNotification;
}
#endif

so that this can be built by other people

take a look at WeakObjCPtr.h

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:602
> +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, 0, 0);

nullptr, nullptr

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2820
> +    m_viewportConfiguration.setForceAlwaysUserScalable(_AXSForceAllowWebScaling());

it seems like this should also respect
store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey());

maybe this method should have all the logic and then updatePreferences below calls updateForceAlways..()

[Nan Wang]

Comment on attachment 273066 [details]
patch

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

Will address other comments.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:214
>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> 
> #if PLATFORM(IOS) &&

the above includes are already in #if PLATFORM(IOS)

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2820
>> +    m_viewportConfiguration.setForceAlwaysUserScalable(_AXSForceAllowWebScaling());
> 
> it seems like this should also respect
> store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey());
> 
> maybe this method should have all the logic and then updatePreferences below calls updateForceAlways..()

In this case we should cache store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey()) somewhere? When we get a notification we don't have access to the store object.

[Nan Wang]

Created attachment 273127 [details]
patch

For the layout test setting, do you mean InternalSettings? I'm not sure if we should expose the ViewportConfiguration so that we can override the force scale? And we should make this a WebKit2 only test I guess.

[WebKit Commit Bot]

Attachment 273127 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223:  _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 273127 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +

is it possible to make a test for this

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:224
> +CFStringRef kAXSAllowForceWebScalingEnabledNotification;

did you check can this be built for simulator when <AccessibilitySupport.h> is not available (essentially using the public SDK)

[Nan Wang]

Created attachment 273215 [details]
patch

Added a layout test.
Also checked that without AccessibilitySupport.h we can still build for simulator.

[WebKit Commit Bot]

Attachment 273215 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223:  _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 273215 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        Overridden the maximum scale factor when forceAlwaysUserScalable is true.

Overridden -> Overide

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:609
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000

seems like we don't need the PLATFORM(IOS) part

[Nan Wang]

Created attachment 273216 [details]
patch

addressed review comments

[WebKit Commit Bot]

Attachment 273216 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223:  _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nan Wang]

Build failures on gtk, working on it.

[Nan Wang]

Created attachment 273234 [details]
build fix patch

Build fix. Make ViewportConfiguration test only on iOS.

[WebKit Commit Bot]

Attachment 273234 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223:  _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 273234 [details]
build fix patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:215
> +#if __has_include(<AccessibilitySupport.h>) 

we might not need to do this difference because we can't use anything in the import directly. everything is a symbol that needs to be soft linked in

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:222
> +extern "C" {

does this compile and run? it seems like we still probably need to soft link libAX in this case

[Nan Wang]

Created attachment 273237 [details]
patch

review comments

[Nan Wang]

Created attachment 273244 [details]
patch

build fix

[chris fleizach]

Comment on attachment 273244 [details]
patch

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

> Source/WebCore/testing/Internals.cpp:3500
> +#if PLATFORM(IOS)

use

 UNUSED_PARAM(forceAlwaysUserScalableEnabled);

[Nan Wang]

Created attachment 273246 [details]
patch

review comments

[Nan Wang]

Created attachment 273249 [details]
patch

more build fix.
Guess we still need "if PLATFORM(IOS)" in ~WebPage()

[chris fleizach]

Comment on attachment 273249 [details]
patch

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

> Source/WebCore/testing/Internals.cpp:3501
> +    m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled);

Is there a reason we need to make this stuff iOS only?

[Nan Wang]

Comment on attachment 273249 [details]
patch

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

>> Source/WebCore/testing/Internals.cpp:3501
>> +    m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled);
> 
> Is there a reason we need to make this stuff iOS only?

Do we have user scalable feature on Mac? If yes I can add support for Mac too.

[Nan Wang]

Created attachment 273273 [details]
Support the Internals setting on Mac

[chris fleizach]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

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

> Source/WebCore/testing/Internals.cpp:3500
> +#if PLATFORM(IOS) || PLATFORM(MAC)

why not support this for all platforms?

[Nan Wang]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

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

>> Source/WebCore/testing/Internals.cpp:3500
>> +#if PLATFORM(IOS) || PLATFORM(MAC)
> 
> why not support this for all platforms?

Seems ViewportConfigutation is not supported on other platform. Got build failures in previous patches without this check.

[WebKit Commit Bot]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

Clearing flags on attachment: 273273

Committed r197766: <http://trac.webkit.org/changeset/197766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Why did this not get review from anyone who understands viewport scaling?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155183

[Simon Fraser (smfr)]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

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

> Source/WebCore/testing/Internals.h:481
> +    void setViewportForceAlwaysUserScalable(bool);
> +    double viewportConfigurationMaximumScale();

I don't think you need this stuff here if you use UIScriptController tests.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:220
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +#include <WebCore/SoftLinking.h>
> +SOFT_LINK_LIBRARY(libAccessibility)
> +SOFT_LINK(libAccessibility, _AXSForceAllowWebScaling, Boolean, (), ())
> +SOFT_LINK_CONSTANT(libAccessibility, kAXSAllowForceWebScalingEnabledNotification, CFStringRef)
> +#define kAXSAllowForceWebScalingEnabledNotification getkAXSAllowForceWebScalingEnabledNotification()
> +#endif

This is not a platform-specific file, and should not have this here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:279
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +static void forceAlwaysUserScalableChangedCallback(CFNotificationCenterRef, void* observer, CFStringRef, const void*, CFDictionaryRef)
> +{
> +    ASSERT(observer);
> +    static_cast<WebPage*>(observer)->updateForceAlwaysUserScalable();
> +}
> +#endif

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:569
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +    CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), this, forceAlwaysUserScalableChangedCallback, kAXSAllowForceWebScalingEnabledNotification, 0, CFNotificationSuspensionBehaviorDeliverImmediately);
> +#endif

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:604
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, nullptr, nullptr);
> +#endif

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2835
> +#if PLATFORM(IOS)
> +void WebPage::updateForceAlwaysUserScalable()
> +{
> +    bool forceAlwaysUserScalableEnabled = m_forceAlwaysUserScalable;
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +    forceAlwaysUserScalableEnabled |= _AXSForceAllowWebScaling();
> +#endif
> +    m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled);
> +}
> +#endif

Move to WebPageIOS.mm

> LayoutTests/ChangeLog:9
> +        * accessibility/ios-simulator/force-user-scalable-expected.txt: Added.
> +        * accessibility/ios-simulator/force-user-scalable.html: Added.

I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/

[Simon Fraser (smfr)]

Sorry, but I feel this patch did not have sufficient review or exposure from WebKit2 owners, and had significant problems, so I rolled it out.

[Nan Wang]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

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

>> LayoutTests/ChangeLog:9
>> +        * accessibility/ios-simulator/force-user-scalable.html: Added.
> 
> I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/

I've taken some look into this. Seems we have to expose a function to set the ViewportConfiguration in WebPage class so that UIScriptController can call that. However the TestController only has access to a WKPageRef. I guess I have to expose another function in WKPage to cast call the WebPage function? Correct me if I'm wrong. This seems to make things even more complicated.

[Simon Fraser (smfr)]

(In reply to comment #34)
> Comment on attachment 273273 [details]
> Support the Internals setting on Mac
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273273&action=review
> 
> >> LayoutTests/ChangeLog:9
> >> +        * accessibility/ios-simulator/force-user-scalable.html: Added.
> > 
> > I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/
> 
> I've taken some look into this. Seems we have to expose a function to set
> the ViewportConfiguration in WebPage class so that UIScriptController can
> call that. However the TestController only has access to a WKPageRef. I
> guess I have to expose another function in WKPage to cast call the WebPage
> function? Correct me if I'm wrong. This seems to make things even more
> complicated.

If forceAlwaysUserScalable were a setting, you could set it via internals in a test.

Then you'd have to synthesize a pinch-out to simulate zooming.

[Nan Wang]

Comment on attachment 273273 [details]
Support the Internals setting on Mac

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

>>>> LayoutTests/ChangeLog:9
>>>> +        * accessibility/ios-simulator/force-user-scalable.html: Added.
>>> 
>>> I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/
>> 
>> I've taken some look into this. Seems we have to expose a function to set the ViewportConfiguration in WebPage class so that UIScriptController can call that. However the TestController only has access to a WKPageRef. I guess I have to expose another function in WKPage to cast call the WebPage function? Correct me if I'm wrong. This seems to make things even more complicated.
> 
> If forceAlwaysUserScalable were a setting, you could set it via internals in a test.
> 
> Then you'd have to synthesize a pinch-out to simulate zooming.

It's only a setting in ViewportConfiguration. So should I expose ViewportConfiguration in Internals and add that setting?

[Simon Fraser (smfr)]

(In reply to comment #36)
> It's only a setting in ViewportConfiguration. So should I expose
> ViewportConfiguration in Internals and add that setting?

The issue here is that Internals is in WebCore, but the ViewportConfiguration lives in WebKit2. I don't think adding a ViewportConfiguration to Internals and just looking at maximumScale is a good way to test, though; that's just testing a small bit of code in ViewportConfiguration, but you really need to test that the user experience works.

Can you programmatically change the answer given by _AXSForceAllowWebScaling() on a per-process basis?

[Nan Wang]

Created attachment 273454 [details]
patch

- Moved the SOFT_LINK code to WebPageIOS.mm
- Added the test to make sure zooming is working correctly.

[Nan Wang]

Created attachment 273458 [details]
patch

Fixed conflicts.

[Build Bot]

Comment on attachment 273458 [details]
patch

Attachment 273458 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/949774

New failing tests:
accessibility/ios-simulator/force-always-user-scalable.html

[Build Bot]

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

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

[Simon Fraser (smfr)]

Comment on attachment 273458 [details]
patch

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

> Source/WebCore/page/ViewportConfiguration.h:93
> -    double maximumScale() const { return m_configuration.maximumScale; }
> +    double maximumScale() const { return m_forceAlwaysUserScalable ? forceAlwaysUserScalableMaximumScale : m_configuration.maximumScale; }

This solution doesn't prevent a page from saying minScale=10, maxScale=10 to avoid scalability.

Also, the various default configurations have maxScale=5. Why did you choose 10?

> Source/WebCore/testing/Internals.cpp:213
> +SOFT_LINK_LIBRARY(libAccessibility)
> +SOFT_LINK(libAccessibility, _AXSSetForceAllowWebScaling, void, (Boolean scalable), (scalable))

Can we just hard link?

> Source/WebCore/testing/Internals.cpp:3534
> +void Internals::setViewportForceAlwaysUserScalable(bool scalable)
> +{
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +    _AXSSetForceAllowWebScaling(scalable);
> +#else
> +    UNUSED_PARAM(scalable);
> +#endif
> +}

You need to reset this at the end of every test that changes it. There's a mechanism to do that.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:131
> +    forceAlwaysUserScalable |= _AXSForceAllowWebScaling();

It's a shame that we have a WebPreference key or this, but you're bypassing that. Should we remove the key, or should the default value of that key be overridden by _AXSForceAllowWebScaling()?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:140
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> +    CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), this, forceAlwaysUserScalableChangedCallback, kAXSAllowForceWebScalingEnabledNotification, 0, CFNotificationSuspensionBehaviorDeliverImmediately);
> +#endif

This soft link of kAXSAllowForceWebScalingEnabledNotification is going to cause a performance hit for everyone on iOS. Can we just hard link?

Does the CFNotificationCenterAddObserver have to happen here in WebPage? Lots of other notifications happen in WKWebView. Does it have to happen right at platformInitialize time?

[Nan Wang]

Comment on attachment 273458 [details]
patch

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

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:131
>> +    forceAlwaysUserScalable |= _AXSForceAllowWebScaling();
> 
> It's a shame that we have a WebPreference key or this, but you're bypassing that. Should we remove the key, or should the default value of that key be overridden by _AXSForceAllowWebScaling()?

The current implementation will also respect the WebPreference key result. I think once all the accessibility setting code were in we can decide to remove it or not.

[Nan Wang]

Created attachment 273622 [details]
patch

Review comments.
- Hard linked the library
- Moved notification code to WKWebView

[Simon Fraser (smfr)]

Comment on attachment 273622 [details]
patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:32265
> +				"OTHER_LDFLAGS[sdk=iphoneos*]" = (
> +					"$(ASAN_OTHER_LDFLAGS)",
> +					"-lAccessibility",
> +				);
> +				"OTHER_LDFLAGS[sdk=iphonesimulator*]" = (
> +					"$(ASAN_OTHER_LDFLAGS)",
> +					"-lAccessibility",

This needs to be done via the .xcconfig files.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:32277
> +					"$(ASAN_OTHER_LDFLAGS)",
> +					"-lAccessibility",
> +				);

Ditto.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:9604
> +				"OTHER_LDFLAGS[sdk=iphoneos*]" = (
> +					"$(OTHER_LDFLAGS)",
> +					"-l$(WEBKIT_SYSTEM_INTERFACE_LIBRARY)",
> +					"-lAccessibility",
> +				);
> +				"OTHER_LDFLAGS[sdk=iphonesimulator*]" = (
> +					"$(OTHER_LDFLAGS)",
> +					"-l$(WEBKIT_SYSTEM_INTERFACE_LIBRARY)",
> +					"-lAccessibility",

Also needs to move to xcconfig file.

[Nan Wang]

Created attachment 273641 [details]
build fix patch

Moved linker flags to xcconfig files.

[WebKit Commit Bot]

Attachment 273641 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:215:  _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 273641 [details]
build fix patch

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

> Source/WebCore/testing/Internals.cpp:212
> +#include "AccessibilitySupport.h"

This should be

<AccessibilitySupport.h>

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:106
> +#include "AccessibilitySupport.h"

Ditto

[Nan Wang]

Created attachment 273649 [details]
patch

review comments

[WebKit Commit Bot]

Attachment 273649 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:215:  _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 273649 [details]
patch

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

> LayoutTests/fast/viewport/ios/force-always-user-scalable.html:24
> +    window.internals.setViewportForceAlwaysUserScalable(true);

We should also try setting this back to NO and then check th scale is back to the author specified value

[Nan Wang]

Created attachment 273657 [details]
patch

Updated test

[WebKit Commit Bot]

Attachment 273657 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:215:  _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 273657 [details]
patch

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

> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:53
> +OTHER_LDFLAGS[sdk=iphoneos*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility;
> +OTHER_LDFLAGS[sdk=iphonesimulator*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility;

Is it OK to link with Accessibility all the way back to Monarch? We have open source builders on Monarch.

> Source/WebCore/page/ViewportConfiguration.cpp:194
> +    if (m_forceAlwaysUserScalable)
> +        minimumScale = std::min(minimumScale, forceAlwaysUserScalableMinimumScale);

This worries me a bit because there are pages which load by default with scale > 1 (e.g. "width=200px" in the viewport meta tag), and you're going to cause them to shrink down.

[chris fleizach]

Comment on attachment 273657 [details]
patch

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

>> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:53
>> +OTHER_LDFLAGS[sdk=iphonesimulator*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility;
> 
> Is it OK to link with Accessibility all the way back to Monarch? We have open source builders on Monarch.

should be ok. that dylib has been there since 3.0

[Nan Wang]

Comment on attachment 273657 [details]
patch

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

>> Source/WebCore/page/ViewportConfiguration.cpp:194
>> +        minimumScale = std::min(minimumScale, forceAlwaysUserScalableMinimumScale);
> 
> This worries me a bit because there are pages which load by default with scale > 1 (e.g. "width=200px" in the viewport meta tag), and you're going to cause them to shrink down.

I've kept all the minimumScale calculation logic below the same. I suppose it would also respect the layout restrictions.

[WebKit Commit Bot]

Comment on attachment 273657 [details]
patch

Clearing flags on attachment: 273657

Committed r197986: <http://trac.webkit.org/changeset/197986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #57)
> Comment on attachment 273657 [details]
> patch
> 
> Clearing flags on attachment: 273657
> 
> Committed r197986: <http://trac.webkit.org/changeset/197986>

Build fix for Production builds:

Committed r197996:  <http://trac.webkit.org/changeset/197996>