Bug 131863

Summary: [iOS WebKit2] Implement CSS viewport units
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, esprehn+autocc, glenn, kondapallykalyan, paulo, sam, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
patch darin: review+

Description Tim Horton 2014-04-18 16:09:32 PDT
<rdar://problem/16279088>
Comment 1 Tim Horton 2014-04-18 17:38:57 PDT
Created attachment 229693 [details]
WIP
Comment 2 Benjamin Poulain 2014-04-18 20:07:45 PDT
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.
Comment 3 Tim Horton 2014-04-18 21:58:05 PDT
Created attachment 229717 [details]
patch
Comment 4 Darin Adler 2014-04-19 14:56:28 PDT
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.
Comment 5 Benjamin Poulain 2014-04-19 16:04:59 PDT
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.
Comment 6 Simon Fraser (smfr) 2014-04-19 16:19:07 PDT
What should viewport units report for subframes?
Comment 7 Benjamin Poulain 2014-04-19 16:22:27 PDT
(In reply to comment #6)
> What should viewport units report for subframes?

viewportSize would be empty, RenderView would fallback to visibleContentRectIncludingScrollbars().
Comment 8 Tim Horton 2014-04-21 11:24:31 PDT
(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.
Comment 9 Tim Horton 2014-04-21 13:23:36 PDT
http://trac.webkit.org/changeset/167616
Comment 10 Benjamin Poulain 2014-06-11 15:14:12 PDT
*** Bug 127446 has been marked as a duplicate of this bug. ***
Comment 11 Benjamin Poulain 2014-06-11 15:14:51 PDT
*** Bug 127771 has been marked as a duplicate of this bug. ***