Bug 202177 – [FTW] Correct compositing, shadow, and radial gradient implementations

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch corrects a number of errors in the implementations of basic Canvas drawing routines.

[Brent Fulgham]

Created attachment 379510 [details]
Patch

[Brent Fulgham]

Prior to this change:

    629 / 775 Pass

With this change:

    665 / 775 Pass

[Brent Fulgham]

Created attachment 379693 [details]
Patch

[Fujii Hironori]

Comment on attachment 379693 [details]
Patch

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

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:41
> +#include "FloatSize.h"

You don't need to include FloatSize.h because FloatRect.h includes it indirectly.

> Source/WebCore/platform/graphics/win/Direct2DUtilities.cpp:97
> +    constexpr double dpiBase = 96.0;

96.0 → 96
https://webkit.org/code-style-guidelines/#float-suffixes

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:346
>  FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) const

How will you implement Path::strokeBoundingRect which is taking StrokeStyleApplier as the argument?

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:57
> +    RELEASE_ASSERT(SUCCEEDED(hr));

Let's use COMPtr method.

m_deviceContext.query(m_renderTarget.get());
RELEASE_ASSERT(!!m_deviceContext);

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:385
> +    compositeIfNeeded();

You don't need to call compositeIfNeeded here because endDraw is called above.

[Fujii Hironori]

Comment on attachment 379693 [details]
Patch

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

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:-237
> -    platformContext.setCompositeMode(targetCompositeMode);

Remove PlatformContextDirect2D::setBlendMode and PlatformContextDirect2D::setCompositeMode which are unused anymore.

[Fujii Hironori]

Comment on attachment 379693 [details]
Patch

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

> Source/WebCore/platform/graphics/win/Direct2DOperations.h:136
> +void drawWithoutShadow(PlatformContextDirect2D&, const FloatRect& ignored, const WTF::Function<void(ID2D1RenderTarget*)>& drawCommands);

I think boundingRect should be removed from drawWithoutShadow's argument. 

There are a lot of code looks like the following:

>  if (shadowState.isVisible())
>      drawWithShadow(platformContext, rect, shadowState, drawFunction);
>  else
>      drawWithoutShadow(platformContext, rect, drawFunction);

This condition should be moved into drawWithShadow.

[Brent Fulgham]

Comment on attachment 379693 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:41
>> +#include "FloatSize.h"
> 
> You don't need to include FloatSize.h because FloatRect.h includes it indirectly.

Fixed.

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:-237
>> -    platformContext.setCompositeMode(targetCompositeMode);
> 
> Remove PlatformContextDirect2D::setBlendMode and PlatformContextDirect2D::setCompositeMode which are unused anymore.

Done.

>> Source/WebCore/platform/graphics/win/Direct2DUtilities.cpp:97
>> +    constexpr double dpiBase = 96.0;
> 
> 96.0 → 96
> https://webkit.org/code-style-guidelines/#float-suffixes

Will do!

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:346
>>  FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) const
> 
> How will you implement Path::strokeBoundingRect which is taking StrokeStyleApplier as the argument?

Oh gosh -- I didn't even look at this one. Can I do that as a separate patch?

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:57
>> +    RELEASE_ASSERT(SUCCEEDED(hr));
> 
> Let's use COMPtr method.
> 
> m_deviceContext.query(m_renderTarget.get());
> RELEASE_ASSERT(!!m_deviceContext);

Oh! That's much better. I'll do so here and the 'setRenderTarget' method, too.

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:385
>> +    compositeIfNeeded();
> 
> You don't need to call compositeIfNeeded here because endDraw is called above.

Good point!

[Brent Fulgham]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 379693 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379693&action=review
> 
> > Source/WebCore/platform/graphics/win/Direct2DOperations.h:136
> > +void drawWithoutShadow(PlatformContextDirect2D&, const FloatRect& ignored, const WTF::Function<void(ID2D1RenderTarget*)>& drawCommands);
> 
> I think boundingRect should be removed from drawWithoutShadow's argument. 

Okay.

> There are a lot of code looks like the following:
> 
> >  if (shadowState.isVisible())
> >      drawWithShadow(platformContext, rect, shadowState, drawFunction);
> >  else
> >      drawWithoutShadow(platformContext, rect, drawFunction);
> 
> This condition should be moved into drawWithShadow.

Sounds good.

[Fujii Hironori]

Comment on attachment 379693 [details]
Patch

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

>>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:346
>>>  FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) const
>> 
>> How will you implement Path::strokeBoundingRect which is taking StrokeStyleApplier as the argument?
> 
> Oh gosh -- I didn't even look at this one. Can I do that as a separate patch?

Yup.

[Brent Fulgham]

Created attachment 379804 [details]
Patch

[Brent Fulgham]

Created attachment 379805 [details]
Patch

[Brent Fulgham]

With this change:

    681 / 775 Pass

[Fujii Hironori]

Comment on attachment 379805 [details]
Patch

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

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:562
> +    auto rect = path.fastBoundingRectForStroke(platformContext);

This rect is a unused variable.

> Source/WebCore/platform/graphics/win/Direct2DUtilities.cpp:494
> +    Direct2D::writeDiagnosticPNGToPath(bitmapTarget, currentCanvas.get(), L"C:/Projects/WebKit/data/currentCanvas.png");

I think this debugging code should be removed.

[Fujii Hironori]

Comment on attachment 379805 [details]
Patch

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

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:349
> +    Direct2D::writeDiagnosticPNGToPath(bitmapTarget.get(), bitmap.get(), L"C:/Projects/WebKit/data/after.png");

Ditto.

[Fujii Hironori]

Comment on attachment 379805 [details]
Patch

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

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:335
> +    float tolerance = 10.0;

https://webkit.org/code-style-guidelines/#float-suffixes

[Brent Fulgham]

Comment on attachment 379805 [details]
Patch

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

Thank you for reviewing -- these reviews have been very helpful to me.

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:562
>> +    auto rect = path.fastBoundingRectForStroke(platformContext);
> 
> This rect is a unused variable.

Great catch! That's a potentially expensive operation, so good to avoid if possible.

>> Source/WebCore/platform/graphics/win/Direct2DUtilities.cpp:494
>> +    Direct2D::writeDiagnosticPNGToPath(bitmapTarget, currentCanvas.get(), L"C:/Projects/WebKit/data/currentCanvas.png");
> 
> I think this debugging code should be removed.

Oh no! How stupid of me. I made this same mistake in several places. I forgot to search and remove before uploading the patch. :-(

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:335
>> +    float tolerance = 10.0;
> 
> https://webkit.org/code-style-guidelines/#float-suffixes

Fixed!

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:349
>> +    Direct2D::writeDiagnosticPNGToPath(bitmapTarget.get(), bitmap.get(), L"C:/Projects/WebKit/data/after.png");
> 
> Ditto.

:-(

[Brent Fulgham]

Committed r250492: <https://trac.webkit.org/changeset/250492>

[Radar WebKit Bug Importer]

<rdar://problem/55822137>