Bug 164005

Summary: [Win][Direct2D] Create a RAII Helper Class for the Render Target
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, dino, pvollan, sabouhallawa, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 161817, 163898    
Attachments:
Description Flags
Patch
none
Patch v2
none
Patch v3
bfulgham: commit-queue-
Patch v4
none
Patch v5 achristensen: review+, achristensen: commit-queue-

Description Brent Fulgham 2016-10-25 22:33:11 PDT
Create a convenience class to handle the logic of beginning and ending D2D draw operations when needed.
Comment 1 Brent Fulgham 2016-10-25 22:43:21 PDT
Created attachment 292876 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-25 22:45:52 PDT
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.
Comment 3 Brent Fulgham 2016-10-25 23:10:47 PDT
Created attachment 292877 [details]
Patch v2
Comment 4 WebKit Commit Bot 2016-10-25 23:13:10 PDT
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.
Comment 5 Brent Fulgham 2016-10-25 23:17:41 PDT
Created attachment 292878 [details]
Patch v3

Resolve style warning.
Comment 6 WebKit Commit Bot 2016-10-25 23:20:03 PDT
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.
Comment 7 Brent Fulgham 2016-10-25 23:20:36 PDT
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).
Comment 8 Brent Fulgham 2016-10-27 14:33:35 PDT
Created attachment 293058 [details]
Patch v4

Correct build issues when building without D2D.
Comment 9 WebKit Commit Bot 2016-10-27 14:35:55 PDT
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.
Comment 10 Alex Christensen 2016-10-27 15:25:48 PDT
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?
Comment 11 Brent Fulgham 2016-10-27 15:54:43 PDT
Created attachment 293072 [details]
Patch v5

Rebased for r208013, and to avoid turning the Direct2D build on by default.
Comment 12 WebKit Commit Bot 2016-10-27 15:56:22 PDT
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.
Comment 13 Said Abou-Hallawa 2016-10-27 16:08:45 PDT
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;
Comment 14 Brent Fulgham 2016-10-27 16:59:01 PDT
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.
Comment 15 Brent Fulgham 2016-10-27 17:42:20 PDT
Committed in r208019 <https://trac.webkit.org/changeset/208019>.