Bug 32504 - Provide Example Printing Implementation
Summary: Provide Example Printing Implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-13 22:52 PST by Brent Fulgham
Modified: 2009-12-19 23:01 PST (History)
4 users (show)

See Also:


Attachments
Add an example printing implementation the WinLauncher example program. (29.47 KB, patch)
2009-12-13 23:28 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Extend WinLauncher with printing support. (24.75 KB, patch)
2009-12-14 21:54 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised patch to clean up check-webkit-style warnings. (23.83 KB, patch)
2009-12-14 22:26 PST, Brent Fulgham
aroben: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2009-12-13 22:52:41 PST
Extend the existing WinLauncher application with an example printing implementation.
Comment 1 Brent Fulgham 2009-12-13 23:28:42 PST
Created attachment 44775 [details]
Add an example printing implementation the WinLauncher example program.
Comment 2 WebKit Review Bot 2009-12-13 23:30:27 PST
Attachment 44775 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WinLauncher/WinLauncher.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.h:33:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKitTools/WinLauncher/WinLauncher.h:102:  Extra space after ( in function call  [whitespace/parens] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:28:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:76:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7
Comment 3 Brent Fulgham 2009-12-14 21:54:06 PST
Created attachment 44842 [details]
Extend WinLauncher with printing support.
Comment 4 WebKit Review Bot 2009-12-14 21:58:20 PST
Attachment 44842 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.h:33:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKitTools/WinLauncher/WinLauncher.h:102:  Extra space after ( in function call  [whitespace/parens] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:28:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:76:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 6
Comment 5 Brent Fulgham 2009-12-14 22:26:52 PST
Created attachment 44846 [details]
Revised patch to clean up check-webkit-style warnings.
Comment 6 WebKit Review Bot 2009-12-14 22:30:22 PST
Attachment 44846 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:28:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/WinLauncher/PrintWebUIDelegate.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4
Comment 7 Adam Roben (:aroben) 2009-12-18 13:09:42 PST
Comment on attachment 44846 [details]
Revised patch to clean up check-webkit-style warnings.

> +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
Comment 8 Brent Fulgham 2009-12-19 23:01:07 PST
Landed in http://trac.webkit.org/changeset/52400.