[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