Bug 164005 – [Win][Direct2D] Create a RAII Helper Class for the Render Target

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

Create a convenience class to handle the logic of beginning and ending D2D draw operations when needed.

[Brent Fulgham]

Created attachment 292876 [details]
Patch

[WebKit Commit Bot]

Attachment 292876 [details] did not pass style-queue:


ERROR: Source/WebKit/win/WebView.cpp:1258:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 292877 [details]
Patch v2

[WebKit Commit Bot]

Attachment 292877 [details] did not pass style-queue:


ERROR: Source/WebKit/win/WebView.cpp:1258:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 292878 [details]
Patch v3

Resolve style warning.

[WebKit Commit Bot]

Attachment 292878 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/RenderTargetScopedDrawing.h:48:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 292878 [details]
Patch v3

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

> Source/cmake/OptionsAppleWin.cmake:9
> +set(USE_DIRECT2D 1)

Whoops! We don't want to check this in. (Yet).

[Brent Fulgham]

Created attachment 293058 [details]
Patch v4

Correct build issues when building without D2D.

[WebKit Commit Bot]

Attachment 293058 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/RenderTargetScopedDrawing.h:48:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 293058 [details]
Patch v4

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

> Source/cmake/OptionsAppleWin.cmake:9
> -# set(USE_DIRECT2D 1)
> +set(USE_DIRECT2D 1)

Did you mean to include this change?

[Brent Fulgham]

Created attachment 293072 [details]
Patch v5

Rebased for r208013, and to avoid turning the Direct2D build on by default.

[WebKit Commit Bot]

Attachment 293072 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/RenderTargetScopedDrawing.h:48:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Comment on attachment 293072 [details]
Patch v5

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

Unofficial r=me.

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:124
> +    RenderTargetScopedDrawing helper(*context);

I am not sure if 'helper' is a good name of a variable of type RenderTargetScopedDrawing.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:348
> +    if (!didBeginDraw())

I think you do not need this if-statement because beginDraw() checks for the same condition.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:367
> +    if (!didBeginDraw())

Ditto.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:405
> +void GraphicsContextPlatformPrivate::beginDraw()

To be consistent, I think this should be named beginDrawIfNeeded() since it has the same logic as GraphicsContext::beginDrawIfNeeded().

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:86
> +    helper.endDraw();
> +

Why do you need to explicitly call endDraw() here?  It is going to be called form the destructor. If you need to call it before the return statement below you could surround the above code by crawly braces:

{
    RenderTargetScopedDrawing helper(*context);
    ....
}

....

return result;

[Brent Fulgham]

Comment on attachment 293072 [details]
Patch v5

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

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:124
>> +    RenderTargetScopedDrawing helper(*context);
> 
> I am not sure if 'helper' is a good name of a variable of type RenderTargetScopedDrawing.

Oh yes! I will fix this.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:348
>> +    if (!didBeginDraw())
> 
> I think you do not need this if-statement because beginDraw() checks for the same condition.

Good point. I'll fix it.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:367
>> +    if (!didBeginDraw())
> 
> Ditto.

Will do.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:405
>> +void GraphicsContextPlatformPrivate::beginDraw()
> 
> To be consistent, I think this should be named beginDrawIfNeeded() since it has the same logic as GraphicsContext::beginDrawIfNeeded().

Good idea!

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:86
>> +
> 
> Why do you need to explicitly call endDraw() here?  It is going to be called form the destructor. If you need to call it before the return statement below you could surround the above code by crawly braces:
> 
> {
>     RenderTargetScopedDrawing helper(*context);
>     ....
> }
> 
> ....
> 
> return result;

Will do.

> Source/WebKit/win/WebView.cpp:145
> +#include <WebCore/RenderTargetScopedDrawing.h>

This broke the build in EWS because it should only be included for DIRECT2D builds. I will fix when landing.

[Brent Fulgham]

Committed in r208019 <https://trac.webkit.org/changeset/208019>.