[webkit-reviews] review denied: [Bug 77135] [chromium] canvas demo is slow due to unnecessary resource cleanups : [Attachment 125201] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 77135: [chromium] canvas demo is slow due to unnecessary resource cleanups
https://bugs.webkit.org/show_bug.cgi?id=77135

Attachment 125201: proposed patch
https://bugs.webkit.org/attachment.cgi?id=125201&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125201&action=review


This again won't work.	Layout can happen at nearly any time, not just when
called by WebViewImpl, since javascript can force layout to happen inline.

> Source/WebCore/ChangeLog:8
> +	   Do not release layer resources during Layout.

bizarre capitalization of a random Word here.

> Source/WebCore/ChangeLog:9
> +	   Collect all render layers being orphaned and release them at the end
of Layout.

same Here

> Source/WebKit/chromium/src/WebViewImpl.cpp:1183
>	   // setFrameRect may have the side-effect of causing existing page
>	   // layout to be invalidated, so layout needs to be called last.
> +	   if (m_layerTreeHost)
> +	       m_layerTreeHost->beginLayout();

there are many other ways to enter layout beside this one. Set a breakpoint in
GraphicsLayerChromium::setChildren() and see.

I don't think this approach is scalable.