[webkit-reviews] review denied: [Bug 137376] Defer resolution of viewport size : [Attachment 239301] simplify patch, add description to ChangeLog

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Martin Hock
<mhock at apple.com>'s request for review:
Bug 137376: Defer resolution of viewport size
https://bugs.webkit.org/show_bug.cgi?id=137376

Attachment 239301: simplify patch, add description to ChangeLog
https://bugs.webkit.org/attachment.cgi?id=239301&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239301&action=review


I believe this is incomplete. There can be no use of
m_configuration.width/height without resolving it to a real value.

> Source/WebCore/page/ViewportConfiguration.cpp:293
> +double ViewportConfiguration::configLength(double length) const

Avoid abbreviations in WebKit.

> Source/WebCore/page/ViewportConfiguration.cpp:298
> +    if (length == ViewportArguments::ValueDeviceWidth)
> +	   return activeMinimumLayoutSizeInScrollViewCoordinates().width();
> +    if (length == ViewportArguments::ValueDeviceHeight)
> +	   return activeMinimumLayoutSizeInScrollViewCoordinates().height();

This is fragile.

I would just have 2 getters that resolve the configuration to a real value, for
width and height.