[webkit-reviews] review granted: [Bug 32504] Provide Example Printing Implementation : [Attachment 44846] Revised patch to clean up check-webkit-style warnings.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 18 13:09:41 PST 2009
Adam Roben (aroben) <aroben at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 32504: Provide Example Printing Implementation
https://bugs.webkit.org/show_bug.cgi?id=32504
Attachment 44846: Revised patch to clean up check-webkit-style warnings.
https://bugs.webkit.org/attachment.cgi?id=44846&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +HRESULT STDMETHODCALLTYPE PrintWebUIDelegate::QueryInterface(REFIID riid,
void** ppvObject)
There's no need for STDMETHODCALLTYPE on any function defintions in this file.
Having it on the declaration is all that matters.
> +{
> + *ppvObject = 0;
> + if (IsEqualGUID(riid, IID_IUnknown))
> + *ppvObject = static_cast<IWebUIDelegate*>(this);
> + else if (IsEqualGUID(riid, IID_IWebUIDelegate))
> + *ppvObject = static_cast<IWebUIDelegate*>(this);
IsEqualIID would be a little better.
> +ULONG STDMETHODCALLTYPE PrintWebUIDelegate::Release(void)
> +{
> + ULONG newRef = --m_refCount;
> + if (!newRef)
> + delete(this);
No need for the parentheses around "this".
> + IWebFramePrivate* privateFrame = 0;
> + if (FAILED(mainFrame->QueryInterface(IID_IWebFramePrivate,
(void**)&privateFrame))) {
> + mainFrame->Release();
> + return E_FAIL;
> + }
You should be able to use the single-parameter version of QueryInterface here,
which also removes the need for the cast. You can also unconditionally release
mainFrame right after calling QueryInterface:
IWebFramePrivate* privateFrame = 0;
HRESULT hr = mainFrame->QueryInterface(&privateFrame);
mainFrame->Release();
if (FAILED(hr))
return E_FAIL;
> + rect->left += 20;
> + rect->top += 20;
> + rect->right -= 20;
> + rect->bottom -= 20;
Can we put this 20 in a constant?
> + gPrintDelegate = new PrintWebUIDelegate();
No need for parentheses here.
> +BOOL CALLBACK AbortProc(HDC hDC, int Error)
Can this be marked static?
> +{
> + MSG msg;
> + while (::PeekMessage(&msg, 0, 0, 0, PM_REMOVE)) {
Why not use GetMessage? Do you need to check for a -1 return value?
> + IWebFramePrivate* framePrivate = 0;
> + if (FAILED (frame->QueryInterface(&framePrivate)))
> + goto exit;
Extra space after FAILED. You can release frame uncoditionally after calling
QueryInterface. Then you won't need the goto.
> + void* graphicsContext = 0;
> + for (size_t page = 0; page < pageCount; ++page) {
> + ::StartPage(printDC);
> + framePrivate->spoolPages(printDC, page, page, graphicsContext);
> + ::EndPage(printDC);
> + }
This won't work in CG builds. It would be nice to detect that we're in a CG
build and show a message to the user.
r=me
More information about the webkit-reviews
mailing list