Bug 163988 – [Win][Direct2D] Implement GraphicsContext::releaseWindowsContext.

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

[Per Arne Vollan]

This is needed to draw native controls.

[Per Arne Vollan]

Created attachment 292828 [details]
Patch

[WebKit Commit Bot]

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


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


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

[Brent Fulgham]

Comment on attachment 292828 [details]
Patch

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

Looks good! After this lands I'll incorporate into my patch.

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

I have a patch to reuse the GDI Interop target. I'll update it to include this code path.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:252
> +        platformContext()->BeginDraw();

I also have a patch with an RIAA class that will get rid of some of this boilerplate.

[Per Arne Vollan]

Created attachment 293049 [details]
Patch

[Brent Fulgham]

Comment on attachment 293049 [details]
Patch

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

Very nice! This should get rid of at least one expensive copy!

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:252
> +    D2D1_BITMAP_PROPERTIES bitmapProperties = D2D1::BitmapProperties(D2D1::PixelFormat(DXGI_FORMAT_B8G8R8A8_UNORM, D2D1_ALPHA_MODE_IGNORE));

Use 'auto' here, please.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:255
> +    HRESULT hr = platformContext()->CreateBitmap(pixelData.size(), pixelData.buffer(), pixelData.bytesPerRow(), &bitmapProperties, &bitmap);

We should probably ASSERT(SUCCEEDED(hr)) here.

[Per Arne Vollan]

Committed r208013: <https://trac.webkit.org/changeset/208013>

[Said Abou-Hallawa]

Comment on attachment 293049 [details]
Patch

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

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:263
> +    if (!didBeginDraw())
> +        hr = platformContext()->EndDraw();

I think this condition is wrong. It can be the opposite:

if (didBeginDraw())
    hr = platformContext()->EndDraw();

But I think this is not accurate either. Since you need to endDraw() only if you beginDraw(), I think you need to track this by flag in the beginDraw if-statetmen abovet. Otherwise you may cause unbalanced draw begin-end nesting.

[Brent Fulgham]

Comment on attachment 293049 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:263
>> +        hr = platformContext()->EndDraw();
> 
> I think this condition is wrong. It can be the opposite:
> 
> if (didBeginDraw())
>     hr = platformContext()->EndDraw();
> 
> But I think this is not accurate either. Since you need to endDraw() only if you beginDraw(), I think you need to track this by flag in the beginDraw if-statetmen abovet. Otherwise you may cause unbalanced draw begin-end nesting.

This whole issue will be fixed by Bug 164005.