Defer resolution of viewport size

[Martin Hock]

Defer resolution of viewport size.

[Martin Hock]

Created attachment 239238 [details]
patch

[Tim Horton]

Comment on attachment 239238 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239238&action=review

> Source/WebCore/ChangeLog:7
> +

Definitely needs more justification here.

[Martin Hock]

Created attachment 239301 [details]
simplify patch, add description to ChangeLog

[Benjamin Poulain]

Comment on attachment 239301 [details]
simplify patch, add description to ChangeLog

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.

[Martin Hock]

Created attachment 239379 [details]
resolve on configuration update

[Martin Hock]

(In reply to comment #4)
> (From update of attachment 239301 [details])
> 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.

I've refactored it in my new patch to apply when the configuration is changed.

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

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

Based on our discussion and the fact that we can set width or height to be device-width or device-height, this logic seems correct.

[Benjamin Poulain]

Comment on attachment 239379 [details]
resolve on configuration update

LGTM.

[Martin Hock]

Committed r174405: <http://trac.webkit.org/changeset/174405>