Bug 163988

Summary: [Win][Direct2D] Implement GraphicsContext::releaseWindowsContext.
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebCore Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 163898    
Attachments:
Description Flags
Patch
none
Patch bfulgham: review+, bfulgham: commit-queue-

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.