Summary: | Need WK2 API to give a WebView a header and footer | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||
Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, bdakin, cmarcelo, eric, esprehn+autocc, jamesr, jeffm, luiz, mark.lam, ojan.autocc, sam, simon.fraser, thorton, tonikitoo, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Beth Dakin
2013-03-26 16:14:44 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 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 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 *. (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? (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). Created attachment 195223 [details]
Patch
Here's a a patch that addresses everyone's feedback.
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 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? Thanks, Simon! I addressed your comments and added some tests. http://trac.webkit.org/changeset/147039 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 Windows build fix landed in r147043: <http://trac.webkit.org/changeset/147043>. 2 more symbols for the Apple Win debug build. Landed on in r147044: <http://trac.webkit.org/changeset/147044>. Reverted r147044 in r144046: <http://trac.webkit.org/changeset/147046>. |