Bug 113352 – Need WK2 API to give a WebView a header and footer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

We need WK2 API that can allow a WebView to have a header and footer. The scrolling machinery in WebCore will need to be able to scroll to the header and footer too.

<rdar://problem/13383835>

[Beth Dakin]

Created attachment 195186 [details]
Patch

I have a follow-up patch that will fix position:fixed and sticky elements when you are in the mode. I separated that code out for now because the patch is big enough as it is.

[Simon Fraser (smfr)]

Comment on attachment 195186 [details]
Patch

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.

[Sam Weinig]

Comment on attachment 195186 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/mac/WKBundlePagePrivateMac.h:40
> +WK_EXPORT CALayer * WKBundlePageGetHeaderLayer(WKBundlePageRef page);
> +WK_EXPORT void WKBundlePageSetHeaderLayer(WKBundlePageRef page, CALayer *headerLayer);
> +WK_EXPORT CALayer * WKBundlePageGetFooterLayer(WKBundlePageRef page);
> +WK_EXPORT void WKBundlePageSetFooterLayer(WKBundlePageRef page, CALayer *footerLayer);

I think we should make the API be setting a layer and its height, and then WebKit would be responsible for setting the layers size based on the width of the viewport.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:373
> +    CALayer * getHeaderLayer() const;

There shouldn't be a space after the *.

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 195186 [details])
> 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.
> 

Switched to an int.

> 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()?
> 

I went with totalContentsSize for now.

> > 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.

willDestroyLayer() is currently a private member of RenderLayerBacking, and this code is in RenderLayerCompositor. It doesn't seem like any of the other layers that are owned by the compositor call this either. What is the best way to proceed?

[Simon Fraser (smfr)]

(In reply to comment #4)
> > > 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.
> 
> willDestroyLayer() is currently a private member of RenderLayerBacking, and this code is in RenderLayerCompositor. It doesn't seem like any of the other layers that are owned by the compositor call this either. What is the best way to proceed?

Ah OK, no need to call it then (it's just used for tiled layer bookkeeping, and these layers will never be tiled I assume).

[Beth Dakin]

Created attachment 195223 [details]
Patch

Here's a a patch that addresses everyone's feedback.

[WebKit Review Bot]

Attachment 195223 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/SpatialNavigation.cpp', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm', u'Source/WebCore/platform/ScrollView.cpp', u'Source/WebCore/platform/ScrollableArea.cpp', u'Source/WebCore/platform/ScrollableArea.h', u'Source/WebCore/platform/mac/ScrollAnimatorMac.mm', u'Source/WebCore/platform/mac/ThemeMac.mm', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/mac/WKBundlePagePrivateMac.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/mac/WKBundlePagePrivateMac.mm', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebCore/page/FrameView.h:410:  The parameter name "headerHeight" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/page/FrameView.h:412:  The parameter name "footerHeight" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 195223 [details]
Patch

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

I think we need to add window.internals methods for adding header and footer height, and add some tests for basic hit testing etc with a header. Otherwise we have no way for other platforms to tell when they break the geometry logic with headers.

> Source/WebCore/page/FrameView.cpp:892
> +    RenderView* renderView = this->renderView();

Assert that this is the root FrameView?

> Source/WebCore/page/FrameView.cpp:904
> +{

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2493
> +    m_layerForBottomOverhangArea->setPosition(FloatPoint(0, m_rootContentLayer->size().height() + m_renderView->frameView()->footerHeight() + m_renderView->frameView()->headerHeight()));

Seems logical to say headerHeight + footerHeight rather than the other way around.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2519
> +    m_layerForHeader->setPosition(FloatPoint(0, 0));
> +    return m_layerForHeader.get();

Don't we want to update the width of the header layer too?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2543
> +    m_layerForFooter->setPosition(FloatPoint(0, m_rootContentLayer->size().height() + m_renderView->frameView()->headerHeight()));

Ditto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:824
> +void WebPage::setHeaderLayerWithHeight(CALayer *childLayer, int height)

Not sure that the "child" adds anything.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:838
> +    m_headerLayer.get().bounds = CGRectMake(0, 0, size().width(), height);
> +    parentLayer->setSize(FloatSize(m_headerLayer.get().bounds.size));

Who's responsible for updating the width and contents of the header and footer layers when the WKView is resized?

[Beth Dakin]

Thanks, Simon! I addressed your comments and added some tests. http://trac.webkit.org/changeset/147039

[Mark Lam]

This cause build failure in Apple Win bots:
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/46660/steps/compile-webkit/logs/stdio

19>Linking...
19>   Creating library C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\DumpRenderTree.lib and object C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\DumpRenderTree.exp
19>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::FrameView::setHeaderHeight(int)" (?setHeaderHeight@FrameView@WebCore@@QAEXH@Z) referenced in function "public: void __thiscall WebCore::Internals::setHeaderHeight(class WebCore::Document *,float)" (?setHeaderHeight@Internals@WebCore@@QAEXPAVDocument@2@M@Z)
19>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::FrameView::setFooterHeight(int)" (?setFooterHeight@FrameView@WebCore@@QAEXH@Z) referenced in function "public: void __thiscall WebCore::Internals::setFooterHeight(class WebCore::Document *,float)" (?setFooterHeight@Internals@WebCore@@QAEXPAVDocument@2@M@Z)
19>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin\DumpRenderTree.dll : fatal error LNK1120: 2 unresolved externals

[Mark Lam]

Windows build fix landed in r147043: <http://trac.webkit.org/changeset/147043>.

[Mark Lam]

2 more symbols for the Apple Win debug build.  Landed on in r147044: <http://trac.webkit.org/changeset/147044>.

[Mark Lam]

Reverted r147044 in r144046: <http://trac.webkit.org/changeset/147046>.