[webkit-reviews] review denied: [Bug 109579] [CoordGfx] Speed up web process rendering with double buffering : [Attachment 196015] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 109579: [CoordGfx] Speed up web process rendering with double buffering
https://bugs.webkit.org/show_bug.cgi?id=109579

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

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196015&action=review


> Source/WebCore/ChangeLog:9
> +	   This is an optimization that has the most value for animations that
are heavy in layout/repaint.

I would say for JS-driven animations.

> Source/WebCore/platform/graphics/texmap/coordinated/AreaAllocator.cpp:356
> +	   if (current->largestFree.isEmpty() && !left && !right)
> +	       m_markedNodes.append(current);
> +	   else {

continue;
(reduce indentations)

> Source/WebCore/platform/graphics/texmap/coordinated/AreaAllocator.cpp:372
> +	   Node* n = m_markedNodes[i];

n -> node

> Source/WebCore/platform/graphics/texmap/coordinated/AreaAllocator.h:72
> +    // It makes possible to devide the area into back and front buffers.

devide -> divide

> Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:65
> +    // Do not release allocations if this is a new atlas that has never been
committed yet.
> +    if (!m_hasCommittedOnce)
> +	   return;

Seems like this should be m_hasMarkedAllocations, and this boolean should be
set to true in every cycle we mark anything in that atlas. Otherwise we'd end
up going through the atlas unmarking allocations for every frame except the
very first one.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.
cpp:320
> +	   m_hasPrerenderedFrame = m_shouldSyncFrame;

What's the difference between those 2 booleans? Seems like one of them is
enough.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.
cpp:338
> +    m_state.contentsSize =
roundedIntSize(m_nonCompositedContentLayer->size());
> +    m_state.coveredRect =
toCoordinatedGraphicsLayer(m_nonCompositedContentLayer.get())->coverRect();
> +    m_state.scrollPosition = m_visibleContentsRect.location();

You have to make sure here that the two rendered frames have the same geometry
parameters. Otherwise this would lead to interesting bugs.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.
cpp:510
> +    if (!m_waitingForUIProcess)
> +	   lockAnimations();

lockAnimations() has to die... but that's a different issue.