[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