[webkit-reviews] review granted: [Bug 53197] Make WebKit2 printing asynchronous : [Attachment 80250] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 26 16:29:48 PST 2011


Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 53197: Make WebKit2 printing asynchronous
https://bugs.webkit.org/show_bug.cgi?id=53197

Attachment 80250: proposed patch
https://bugs.webkit.org/attachment.cgi?id=80250&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80250&action=review

There is some really fragile stuff here, with various assumptions about AppKit.
I wonder what we can do to reduce the assumptions and tighten it up.

> Source/WebKit2/UIProcess/GenericCallback.h:121
> +	   RefPtr<WebError> error = WebError::create();
> +	   m_callback(Vector<WebCore::IntRect>(), 0, toAPI(error.get()),
m_context);

I don’t think you need a local variable for “error”.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2197
> +	   // FIXME: Log error or assert.

I think we probably want to taint the web process here rather than logging an
error or asserting. The same thing we do with bad frame IDs, for example. Or
maybe I have this backwards.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:-39
> -    Vector<WebCore::IntRect> _webPrintingPageRects;
> -    double _webTotalScaleFactorForPrinting;

Were the “web” prefixes here an attempt to avoid conflicting with AppKit field
names? The rules about who gets to use underscore-prefixed identifiers are
unclear! Maybe we shouldn’t use underscores at all since this is a private
class and the issue is conflict with AppKit, not with framework clients.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:42
> +    HashMap<WebCore::IntRect, Vector<uint8_t> > _pagePreviews;

A typedef for this HashMap could make the code below easier to read.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:48
> +    HashMap<uint64_t, WebCore::IntRect> _expectedPreviewCallbacks;

Ditto.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:132
> +static void pageDidDrawToPDF(WKDataRef dataRef, WKErrorRef, void* ctx)

There must be a better name than ctx. I think I used the name untypedContext
somewhere for this.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:156
> +	       (*entry.first).second.append(data->bytes(), data->size());

You can use an arrow here instead of (*x). and I think it’s easier to read.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:187
> +    if (lastPage > _printingPageRects.size()) // Last page is NSIntegerMax
if not set by the user.

NSIntegerMax won’t fit in an unsigned. I guess we get the max unsigned?

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:195
> +    RefPtr<DataCallback> cb = DataCallback::create(context,
pageDidDrawToPDF);

How about splurging on a few extra letters and calling this callback instead of
cb?

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:204
> +static void pageDidComputePageRects(const Vector<WebCore::IntRect>&
pageRects, double totalScaleFactorForPrinting, WKErrorRef, void* ctx)

Still not a fan of ctx.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:208
> +    IPCCallbackContext* context = static_cast<IPCCallbackContext*>(ctx);

Other people have been using OwnPtr and adoptPtr instead of delete. This allows
you to do things like early returns if you like.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:244
> +    IPCCallbackContext* context = new IPCCallbackContext;

Maybe it’s a bit stilted, but I like the idea of using OwnPtr and leakPtr
instead of raw pointers.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:245
> +    RefPtr<ComputedPagesCallback> cb =
ComputedPagesCallback::create(context, pageDidComputePageRects);

Again, I prefer the full word name, callback.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:250
> +    _webFrame->page()->computePagesForPrinting(_webFrame.get(),
PrintInfo([_printOperation printInfo]), cb.get());

Should be release, not get, here.


More information about the webkit-reviews mailing list