We need API that allows a client to set an image that it wants to draw into the top of bottom overhang area. Patch forthcoming. <rdar://problem/13291415>
Created attachment 191914 [details] Patch
Comment on attachment 191914 [details] Patch Attachment 191914 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16986147
Comment on attachment 191914 [details] Patch Attachment 191914 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17006062
Comment on attachment 191914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191914&action=review > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). > + > + No new tests (OOPS!). You should say a bit more here about what these overhang areas are intended for. > Source/WebCore/page/ChromeClient.h:228 > + virtual bool wantsTopOverhangImage() const; This talks about images, but the setUp function talks about layers. Confusing. > Source/WebCore/page/ChromeClient.h:229 > + virtual void setUpTopOverhangAreaLayer(GraphicsLayer*); setupFoo seems slightly more common than setUpFoo in the codebase. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1586 > + if (m_layerForTopOverhangArea || (!m_renderView->document()->ownerElement() && page()->chrome()->client()->wantsTopOverhangImage())) > + updateLayerForTopOverhangArea(); Why check !m_renderView->document()->ownerElement() && page()->chrome()->client()->wantsTopOverhangImage()) again here. Won't you only have an overhang layer if the client wants one? You could just call updateLayerForTopOverhangArea() unconditionally, and have it return early if there isn't a layer. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2435 > + ASSERT(!m_renderView->document()->ownerElement()); I really wish we just said foo->isMainFrame() rather than doing this obscure check everywhere. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2534 > + if (!m_renderView->document()->ownerElement()) { > + if (page()->chrome()->client()->wantsTopOverhangImage() && !m_layerForTopOverhangArea) > + updateLayerForTopOverhangArea(); > + else if (m_layerForTopOverhangArea) { > + m_layerForTopOverhangArea->removeFromParent(); > + m_layerForTopOverhangArea = nullptr; > + } I don't really like how the layer creation code is separated from the clearing code. There should be one function that makes or deletes the layer, and that should be the only function to call wantsTopOverhangImage(). I think the flow control is also wrong here. if (page()->chrome()->client()->wantsTopOverhangImage() && !m_layerForTopOverhangArea) do A else if (m_layerForTopOverhangArea) do B You'll do B when wantsTopOverhangImage() is true and you have a layer. And that clears the layer, which seems wrong. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebChromeClientMac.mm:51 > + layer->platformLayer().contents = [[(id)cgImage.get() retain] autorelease]; You should be able to just do layer.contents = (id)cgImage.get(). > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1392 > + frameView->forceLayout(); Ouch! I think we should find a less sledgehammer approach to get RLC to make the overhang layer.
Created attachment 192093 [details] Patch Simon and I discussed in person and came up with this slightly different approach which avoids the ChromeClient entirely.
Comment on attachment 192093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192093&action=review r+ but I'd like to see the position/anchor point stuff resolved. > Source/WebCore/page/FrameView.cpp:871 > + return renderView->compositor()->updateLayerForTopOverhangArea(wantsLayer); This should be #ifdeffed for ACCELERATED_COMPOSITING, and even then probably with a null-check on the compositor. > Source/WebCore/page/FrameView.cpp:879 > + return renderView->compositor()->updateLayerForBottomOverhangArea(wantsLayer); Ditto. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:796 > + layer->setPosition(FloatPoint(0, -image->size().height())); If you set the anchor point to (0,1) then the position here should be (0,0)
http://trac.webkit.org/changeset/145156