Bug 163898 – [Win][Direct2D] Simplify Begin/EndDraw state maintenance based on GraphicsContext object lifetimes.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Tests (like MotionMark) that use CSS filters and other effects currently fail under Direct2D on Windows because Direct2D expects certain operations (such as clipping) to only occur during drawing calls (i.e., inside a "BeginDraw", "EndDraw" pair).

This patch does the following:
1. Uses a smart pointer for Direct2D native path types to reduce memory leaks.
2. Adds a "BeginDraw" call in the Filter code at the start of filter drawing.
3. Adds an "EndDraw" call in the Filter code when drawing is finished.
4. Adds a new "temporaryBeginDrawIfNeeded" method to enter a draw operation if needed to execute the drawing commands needed during a filter or canvas drawing operation.
5. Improve performance by creating a single GDI Interop object for the Direct2D device instead of creating one every time a GDI drawing operation is needed.

[Brent Fulgham]

Created attachment 292626 [details]
Patch

[Brent Fulgham]

Created attachment 292635 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:337:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 14 files


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

[Brent Fulgham]

Created attachment 292650 [details]
Patch v2

Revised patch to remove unrelated changes.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:337:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 10 files


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

[Simon Fraser (smfr)]

Comment on attachment 292650 [details]
Patch v2

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

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:288
> +    HRESULT hr = m_renderTarget->QueryInterface(__uuidof(ID2D1GdiInteropRenderTarget), (void**)&m_gdiRenderTarget);

Would be nice to have the (void**)&m_gdiRenderTarget wrapped in some kind of Adopt syntax.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:446
> +    --m_beginDrawCount;

You should assert that m_beginDrawCount doesn't underflow.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:943
> -    auto context = platformContext();
> +    bool temporaryDraw = temporaryBeginDrawIfNeeded();

Seems like you could have an RIAA class for this.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:445
> +    filter->inputContext()->endDraw();

Is this the same context as sourceGraphicsContext? It's not clear.

[Brent Fulgham]

Comment on attachment 292650 [details]
Patch v2

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

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:446
>> +    --m_beginDrawCount;
> 
> You should assert that m_beginDrawCount doesn't underflow.

I could make it a CheckedArithmetic object.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:943
>> +    bool temporaryDraw = temporaryBeginDrawIfNeeded();
> 
> Seems like you could have an RIAA class for this.

Good idea.

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:445
>> +    filter->inputContext()->endDraw();
> 
> Is this the same context as sourceGraphicsContext? It's not clear.

Yes. I could make a "sourceGraphicsContext" temporary to make this clearer.

[Alex Christensen]

Comment on attachment 292650 [details]
Patch v2

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

> Source/WebCore/platform/graphics/Path.h:206
> +#if !USE(DIRECT2D)

Could we switch these around and put #if USE(DIRECT2D) instead of its inverse?

[Said Abou-Hallawa]

Comment on attachment 292650 [details]
Patch v2

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

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:448
> +    if (!m_beginDrawCount)
> +        m_didBeginDraw = false;

What is the point of having m_didBeginDraw and m_beginDrawCount? This function can replace m_didBeginDraw:

inline bool GraphicsContextPlatformPrivate::didBeginDraw() { return m_beginDrawCount; }

>>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:943
>>> +    bool temporaryDraw = temporaryBeginDrawIfNeeded();
>> 
>> Seems like you could have an RIAA class for this.
> 
> Good idea.

What is the purpose of temporaryBeginDraw()? Why do we need to beginDraw() here instead of doing it in the caller side? drawWithShadow() does not beginDraw(), so why drawWithoutShadow() is different from drawWithShadow() in this regard?

[Brent Fulgham]

Comment on attachment 292650 [details]
Patch v2

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

>> Source/WebCore/platform/graphics/Path.h:206
>> +#if !USE(DIRECT2D)
> 
> Could we switch these around and put #if USE(DIRECT2D) instead of its inverse?

Sure!

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:448
>> +        m_didBeginDraw = false;
> 
> What is the point of having m_didBeginDraw and m_beginDrawCount? This function can replace m_didBeginDraw:
> 
> inline bool GraphicsContextPlatformPrivate::didBeginDraw() { return m_beginDrawCount; }

Good point! I'll switch.

>>>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:943
>>>> +    bool temporaryDraw = temporaryBeginDrawIfNeeded();
>>> 
>>> Seems like you could have an RIAA class for this.
>> 
>> Good idea.
> 
> What is the purpose of temporaryBeginDraw()? Why do we need to beginDraw() here instead of doing it in the caller side? drawWithShadow() does not beginDraw(), so why drawWithoutShadow() is different from drawWithShadow() in this regard?

In many cases, we can call 'beginDraw' on the caller side. However, I have found that the code path for Canvas drawing often consists of a set of disparate draw events (e.g., clip a region, clear a rect, draw an arc, fill a path) that need to be done within a begin/end draw pair. It would be invasive and ugly looking to add a bunch of explicit begin/end draw pairs around all of these use cases.

Instead, the 'temporaryBeginDraw' checks if we are already in a drawing operation, and calls 'beginDraw' if we are not. In the case where we are not already drawing, we want to end the drawing operation here so that we are in the same state when we return from the current method.

I think switching to Simon's RIAA class will help clean some of this stuff up.

[Brent Fulgham]

Created attachment 292806 [details]
Patch v3

Revised to incorporate Simon, Alex, and Said's comments.

[Brent Fulgham]

Created attachment 292809 [details]
Patch v4

Revised to address all comments.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:343:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 12 files


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

[Simon Fraser (smfr)]

Comment on attachment 292809 [details]
Patch v4

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

> Source/WebCore/platform/graphics/filters/SourceGraphic.cpp:62
> +#if USE(DIRECT2D)
> +    resultImage->context().temporaryBeginDrawIfNeeded();
> +#endif

How does someone writing cross-platform code know that they have to do this?

[Said Abou-Hallawa]

Comment on attachment 292809 [details]
Patch v4

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

> Source/WebCore/platform/graphics/Path.h:211
>  #if USE(DIRECT2D)
> +        COMPtr<ID2D1GeometryGroup> m_path;
>          COMPtr<ID2D1PathGeometry> m_activePathGeometry;
>          COMPtr<ID2D1GeometrySink> m_activePath;
> +#else
> +        PlatformPathPtr m_path { nullptr };

Why don't we define PlatformPathPtr for DIRECT2D to be:

typedef COMPtr<PlatformPath> PlatformPathPtr;

> Source/WebCore/platform/graphics/filters/SourceGraphic.cpp:68
> +#if USE(DIRECT2D)
> +    resultImage->context().temporaryBeginDrawIfNeeded();
> +#endif
> +
>      resultImage->context().drawImageBuffer(*sourceImage, IntPoint());
> +
> +#if USE(DIRECT2D)
> +    resultImage->context().endDraw();
> +#endif

This code can be replaced by creating an object of type RenderTargetHelper.

#if USE(DIRECT2D)
   RenderTargetHelper helper(resultImage->context());
#endif

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:123
> -    if (!context->didBeginDraw())
> -        d2dContext->BeginDraw();
> +    bool temporaryDraw = context->temporaryBeginDrawIfNeeded();

Ditto.

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:131
> +    if (temporaryDraw)
> +        context->endDraw();

Ditto.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:118
> +    HBITMAP bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));

What exactly are we getting from GetCurrentObject()? Is there a bitmap already selected in the cdc? Do we need check whether the returned bitmap is NULL?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:120
> +    DIBPixelData pixelData(bitmap);

Do we need to check whether the pixelData.size().empty()?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:134
> +

I do not think this new code needs to be part of this function. All it does is creating a render target form an HDC. The rest of the code code is very similar to GraphicsContext::platformInit(ID2D1RenderTarget* renderTarget).

void GraphicsContext::platformInit(HDC hdc, bool hasAlpha)
{
    ID2D1RenderTarget* renderTarget = renderTragetFromHdc(hdc, hasAlpaha);
    platformInit(renderTarget);
}

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:289
> +    HRESULT hr = m_renderTarget->QueryInterface(__uuidof(ID2D1GdiInteropRenderTarget), reinterpret_cast<void**>(&m_gdiRenderTarget));

Maybe it is a naive question. Is working with the GDI Interop mode temporary? Is there a plan to switch all the drawing function to use the native render target APIs?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:336
> +HDC GraphicsContextPlatformPrivate::hdcForRenderTarget()

Can't we have a member called m_hdc? Can't we call m_hdc = GetDC() in the constructor of the GraphicsContextPlatformPrivate? Is there any chance the hdc of the render target to change after constructing this object?

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

Ditto. Instead of having to call hdcForRenderTarget() and releaseHdcForRenderTarget(), the caller can use a member called hdc() of this class.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:435
> +    if (didBeginDraw())
> +        return;

I think this should be an ASSERT(!didBeginDraw()).

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:439
> +    ++m_beginDrawCount;

What is the point of having a m_beginDrawCount be counter even though its value will be either 0 or 1? If we allow nested begin-end drawing then it makes sense to have it a counter. But this function disallows nesting the begin-end drawing.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:444
>      ASSERT(m_renderTarget.get());

Do we need to ASSERT(didBeginDraw());

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:554
> +    if (m_data->didBeginDraw())
> +        return false;
> +
> +    m_data->beginDraw();
> +    return true;

I think all the code should be using RenderTargetHelper. And I think the logic, of having to call beginDraw() or not, should be moved to the constructor of RenderTargetHelper.

> Source/WebCore/platform/graphics/win/NativeImageDirect2D.cpp:-89
> -    if (!context.didBeginDraw())
> -        platformContext->BeginDraw();

This can be replaced by creating an object of class RenderTargetHelper.

> Source/WebCore/platform/graphics/win/NativeImageDirect2D.cpp:95
> +    if (temporaryDraw)
> +        context.endDraw();

Ditto

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:232
> +    applier->strokeStyle(&scratchContext);

Since scratchRenderTarget() is a static variable and since we draw on it to get the geometry of s shape, do not we need to clear it before drawing on it?

> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:32
> +class RenderTargetHelper {

I think the name is a little bit vague. I would recommend something like RenderTargetScopedDrawing.

> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:36
> +        : m_context(context)

A flag named 'beginAndEnd' should be added to control whether beginDraw() should be called in the constructor or should be delayed; something similar to what we do in GraphicsContextStateSaver.

> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:38
> +        m_drawIsScoped = context.temporaryBeginDrawIfNeeded();

Do we need to check if (!context.didBeginDraw()) before calling beginDraw()?

> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:46
> +    void endDraw()

We should expose beginDraw() also if we add the 'beginAndEnd' flag.

> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:49
> +            m_context.endDraw();

We should set m_drawIsScoped to false.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:420
> +#if USE(DIRECT2D)
> +    sourceGraphicsContext->temporaryBeginDrawIfNeeded();
> +#endif

temporaryBeginDrawIfNeeded() is called here and endDraw() is called in applyFilterEffect().

I would suggest using RenderTargetHelper in the caller of both beginFilterEffect() and applyFilterEffect().

[Said Abou-Hallawa]

Comment on attachment 292809 [details]
Patch v4

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

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:134
>> +
> 
> I do not think this new code needs to be part of this function. All it does is creating a render target form an HDC. The rest of the code code is very similar to GraphicsContext::platformInit(ID2D1RenderTarget* renderTarget).
> 
> void GraphicsContext::platformInit(HDC hdc, bool hasAlpha)
> {
>     ID2D1RenderTarget* renderTarget = renderTragetFromHdc(hdc, hasAlpaha);
>     platformInit(renderTarget);
> }

I meant: I do not think this new code DOES NOT need to be...

[Brent Fulgham]

Comment on attachment 292809 [details]
Patch v4

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

>> Source/WebCore/platform/graphics/Path.h:211
>> +        PlatformPathPtr m_path { nullptr };
> 
> Why don't we define PlatformPathPtr for DIRECT2D to be:
> 
> typedef COMPtr<PlatformPath> PlatformPathPtr;

The problem is PlatformPathPtr is also a return type, which is expected to be a bare pointer by many callers.

>> Source/WebCore/platform/graphics/filters/SourceGraphic.cpp:68
>> +#endif
> 
> This code can be replaced by creating an object of type RenderTargetHelper.
> 
> #if USE(DIRECT2D)
>    RenderTargetHelper helper(resultImage->context());
> #endif

Will do -- but I'm reviewing whether I should make this call here and other places. Maybe I can avoid it somehow.

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:123
>> +    bool temporaryDraw = context->temporaryBeginDrawIfNeeded();
> 
> Ditto.

I spun this off into a separate patch (Bug 164005). Maybe you could review that one, too? :-)

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:118
>> +    HBITMAP bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));
> 
> What exactly are we getting from GetCurrentObject()? Is there a bitmap already selected in the cdc? Do we need check whether the returned bitmap is NULL?

Maybe this is wrong. I followed the logic in the path from HDC -> CoreGraphics context. But D2D might be able to just run right from the HDC directly. I'll revise this.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:120
>> +    DIBPixelData pixelData(bitmap);
> 
> Do we need to check whether the pixelData.size().empty()?

We don't elsewhere, but it does seem like a reasonable precaution.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:289
>> +    HRESULT hr = m_renderTarget->QueryInterface(__uuidof(ID2D1GdiInteropRenderTarget), reinterpret_cast<void**>(&m_gdiRenderTarget));
> 
> Maybe it is a naive question. Is working with the GDI Interop mode temporary? Is there a plan to switch all the drawing function to use the native render target APIs?

I am hoping it is temporary, but there don't seem to be native D2D routines for drawing scrollbars, drop-downs, etc. So we might be stuck with this for the foreseeable future.

I have a separate patch that creates and caches the GDI Interop Render Target for the lifetime of the renderer, which should make things more efficient.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:336
>> +HDC GraphicsContextPlatformPrivate::hdcForRenderTarget()
> 
> Can't we have a member called m_hdc? Can't we call m_hdc = GetDC() in the constructor of the GraphicsContextPlatformPrivate? Is there any chance the hdc of the render target to change after constructing this object?

I tried that first, and learned that the HDC exposed by Direct2D only lives between the Begin/End draw operations. So unfortunately I can't cache it.

Going the other direction (starting from a GDI HDC and creating a render target from that) does work, but only because Direct2D seems to be copying the contents of the HDC into separate memory (and back and forth).

All of this is inefficient, so we want to get rid of as much HDC code as possible, as soon as we determine all of this is working.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:439
>> +    ++m_beginDrawCount;
> 
> What is the point of having a m_beginDrawCount be counter even though its value will be either 0 or 1? If we allow nested begin-end drawing then it makes sense to have it a counter. But this function disallows nesting the begin-end drawing.

Begin-end calls are allowed to be nested, so I wanted to leave that option available for later if I need it.

If we don't, I can alway switch back to the boolean version.

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:554
>> +    return true;
> 
> I think all the code should be using RenderTargetHelper. And I think the logic, of having to call beginDraw() or not, should be moved to the constructor of RenderTargetHelper.

Agreed!

>> Source/WebCore/platform/graphics/win/NativeImageDirect2D.cpp:-89
>> -        platformContext->BeginDraw();
> 
> This can be replaced by creating an object of class RenderTargetHelper.

Will do.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:232
>> +    applier->strokeStyle(&scratchContext);
> 
> Since scratchRenderTarget() is a static variable and since we draw on it to get the geometry of s shape, do not we need to clear it before drawing on it?

Oh! Good point. I didn't notice if we did this in CoreGraphics. I'll double check and fix that if necessary.

>> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:32
>> +class RenderTargetHelper {
> 
> I think the name is a little bit vague. I would recommend something like RenderTargetScopedDrawing.

Ok. I'll use that.

>> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:36
>> +        : m_context(context)
> 
> A flag named 'beginAndEnd' should be added to control whether beginDraw() should be called in the constructor or should be delayed; something similar to what we do in GraphicsContextStateSaver.

I don't want to make this change unless I find a case where we want to delay the 'beginDraw' operation.

I can't see any obvious times this would be needed, since it really just marks the render target as "in use"; the real interesting work happens when 'endDraw' actually gets called.

>> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:38
>> +        m_drawIsScoped = context.temporaryBeginDrawIfNeeded();
> 
> Do we need to check if (!context.didBeginDraw()) before calling beginDraw()?

No -- it check its internal state before calling beginDraw.

>> Source/WebCore/platform/graphics/win/RenderTargetHelper.h:49
>> +            m_context.endDraw();
> 
> We should set m_drawIsScoped to false.

Whoops!

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:420
>> +#endif
> 
> temporaryBeginDrawIfNeeded() is called here and endDraw() is called in applyFilterEffect().
> 
> I would suggest using RenderTargetHelper in the caller of both beginFilterEffect() and applyFilterEffect().

I agree.

[Brent Fulgham]

There is some additional work needed to get filters fully working, so I'm retitling this bug to reflect its changed scope (i.e., focusing on the begin/end drawing pair issue).

Much of the code in the original patch has been broken out into smaller patches (see Bug 163988, 163994, and 164005), so what remains is fairly small.

[Brent Fulgham]

Created attachment 293173 [details]
Rebaselined Patch

Revised patch based on the smaller work involved, and it's changed scope to simply dealing with Begin/End drawing pair bookkeeping.

[Brent Fulgham]

Comment on attachment 293173 [details]
Rebaselined Patch

I have a better idea.

[Brent Fulgham]

Created attachment 293175 [details]
Patch v5

Rather than maintaining two Booleans, use a single flag to represent both.

[Brent Fulgham]

Created attachment 293176 [details]
Patch v6

Correct a typo.

[Darin Adler]

Comment on attachment 293176 [details]
Patch v6

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

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:408
>      ASSERT(m_renderTarget.get());

ASSERT(reason != DrawReason::NotDrawing);

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:409
>      if (didBeginDraw())

It is really always OK to silently return here? In all four cases?

If m_beginDrawState is Clipping and reason is Normal?
If m_beginDrawState is Normal and reason is Clipping?
If m_beginDrawState is Clipping and reason is Clipping?
If m_beginDrawState is Normal and reason is Normal?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:426
> +    m_beginDrawState = DrawReason::NotDrawing;

Should this have asserted didBeginDraw() earlier in the function?

[Said Abou-Hallawa]

Comment on attachment 293176 [details]
Patch v6

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

> Source/WebCore/ChangeLog:10
> +        Direct2D does not permit nested Begin/End pairs, so we never want to issue a
> +        second BeginDraw on a context that has already entered this state. Therefore,
> +        I changed this to a flag, rather than a count.

Although Direct2D does not allow nested Begin/End drawing, you can allow it and have a counter to track that safely.

void GraphicsContextPlatformPrivate::beginDraw()
{
    if (!m_beginDrawCount++)
       m_renderTarget.BeginDraw();
}

void GraphicsContextPlatformPrivate::endDraw()
{
    ASSERT(m_beginDrawCount > 0);
    if (!--m_beginDrawCount)
       m_renderTarget.EndDraw();
}
 
The benefit of that, is you can call the begin/end pair whenever you want without having to check whether if beginDraw() is called previously or not. The only thing you have to do it, is to match the begin/end pairs. And you are sure that the the caller at the highest level will be the one which actually does begin/end the drawing.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:312
> +    if (m_beginDrawState == DrawReason::Clipping)
> +        endDraw();

This statement does not seem to be right. Why do we flush the drawing at the destructor of this object? This means we beginDraw() needlessly. Actually we should assert here that we are not in the middle of begin/draw pairs.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:351
> -    beginDrawIfNeeded();
> +    beginDrawIfNeeded(DrawReason::Clipping);

Who is going to endDraw() if beginDrawIfNeeded() calls beginDraw() for the first time? What is the guarantee that this is going to happen? This function is at a very low level such that it should not beginDraw() itself.

I would suggest the following if you need to apply the clipping outside the drawing.

1. In GraphicsContextPlatformPrivate::clip() if we are in the middle of the drawing,the clipping is pushed to the render target. Otherwise it is pushed to m_pendingClippingLayers vector.
2. In beginDraw(), you push all the m_pendingClippingLayers to the render target and clear m_pendingClippingLayers.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:369
> -    beginDrawIfNeeded();
> +    beginDrawIfNeeded(DrawReason::Clipping);

Ditto.

> Source/WebCore/platform/graphics/win/GraphicsContextPlatformPrivateDirect2D.h:51
> +    enum DrawReason { NotDrawing, Normal, Clipping };

Is this enum a reason or a state?

[Brent Fulgham]

Simon pointed out that GraphicsContext objects are generally only created at the time we intend to do drawing, so calling 'BeginDraw' as part of object construction, and 'EndDraw' at time of destruction should be sufficient.

As usual, he was correct.
        
This patch gets rid of unneeded code and greatly simplifies the Direct2D drawing path.

[Brent Fulgham]

Created attachment 293223 [details]
Patch v7

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:72:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 7 files


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

[WebKit Commit Bot]

Comment on attachment 293223 [details]
Patch v7

Rejecting attachment 293223 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 293223, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
file Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/NativeImageDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/RenderTargetScopedDrawing.h
rm 'Source/WebCore/platform/graphics/win/RenderTargetScopedDrawing.h'
patching file Source/WebCore/svg/graphics/SVGImage.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2397186

[Brent Fulgham]

Committed in r208131 <https://trac.webkit.org/changeset/208131>.