Bug 55416 - Viewport no longer allows an auto value for "user-scalable"
: Viewport no longer allows an auto value for "user-scalable"
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-02-28 15:03 PST by
Modified: 2011-03-16 21:28 PST (History)


Attachments
[PATCH] Convert userScalable back to float (9.20 KB, patch)
2011-03-07 20:57 PST, Joseph Pecoraro
kenneth: review+
Review Patch | Details | Formatted Diff | Diff
[PATCH] Improve: Add userScalable to LayoutTests print statements (11.18 KB, patch)
2011-03-14 13:47 PST, Joseph Pecoraro
kenneth: review+
joepeck: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2011-03-07 20:57:05 PST -------
Created an attachment (id=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 From 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 From 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 From 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 From 2011-03-08 00:57:57 PST -------
(From update of attachment 85015 [details])
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 From 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 From 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 From 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 From 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 From 2011-03-14 13:47:21 PST -------
Created an attachment (id=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 From 2011-03-15 01:38:39 PST -------
(From update of attachment 85714 [details])
This looks pretty good.
------- Comment #13 From 2011-03-16 18:54:30 PST -------
Landed r81309:
http://trac.webkit.org/changeset/81309

I'll be pulling new expected results off of the Gtk/Qt bots.
------- Comment #14 From 2011-03-16 19:48:49 PST -------
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 From 2011-03-16 19:50:24 PST -------
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 From 2011-03-16 19:53:09 PST -------
I opened:
<http://webkit.org/b/56527> Viewport test for user-scalable=yes
------- Comment #17 From 2011-03-16 21:23:31 PST -------
Looks like GTK needs a little bit of love. I'll update shortly.
------- Comment #18 From 2011-03-16 21:28:25 PST -------
Cleaning up GTK results in r81318:
http://trac.webkit.org/changeset/81318