[webkit-reviews] review granted: [Bug 223056] Optimize canvas repaints : [Attachment 423245] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 223056: Optimize canvas repaints
https://bugs.webkit.org/show_bug.cgi?id=223056

Attachment 423245: Patch

https://bugs.webkit.org/attachment.cgi?id=423245&action=review




--- Comment #5 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 423245
  --> https://bugs.webkit.org/attachment.cgi?id=423245
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1322
>	   boundingRect.inflate(state().lineWidth / 2);

Should we call inflateStrokeRect() since it handles the miterLimit and lineCap
of the stroke?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2094
> +    if (m_dirtyRect == oldDirtyRect)
> +	   canvasBase().didDraw(WTF::nullopt);
> +    else
> +	   canvasBase().didDraw(m_dirtyRect);

Maybe

if (m_dirtyRect.contains(oldDirtyRect))
    canvasBase().didDraw(WTF::nullopt);
else {
    m_dirtyRect.unite(dirtyRect);
    canvasBase().didDraw(m_dirtyRect);
}

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2104
> +    return m_dirtyRect == backingStoreBounds();

Is it okay to check for FloatRect equality like this? Should we use some sort
of areEssentiallyEqual()? Or maybe comparing enclosingIntRect() instead?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2549
> +    if (isEntireBackingStoreDirty())
> +	   didDraw(WTF::nullopt);
> +    else if (repaintEntireCanvas)
> +	   didDrawEntireCanvas();
> +    else
>	   didDraw(textRect);

This code was repeated multiple times in this patch. Can't this logic be moved
to didDraw()? For example, pass 'repaintEntireCanvas' to didDraw() and make
didDraw() check for isEntireBackingStoreDirty().