[webkit-reviews] review denied: [Bug 111679] Need API to draw custom overhang area : [Attachment 191914] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 111679: Need API to draw custom overhang area
https://bugs.webkit.org/show_bug.cgi?id=111679

Attachment 191914: Patch
https://bugs.webkit.org/attachment.cgi?id=191914&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.