Bug 111679 – Need API to draw custom overhang area

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

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>

[Beth Dakin]

Created attachment 191914 [details]
Patch

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 191914 [details]
Patch

Attachment 191914 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17006062

[Simon Fraser (smfr)]

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.

[Beth Dakin]

Created attachment 192093 [details]
Patch

Simon and I discussed in person and came up with this slightly different approach which avoids the ChromeClient entirely.

[Simon Fraser (smfr)]

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)

[Beth Dakin]

http://trac.webkit.org/changeset/145156