[webkit-reviews] review denied: [Bug 103068] Viewport CSS rules should not clamp values like Viewport META : [Attachment 175784] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Thiago Marcos P.
Santos <tmpsantos at gmail.com>'s request for review:
Bug 103068: Viewport CSS rules should not clamp values like Viewport META
https://bugs.webkit.org/show_bug.cgi?id=103068

Attachment 175784: Patch
https://bugs.webkit.org/attachment.cgi?id=175784&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=175784&action=review


> Source/WebCore/dom/ViewportArguments.cpp:186
> +    } else {
> +	   // Clamp values to a valid range, but not for @viewport since is
> +	   // not mandated by the specification.
> +	   resultWidth = clampLengthValue(resultWidth);
> +	   resultHeight = clampLengthValue(resultHeight);
> +	   resultZoom = clampScaleValue(resultZoom);
> +	   resultMinZoom = clampScaleValue(resultMinZoom);
> +	   resultMaxZoom = clampScaleValue(resultMaxZoom);
>      }

I think an if would be nicer, we probably dont want to clam implicit viewport
either

if (type != ViewportArguments::CSSDeviceAdaptation && type !=
ViewportArguments::Implicit) or maybe the other way around. Make it explicit
what we are clamping.

> Source/WebCore/dom/ViewportArguments.cpp:240
> +    } else {
> +	   // Extend width and height to fill the visual viewport for the
resolved initial-scale.
> +	   resultWidth = max<float>(resultWidth, initialViewportSize.width() /
result.initialScale);
> +	   resultHeight = max<float>(resultHeight, initialViewportSize.height()
/ result.initialScale);
> +    }

I think this should probably only happen for viewport meta, not the old legacy
meta tags which dont support scale anyway