Bug 55416

Summary:

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

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dbates, ddkilzer, eric, joepeck, kenneth, koivisto, simon.fraser, webkit.review.bot, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
[PATCH] Convert userScalable back to float	kenneth: review+
[PATCH] Improve: Add userScalable to LayoutTests print statements	kenneth: review+, joepeck: commit-queue-

Description Joseph Pecoraro 2011-02-28 15:03:44 PST

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.

Comment 1 Kenneth Rohde Christiansen 2011-03-01 01:03:38 PST

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

Comment 2 Joseph Pecoraro 2011-03-07 20:57:05 PST

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.

Comment 3 WebKit Review Bot 2011-03-07 20:59:53 PST

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.

Comment 4 Joseph Pecoraro 2011-03-07 21:00:12 PST

(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.

Comment 5 Joseph Pecoraro 2011-03-07 21:02:16 PST

> 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 6 Kenneth Rohde Christiansen 2011-03-08 00:57:57 PST

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.)

Comment 7 Kenneth Rohde Christiansen 2011-03-08 00:59:01 PST

(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.

Comment 8 Kenneth Rohde Christiansen 2011-03-08 00:59:55 PST

> 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.

Comment 9 Joseph Pecoraro 2011-03-09 09:30:21 PST

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

In that case we should await another patch.

Comment 10 Joseph Pecoraro 2011-03-09 14:51:28 PST

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.

Comment 11 Joseph Pecoraro 2011-03-14 13:47:21 PDT

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 12 Kenneth Rohde Christiansen 2011-03-15 01:38:39 PDT

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

This looks pretty good.

Comment 13 Joseph Pecoraro 2011-03-16 18:54:30 PDT

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

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

Comment 14 WebKit Review Bot 2011-03-16 19:48:49 PDT

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

Comment 15 Joseph Pecoraro 2011-03-16 19:50:24 PDT

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.

Comment 16 Joseph Pecoraro 2011-03-16 19:53:09 PDT

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

Comment 17 Joseph Pecoraro 2011-03-16 21:23:31 PDT

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

Comment 18 Joseph Pecoraro 2011-03-16 21:28:25 PDT

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