Bug 133162 – [iOS][WK2] Add support for minimal-ui viewports

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS][WK2] Add support for minimal-ui viewports

[Benjamin Poulain]

Created attachment 231846 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231846 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        When the first frame is rendered, the WebKit layer calls pageWillRenderFirstFrame() which freeze

freezes

> Source/WebKit2/ChangeLog:8
> +        In the WebKit2 layers, we have two part to minimal-ui.

two parts

> Source/WebKit2/ChangeLog:10
> +         An other part is freezing the state on the first frame.

Another

> Source/WebCore/page/ViewportConfiguration.h:86
> +    void pageWillRenderFirstFrame();

"frame" is ambiguous (with Frame/iframe).  Maybe didFirstRender or something? Can you just leverage a layout milestone?

> Source/WebCore/page/ViewportConfiguration.h:118
> +    bool m_firstFrameCommited;

Name should be consistent with pageWillRenderFirstFrame. "Committing" is a WK2/compositing notion, and should not be used here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:103
> +@property (nonatomic, setter=_setLargestUnobscuredSizeOverride:) CGSize _largestUnobscuredSizeOverride;

Not sure what override means.

> Source/WebKit2/UIProcess/WebPageProxy.h:602
> +    void setLargestUnobscuredSize(const WebCore::FloatSize&);

largest -> maximum?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1930
> +    if (m_hasRenderedContentAfterDidCommitLoad)
> +        return;
> +
> +    m_hasRenderedContentAfterDidCommitLoad = true;
> +    m_viewportConfiguration.pageWillRenderFirstFrame();

Don't really like this statefulness in another place. We should use layout milestones for this.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:249
> +    m_webPage->willFlushLayers();
> +
>      m_webPage->layoutIfNeeded();

I think you should call willFlush after layoutIfNeeded (in case it tries to query layout).

[Tim Horton]

Comment on attachment 231846 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Initialy, the page is initialized with resetMinimalUI() and start with a normal layout.

typo "initially"

> Source/WebKit2/ChangeLog:8
> +        In the WebKit2 layers, we have two part to minimal-ui.

typo "parts"

> Source/WebKit2/ChangeLog:10
> +         An other part is freezing the state on the first frame.

typo "another"

> Source/WebCore/page/ViewportConfiguration.cpp:117
> +void ViewportConfiguration::pageWillRenderFirstFrame()

I have already registered my objection to this part :D

[Benjamin Poulain]

Created attachment 231849 [details]
Patch

[Benjamin Poulain]

<rdar://problem/16283914>

[Benjamin Poulain]

Created attachment 231930 [details]
Patch

[Benjamin Poulain]

Created attachment 231933 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231933 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Initially, the page is initialized with resetMinimalUI() and start with a normal layout.

starts with

> Source/WebCore/ChangeLog:18
> +        layout size. Any layout after that take into account the minimal-ui.

takes minimal-ui into account.

> Source/WebCore/page/ViewportConfiguration.cpp:120
> +    if (m_pageDidFinishDocumentLoad)
> +        return;

No point having these lines.

> Source/WebCore/page/ViewportConfiguration.cpp:286
>  int ViewportConfiguration::layoutWidth() const

This should return LayoutUnits at some point?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:99
> +// Define the smallest size a page take with a regular.

With a regular what?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:253
> +    void didFinishDocumentLoad(WebFrame*);
>      void didFinishLoad(WebFrame*);

What's the difference? Is the first like DOMContentLoaded?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1992
>      resetTextAutosizingBeforeLayoutIfNeeded(m_viewportConfiguration.minimumLayoutSize(), minimumLayoutSize);
> +    resetTextAutosizingBeforeLayoutIfNeeded(m_viewportConfiguration.minimumLayoutSizeForMinimalUI(), minimumLayoutSizeForMinimalUI);

Do we have to call resetTextAutosizingBeforeLayoutIfNeeded twice?

[Benjamin Poulain]

Thanks for the review!

(In reply to comment #8)
> > Source/WebCore/page/ViewportConfiguration.cpp:286
> >  int ViewportConfiguration::layoutWidth() const
> 
> This should return LayoutUnits at some point?

No idea. Do we plan to support non-integer document sizes?

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:253
> > +    void didFinishDocumentLoad(WebFrame*);
> >      void didFinishLoad(WebFrame*);
> 
> What's the difference? Is the first like DOMContentLoaded?

Exactly :)
 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1992
> >      resetTextAutosizingBeforeLayoutIfNeeded(m_viewportConfiguration.minimumLayoutSize(), minimumLayoutSize);
> > +    resetTextAutosizingBeforeLayoutIfNeeded(m_viewportConfiguration.minimumLayoutSizeForMinimalUI(), minimumLayoutSizeForMinimalUI);
> 
> Do we have to call resetTextAutosizingBeforeLayoutIfNeeded twice?

We could add some code to do it, but it should not matter much, the layout on resize is so expensive this will be negligible.

[Benjamin Poulain]

Committed r169245: <http://trac.webkit.org/changeset/169245>