Bug 111679 - Need API to draw custom overhang area
Summary: Need API to draw custom overhang area
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Beth Dakin
Keywords: InRadar
Depends on:
Reported: 2013-03-06 21:19 PST by Beth Dakin
Modified: 2013-03-07 17:01 PST (History)
9 users (show)

See Also:

Patch (21.68 KB, patch)
2013-03-06 21:31 PST, Beth Dakin
simon.fraser: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (13.60 KB, patch)
2013-03-07 15:25 PST, 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-06 21:19:17 PST
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.

Comment 1 Beth Dakin 2013-03-06 21:31:40 PST
Created attachment 191914 [details]
Comment 2 Early Warning System Bot 2013-03-06 21:51:15 PST
Comment on attachment 191914 [details]

Attachment 191914 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16986147
Comment 3 EFL EWS Bot 2013-03-06 21:57:49 PST
Comment on attachment 191914 [details]

Attachment 191914 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17006062
Comment 4 Simon Fraser (smfr) 2013-03-06 23:17:18 PST
Comment on attachment 191914 [details]

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.
Comment 5 Beth Dakin 2013-03-07 15:25:25 PST
Created attachment 192093 [details]

Simon and I discussed in person and came up with this slightly different approach which avoids the ChromeClient entirely.
Comment 6 Simon Fraser (smfr) 2013-03-07 16:35:23 PST
Comment on attachment 192093 [details]

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);


> 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)
Comment 7 Beth Dakin 2013-03-07 17:01:20 PST