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.
I'm totally OK with making this a float again. It would be good if we could make some tests for it as well.
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.
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.
(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.
> 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.
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.)
(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.
> 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.
> Would be good to add this to the output, yes. In that case we should await another patch.
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.
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.
Comment on attachment 85714 [details] [PATCH] Improve: Add userScalable to LayoutTests print statements This looks pretty good.
Landed r81309: http://trac.webkit.org/changeset/81309 I'll be pulling new expected results off of the Gtk/Qt bots.
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
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.
I opened: <http://webkit.org/b/56527> Viewport test for user-scalable=yes
Looks like GTK needs a little bit of love. I'll update shortly.
Cleaning up GTK results in r81318: http://trac.webkit.org/changeset/81318