[webkit-reviews] review granted: [Bug 201594] [FTW] ImageBuffer/ImageBufferData is highly inefficient : [Attachment 378690] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 201594: [FTW] ImageBuffer/ImageBufferData is highly inefficient
https://bugs.webkit.org/show_bug.cgi?id=201594

Attachment 378690: Patch

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




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

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

> Source/WebCore/ChangeLog:22
> +	      an ID2D1Bitmap is requested for drawing.

... and before a drawing operation is performed on the associated ImageBuffer
context (i.e. the other calls to notifyPreDrawObserver).

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:285
> +    if (bitmapBufferSync == BitmapBufferSync::BitmapOutOfSync &&
!data.isEmpty())

Should this if-statement be moved to loadDataToBitmap()?

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:119
> +	   if (m_data.bitmapBufferSync ==
ImageBufferData::BitmapBufferSync::BitmapOutOfSync && !m_data.data.isEmpty())

Ditto.

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:122
> +	   m_data.bitmapBufferSync =
ImageBufferData::BitmapBufferSync::BufferOutOfSync;

Should this be moved to a function in ImageBufferData for better reading,
markBufferOutOfSync() for example?

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:278
> +    if (m_preDrawHandler)
> +	   m_preDrawHandler();

What is the meaning of this if statement? m_preDrawHandler is not a pointer and
it does not return a value. I am wondering what the complier translates it to.
But I think you just need to call m_preDrawHandler() without the if statement.

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:284
> +    if (m_postDrawHandler)
> +	   m_postDrawHandler();

Ditto.