[webkit-reviews] review granted: [Bug 133162] [iOS][WK2] Add support for minimal-ui viewports : [Attachment 231846] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 21 18:25:45 PDT 2014


Simon Fraser (smfr) <simon.fraser at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 133162: [iOS][WK2] Add support for minimal-ui viewports
https://bugs.webkit.org/show_bug.cgi?id=133162

Attachment 231846: Patch
https://bugs.webkit.org/attachment.cgi?id=231846&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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
> + at 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).


More information about the webkit-reviews mailing list