Bug 113352 - Need WK2 API to give a WebView a header and footer
Summary: Need WK2 API to give a WebView a header and footer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-26 16:14 PDT by Beth Dakin
Modified: 2013-03-27 19:29 PDT (History)
15 users (show)

See Also:


Attachments
Patch (47.55 KB, patch)
2013-03-26 16:38 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (49.58 KB, patch)
2013-03-26 22:24 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2013-03-26 16:14:44 PDT
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>
Comment 1 Beth Dakin 2013-03-26 16:38:05 PDT
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.
Comment 2 Simon Fraser (smfr) 2013-03-26 16:45:39 PDT
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.
Comment 3 Sam Weinig 2013-03-26 17:12:36 PDT
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 *.
Comment 4 Beth Dakin 2013-03-26 20:45:54 PDT
(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?
Comment 5 Simon Fraser (smfr) 2013-03-26 21:13:31 PDT
(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).
Comment 6 Beth Dakin 2013-03-26 22:24:13 PDT
Created attachment 195223 [details]
Patch

Here's a a patch that addresses everyone's feedback.
Comment 7 WebKit Review Bot 2013-03-26 22:26:57 PDT
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.
Comment 8 Simon Fraser (smfr) 2013-03-27 13:41:31 PDT
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?
Comment 9 Beth Dakin 2013-03-27 17:41:19 PDT
Thanks, Simon! I addressed your comments and added some tests. http://trac.webkit.org/changeset/147039
Comment 10 Mark Lam 2013-03-27 18:25:47 PDT
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
Comment 11 Mark Lam 2013-03-27 18:40:10 PDT
Windows build fix landed in r147043: <http://trac.webkit.org/changeset/147043>.
Comment 12 Mark Lam 2013-03-27 18:53:57 PDT
2 more symbols for the Apple Win debug build.  Landed on in r147044: <http://trac.webkit.org/changeset/147044>.
Comment 13 Mark Lam 2013-03-27 19:29:55 PDT
Reverted r147044 in r144046: <http://trac.webkit.org/changeset/147046>.