Bug 131863 – [iOS WebKit2] Implement CSS viewport units

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

<rdar://problem/16279088>

[Tim Horton]

Created attachment 229693 [details]
WIP

[Benjamin Poulain]

Comment on attachment 229693 [details]
WIP

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

Looks good to me.

> Source/WebCore/page/ViewportConfiguration.cpp:108
> +IntSize ViewportConfiguration::layoutSizeForMinimalUI() const

This name is extremely misleading. The layout size is computed from the minimumLayoutSize and all the viewport parameters.

I think I would do this computation out of ViewportConfiguration. ViewportConfiguration is already complicated enough as it is, and the fact that we can use the minimal UI for viewport unit is iOS specific.

[Tim Horton]

Created attachment 229717 [details]
patch

[Darin Adler]

Comment on attachment 229717 [details]
patch

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

> Source/WebCore/rendering/RenderView.cpp:1050
> +    IntSize viewportSize = frameView().frame().page()->viewportSize();
> +    if (!viewportSize.isEmpty())
> +        return viewportSize;

Seems a little strange. I understand that we want to distinguish a size that is set from a size that is not. But it doesn’t seem right that “empty” is now we track “not set”.

> Source/WebCore/rendering/RenderView.h:202
> +    void setViewportSize(IntSize);

In the old days we would invariably have used "const IntSize&", but I gather that’s just old fashioned attempt at optimizing that didn’t really work.

[Benjamin Poulain]

Comment on attachment 229717 [details]
patch

Just a suggestion: maybe viewport size should be stored on FrameView instead of Page. It would make more sense for FrameView/ScrollView to know about the viewport geometry.

RenderView::viewportSize() would just invoke frameView().viewportSize(), and FrameView::viewportSize() would make the decision between the visibleContentRectIncludingScrollbars() and the special viewport size override.

[Simon Fraser (smfr)]

What should viewport units report for subframes?

[Benjamin Poulain]

(In reply to comment #6)
> What should viewport units report for subframes?

viewportSize would be empty, RenderView would fallback to visibleContentRectIncludingScrollbars().

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 229717 [details])
> Just a suggestion: maybe viewport size should be stored on FrameView instead of Page. It would make more sense for FrameView/ScrollView to know about the viewport geometry.
> 
> RenderView::viewportSize() would just invoke frameView().viewportSize(), and FrameView::viewportSize() would make the decision between the visibleContentRectIncludingScrollbars() and the special viewport size override.

OK, I think that sounds like a reasonable change.

[Tim Horton]

http://trac.webkit.org/changeset/167616

[Benjamin Poulain]

*** Bug 127446 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

*** Bug 127771 has been marked as a duplicate of this bug. ***