Bug 163988 - [Win][Direct2D] Implement GraphicsContext::releaseWindowsContext.
Summary: [Win][Direct2D] Implement GraphicsContext::releaseWindowsContext.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords:
Depends on:
Blocks: 163898
  Show dependency treegraph
 
Reported: 2016-10-25 14:44 PDT by Per Arne Vollan
Modified: 2016-10-27 15:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.39 KB, patch)
2016-10-25 14:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2016-10-27 13:31 PDT, Per Arne Vollan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2016-10-25 14:44:20 PDT
This is needed to draw native controls.
Comment 1 Per Arne Vollan 2016-10-25 14:53:34 PDT
Created attachment 292828 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-25 14:55:36 PDT
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.
Comment 3 Brent Fulgham 2016-10-25 22:07:58 PDT
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.
Comment 4 Per Arne Vollan 2016-10-27 13:31:51 PDT
Created attachment 293049 [details]
Patch
Comment 5 Brent Fulgham 2016-10-27 15:07:12 PDT
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.
Comment 6 Per Arne Vollan 2016-10-27 15:42:04 PDT
Committed r208013: <https://trac.webkit.org/changeset/208013>
Comment 7 Said Abou-Hallawa 2016-10-27 15:49:31 PDT
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.
Comment 8 Brent Fulgham 2016-10-27 15:51:08 PDT
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.