Bug 55416 – Viewport no longer allows an auto value for "user-scalable"

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

With the ViewportArguments change in r67376 the "default" keyword which explicitly
meant "auto" was lost. More noticeable is that the "user-scalable" argument lost
the concept of auto altogether and now is only true or false (was float, now bool):
http://trac.webkit.org/changeset/67376

This is problematic if the browser wants to set a default user-scalable value. It always
sees the Viewport coming from WebCore as an explicit true or false, never a "undefined
value, use default or auto". Also previously, the "default" keyword provided users
with the option of changing the viewport from something they set explicitly, to fallback
to the browser default. That is no longer possible.

I don't think this concept is mentioned in the "css-viewport" spec, but this is a
regression from former behavior and functionality:
http://people.opera.com/rune/TR/css-viewport/

The "auto/default" value is a browser level concept, so it doesn't look like there
is an obvious solution in ViewportArguments. This is problematic since
ViewportArguments::computeViewportAttributes does nice validation/processing
of user-scalable (automatically equating max/min/initial scale). An ideal solution
would not require duplicating that validation.

[Kenneth Rohde Christiansen]

I'm totally OK with making this a float again. It would be good if we could make some tests for it as well.

[Joseph Pecoraro]

Created attachment 85015 [details]
[PATCH] Convert userScalable back to float

First attempt at a patch. userScalable is a float that starts with ValueAuto.
WebKit clients handle ValueAuto (currently all just make it "true") in client code
after computing the viewport attributes and still seeing an Auto value.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:564:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/dom/ViewportArguments.cpp:175:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/qt/Api/qwebpage.cpp:2543:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/gtk/webkit/webkitviewportattributes.cpp:545:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/efl/ewk/ewk_view.cpp:4308:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 11 files


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

[Joseph Pecoraro]

(In reply to comment #1)
> I'm totally OK with making this a float again. It would be good if we
> could make some tests for it as well.

My patch just makes all the WebKit ports set userScalable to true,
if it was seen as Auto after computing viewport attributes. So the
existing tests and behavior of those ports won't change. This
is basically handled by all the tests that don't define "user-scalable".

For example:

  <meta name="viewport" content="width=100">

The userScalable argument is Auto, and current ports will default
to userScalable true. I suppose I could add output about the
userScalable property to the existing DumpRenderTree
implementations. They print everything but the userScalable value.

[Joseph Pecoraro]

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:564:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Bah, I just wanted to be explicit about the conversion from non-zero to true.
Since possible values are -1, 0, and 1, it seems weird to me here to do
static_cast<bool>(someFloat), but I could do that. Same with !0.

[Kenneth Rohde Christiansen]

Comment on attachment 85015 [details]
[PATCH] Convert userScalable back to float

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

>> Source/WebCore/dom/ViewportArguments.cpp:175
>> +    if (args.userScalable == 0.0f)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Our coding style actually says to always use 0 and not .0 or .0f. (Section Floating point literals, item 1.)

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:564:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> Bah, I just wanted to be explicit about the conversion from non-zero to true.
> Since possible values are -1, 0, and 1, it seems weird to me here to do
> static_cast<bool>(someFloat), but I could do that. Same with !0.

The coding style is educational and a guide line. If it makes sense you are allowed to divert from it, though I think !0 would be fine here.

[Kenneth Rohde Christiansen]

> The userScalable argument is Auto, and current ports will default
> to userScalable true. I suppose I could add output about the
> userScalable property to the existing DumpRenderTree
> implementations. They print everything but the userScalable value.

Would be good to add this to the output, yes.

[Joseph Pecoraro]

> Would be good to add this to the output, yes.

In that case we should await another patch.

[Joseph Pecoraro]

Also this point was raised:

> Don't you want a way to generate a warning if a bad value is being
> used here?  Specifically when fabs(value) < 1 in the code above.
> Maybe that's outside the scope of this fix, though.

[Joseph Pecoraro]

Created attachment 85714 [details]
[PATCH] Improve: Add userScalable to LayoutTests print statements

Updates:

  • 1.0f and 0.0f => 1 and 0
  • (userScalable != 0.0) => static_cast<bool>(userScalable)
  • updated ChangeLogs
  • Test printf statements
    -    fprintf(stdout, "viewport size %dx%d scale %f with limits [%f, %f]\n", ...
    +    fprintf(stdout, "viewport size %dx%d scale %f with limits [%f, %f] and userScalable %f\n", ...

This would need to be manually landed since ALL viewport tests will
change, and I'll need to update them once I can pull from the bots.

[Kenneth Rohde Christiansen]

Comment on attachment 85714 [details]
[PATCH] Improve: Add userScalable to LayoutTests print statements

This looks pretty good.

[Joseph Pecoraro]

Landed r81309:
http://trac.webkit.org/changeset/81309

I'll be pulling new expected results off of the Gtk/Qt bots.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81309 might have broken Qt Linux Release
The following tests are not passing:
fast/viewport/viewport-1.html
fast/viewport/viewport-10.html
fast/viewport/viewport-100.html
fast/viewport/viewport-101.html
fast/viewport/viewport-102.html
fast/viewport/viewport-103.html
fast/viewport/viewport-104.html
fast/viewport/viewport-105.html
fast/viewport/viewport-106.html
fast/viewport/viewport-107.html
fast/viewport/viewport-108.html
fast/viewport/viewport-109.html
fast/viewport/viewport-11.html
fast/viewport/viewport-110.html
fast/viewport/viewport-111.html
fast/viewport/viewport-112.html
fast/viewport/viewport-113.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-116.html
fast/viewport/viewport-117.html
fast/viewport/viewport-118.html
fast/viewport/viewport-119.html
fast/viewport/viewport-12.html
fast/viewport/viewport-120.html
fast/viewport/viewport-121.html
fast/viewport/viewport-122.html
fast/viewport/viewport-123.html
fast/viewport/viewport-124.html
fast/viewport/viewport-125.html
fast/viewport/viewport-129.html
fast/viewport/viewport-13.html
fast/viewport/viewport-14.html
fast/viewport/viewport-15.html
fast/viewport/viewport-16.html
fast/viewport/viewport-17.html
fast/viewport/viewport-18.html
fast/viewport/viewport-19.html
fast/viewport/viewport-2.html
fast/viewport/viewport-20.html
fast/viewport/viewport-21.html
fast/viewport/viewport-22.html
fast/viewport/viewport-23.html
fast/viewport/viewport-24.html
fast/viewport/viewport-25.html
fast/viewport/viewport-26.html
fast/viewport/viewport-27.html
fast/viewport/viewport-28.html
fast/viewport/viewport-29.html
fast/viewport/viewport-3.html
fast/viewport/viewport-30.html
fast/viewport/viewport-31.html
fast/viewport/viewport-32.html
fast/viewport/viewport-33.html
fast/viewport/viewport-34.html
fast/viewport/viewport-35.html
fast/viewport/viewport-36.html
fast/viewport/viewport-37.html
fast/viewport/viewport-38.html
fast/viewport/viewport-39.html
fast/viewport/viewport-4.html
fast/viewport/viewport-40.html
fast/viewport/viewport-41.html
fast/viewport/viewport-42.html
fast/viewport/viewport-43.html
fast/viewport/viewport-44.html
fast/viewport/viewport-46.html
fast/viewport/viewport-47.html
fast/viewport/viewport-48.html
fast/viewport/viewport-49.html
fast/viewport/viewport-5.html
fast/viewport/viewport-50.html
fast/viewport/viewport-51.html
fast/viewport/viewport-52.html
fast/viewport/viewport-53.html
fast/viewport/viewport-54.html
fast/viewport/viewport-55.html
fast/viewport/viewport-56.html
fast/viewport/viewport-57.html
fast/viewport/viewport-59.html
fast/viewport/viewport-60.html
fast/viewport/viewport-61.html
fast/viewport/viewport-62.html
fast/viewport/viewport-63.html
fast/viewport/viewport-64.html
fast/viewport/viewport-66.html
fast/viewport/viewport-67.html
fast/viewport/viewport-68.html
fast/viewport/viewport-69.html
fast/viewport/viewport-7.html
fast/viewport/viewport-70.html
fast/viewport/viewport-71.html
fast/viewport/viewport-72.html
fast/viewport/viewport-73.html
fast/viewport/viewport-74.html
fast/viewport/viewport-75.html
fast/viewport/viewport-76.html
fast/viewport/viewport-77.html
fast/viewport/viewport-78.html
fast/viewport/viewport-79.html
fast/viewport/viewport-8.html
fast/viewport/viewport-80.html
fast/viewport/viewport-81.html
fast/viewport/viewport-83.html
fast/viewport/viewport-85.html
fast/viewport/viewport-86.html
fast/viewport/viewport-88.html
fast/viewport/viewport-9.html
fast/viewport/viewport-90.html
fast/viewport/viewport-91.html
fast/viewport/viewport-92.html
fast/viewport/viewport-93.html
fast/viewport/viewport-94.html
fast/viewport/viewport-95.html
fast/viewport/viewport-96.html
fast/viewport/viewport-warnings-1.html
fast/viewport/viewport-warnings-2.html
fast/viewport/viewport-warnings-3.html
fast/viewport/viewport-warnings-4.html
fast/viewport/viewport-warnings-5.html
fast/viewport/viewport-warnings-6.html

[Joseph Pecoraro]

Landed expected results in r81312.
http://trac.webkit.org/changeset/81312

Looks like there wasn't actually a test for userScalable=1 or yes. I should add one...
Marking this as resolved though.

[Joseph Pecoraro]

I opened:
<http://webkit.org/b/56527> Viewport test for user-scalable=yes

[Joseph Pecoraro]

Looks like GTK needs a little bit of love. I'll update shortly.

[Joseph Pecoraro]

Cleaning up GTK results in r81318:
http://trac.webkit.org/changeset/81318