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
Brent Fulgham
2016-10-25 22:33:11 PDT
Created attachment 292876 [details]
Patch
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.
Created attachment 292877 [details]
Patch v2
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.
Created attachment 292878 [details]
Patch v3
Resolve style warning.
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 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). Created attachment 293058 [details]
Patch v4
Correct build issues when building without D2D.
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 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? Created attachment 293072 [details] Patch v5 Rebased for r208013, and to avoid turning the Direct2D build on by default. 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 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 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. Committed in r208019 <https://trac.webkit.org/changeset/208019>. |