[webkit-reviews] review denied: [Bug 57409] [WinCairo] Implement Missing drawWindowsBitmap : [Attachment 87775] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 31 14:02:59 PDT 2011
Adam Roben (:aroben) <aroben at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 57409: [WinCairo] Implement Missing drawWindowsBitmap
https://bugs.webkit.org/show_bug.cgi?id=57409
Attachment 87775: Patch
https://bugs.webkit.org/attachment.cgi?id=87775&action=review
------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87775&action=review
It seems buggy that releaseWindowsContext doesn't select the HDC's original
bitmap back into it before calling ::DeleteDC. That's not a new problem,
though.
This is looking good. Let's do one more pass to nail down the last few things.
> Source/WebCore/platform/graphics/GraphicsContext.h:447
> +
I don't think this extra line is needed.
> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:101
> + OwnPtr<HBITMAP> bitmap(static_cast<HBITMAP>(GetCurrentObject(hdc,
OBJ_BITMAP)));
The more modern way of writing this is to use assignment and adoptPtr.
> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:99
> +static void drawBitmapToContext(GraphicsContextPlatformPrivate* context,
const WindowsDIB& windowsDIB, double tx, double ty)
tx/ty would be better as an IntSize or FloatSize. (I think only ints are passed
in?)
> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:137
> + OwnPtr<HBITMAP> bitmap(static_cast<HBITMAP>(GetCurrentObject(hdc,
OBJ_BITMAP)));
Same comment here about adoptPtr.
> Source/WebCore/platform/graphics/win/WindowsDIB.h:39
> +class WindowsDIB {
Since this class doesn't hold onto the DIB itself, maybe a name like DIBData or
DIBPixelData would be better.
> Source/WebCore/platform/graphics/win/WindowsDIB.h:48
> + WindowsDIB(HBITMAP);
This should be marked explicit.
> Source/WebCore/platform/graphics/win/WindowsDIB.h:50
> + void initializeFromHBitmap(const HBITMAP&);
No need for the const&. I think "FromHBitmap" could be removed, too.
More information about the webkit-reviews
mailing list