[webkit-reviews] review granted: [Bug 215989] REGRESSION(r262366): MotionMark1.1 | macOS | Some devices | 1-3% overall regression : [Attachment 407571] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 215989: REGRESSION(r262366): MotionMark1.1 | macOS | Some devices | 1-3%
overall regression
https://bugs.webkit.org/show_bug.cgi?id=215989

Attachment 407571: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 407571
  --> https://bugs.webkit.org/attachment.cgi?id=407571
Patch

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

> Source/WebCore/dom/Document.cpp:8639
> +    if (m_canvasesNeedingDisplayPreparation.isEmpty())
> +	   return;

This isn’t needed. The code below already efficiently does nothing in this
case. Making an empty vector, an empty for loop, and an empty clear function
are all efficient.

> Source/WebCore/html/CanvasBase.cpp:140
> +    if (m_observers.isEmpty())
> +	   return;

This isn’t needed. The code below already efficiently does nothing in this
case. Making an empty vector and an empty for loop are efficient.

> Source/WebCore/html/CanvasBase.cpp:149
> +    if (m_observers.isEmpty())
> +	   return;

This isn’t needed. The code below already efficiently does nothing in this
case. Making an empty vector and an empty for loop are efficient.

> Source/WebCore/html/CanvasBase.cpp:162
> +    if (m_observers.isEmpty()) {
> +#if ASSERT_ENABLED
> +	   m_didNotifyObserversCanvasDestroyed = true;
> +#endif
> +	   return;
> +    }

This isn’t needed. The code below already efficiently does nothing in this
case. Making an empty vector, an empty for loop, and an empty clear function
are all efficient.

> Source/WebCore/html/HTMLCanvasElement.cpp:1054
> +    if (needsPreparationForDisplay()) {
> +	   if (insertionType.connectedToDocument) {

I suggest && rather than a nested if statement.

> Source/WebCore/html/HTMLCanvasElement.cpp:1055
> +	       auto& doc = parentOfInsertedTree.document();

Please use document, not doc.

> Source/WebCore/html/HTMLCanvasElement.cpp:1060
> +	       if (is<WebGLRenderingContextBase>(m_context.get())) {
> +		   if
(downcast<WebGLRenderingContextBase>(m_context.get())->compositingResultsNeedUp
dating())

Three thoughts:

1) Instead of "(m_context.get())->" you can use "(*m_context)." and this
applies here and other places in the file.
2) Instead of nested if you can use &&, which I think is clearer, but it ends
up being a long line because of repeating the WebGLRenderingContextBase class
name twice.
3) Could make compositingResultsNeedUpdating a virtual function and avoid the
type check entirely.

> Source/WebCore/html/HTMLCanvasElement.cpp:1072
> +    if (needsPreparationForDisplay()) {
> +	   if (removalType.disconnectedFromDocument) {

I suggest && rather than a nested if statement.

> Source/WebCore/html/HTMLCanvasElement.cpp:1084
> +    if (!m_context)
> +	   return false;

This isn’t needed. The code below already efficiently returns false when
m_context is null.