[webkit-reviews] review granted: [Bug 64535] Add fast path for ImageBuffer::draw : [Attachment 101333] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 64535: Add fast path for ImageBuffer::draw
https://bugs.webkit.org/show_bug.cgi?id=64535

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101333&action=review


r=me but please consider the feedback.

> Source/WebCore/ChangeLog:12
> +	   This isn't necessary, but it has been bothering me forever :-)

No need to justify this.

> Source/WebCore/ChangeLog:13
> +	   (WebCore::GraphicsContext::drawImageBuffer): Removed the need to
pass down useLowQualityScale since it's not needed below.

It's odd that it's not needed; is that just because no-one happens to use it in
this code path? Might they in future?

> Source/WebCore/ChangeLog:18
> +	   0) Changed copyImage() to deepCopy() to make it clear this copy is
indeed deep (but could be COW)
> +	   1) Added shallowCopy() that gives a copy that doesn't guarantee to
be a deep copy.

I'm not sure these names are really accurate, but I can't think of better ones.
The main difference seems to be that if you call shallowCopy(), subsequent
drawing into the image buffer will change what you have.

Another approach would be:
enum BackingStoreCopy { CopyBackingStore, DontCopyBackingStore }
copyImage(BackingStoreCopy)

> Source/WebCore/ChangeLog:27
> +	   2) Added in plaformDeepCopy(), and platformShallowCopy() to other
methods that should use them.

plaformDeepCopy is misspelled.

> Source/WebCore/platform/graphics/GraphicsContext.h:268
> +	   void drawImage(NativeImagePtr, const FloatSize& selfSize, ColorSpace
styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect,
CompositeOperator = CompositeSourceOver);

We'd have to be careful that this overload isn't called by mistake if an Image
has an operator NativeImagePtr(). I'd suggest renaming it to drawNativeImage()

> Source/WebCore/platform/graphics/ImageBuffer.h:114
> +	   void draw(GraphicsContext*, ColorSpace styleColorSpace, const
FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1),
CompositeOperator = CompositeSourceOver, bool useLowQualityScale = false);

You can remove the styleColorSpace param name.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:240
> +	       // We're drawing into our own buffer, need to deep copy.

Why this comment here and not in draw() above? I'd prefer it in both.