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
Per Arne Vollan
2016-10-25 14:44:20 PDT
Created attachment 292828 [details]
Patch
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 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. Created attachment 293049 [details]
Patch
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. Committed r208013: <https://trac.webkit.org/changeset/208013> 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 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. |