[webkit-reviews] review denied: [Bug 113352] Need WK2 API to give a WebView a header and footer : [Attachment 195186] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 113352: Need WK2 API to give a WebView a header and footer
https://bugs.webkit.org/show_bug.cgi?id=113352

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195186&action=review


> Source/WebCore/platform/ScrollableArea.h:153
> +    unsigned headerHeight() const { return m_headerHeight; }
> +    void setHeaderHeight(unsigned headerHeight) { m_headerHeight =
headerHeight; }
> +    unsigned footerHeight() const { return m_footerHeight; }
> +    void setFooterHeight(unsigned footerHeight) { m_footerHeight =
footerHeight; }

I don't think we should use unsigned for geometry.

You're also allowing these to be called on any ScrollableArea, but it looks
like the code only supports them for the main frame. You should assert this.

> Source/WebCore/platform/ScrollableArea.h:156
> +    IntSize scrollableAreaSize() const;

I think we need a better name for this. It's too easy to read as "the size of
the scrollable area" (i.e. the outside), not the scrolledContents size. Maybe
totalContentSize()? contentSizeWithHeaderAndFooter()?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2501
> +	   if (m_layerForHeader) {
> +	       m_layerForHeader->removeFromParent();

Please call willDestroyLayer() here.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2526
> +	   if (m_layerForFooter) {
> +	       m_layerForFooter->removeFromParent();

Please call willDestroyLayer() here.