[webkit-reviews] review denied: [Bug 45845] [chromium] WebViewImpl should not destroy accelerated compositor when compositing is not needed : [Attachment 67744] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 45845: [chromium] WebViewImpl should not destroy accelerated compositor
when compositing is not needed
https://bugs.webkit.org/show_bug.cgi?id=45845

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
Looks like a great optimization but I think the logic is off.

View in context:
https://bugs.webkit.org/attachment.cgi?id=67744&action=prettypatch

> WebKit/chromium/src/WebViewImpl.cpp:317
> +    m_layerRenderer.clear();
You don't have to do this. m_layerRenderer is an OwnPtr() and will delete what
it points to when it is destroyed.

> WebKit/chromium/src/WebViewImpl.cpp:2313
>	   m_layerRenderer =
LayerRendererChromium::create(getOnscreenGLES2Context());
>	   if (m_layerRenderer) {
>	       m_isAcceleratedCompositingActive = true;
> +	       m_compositorCreationFailed = false;
I don't quite understand - if someone calls setIsAcceleratedCompositing(false)
then setIsAcceleratedCompositing(true), then on the second call the assignment
to m_layerRenderer will still destroy the old LayerRendererChromium and then
create a new one.  Did you mean to null-check m_layerRenderer before creating a
new one?