Bug 131863 - [iOS WebKit2] Implement CSS viewport units
Summary: [iOS WebKit2] Implement CSS viewport units
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
: 127446 127771 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-04-18 16:09 PDT by Tim Horton
Modified: 2014-06-11 15:14 PDT (History)
8 users (show)

See Also:


Attachments
WIP (13.99 KB, patch)
2014-04-18 17:38 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (12.45 KB, patch)
2014-04-18 21:58 PDT, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***