RESOLVED FIXED 53197
Make WebKit2 printing asynchronous
https://bugs.webkit.org/show_bug.cgi?id=53197
Summary Make WebKit2 printing asynchronous
Alexey Proskuryakov
Reported 2011-01-26 15:22:30 PST
Print preview still blocks the UI process, even with WebKit2. It shouldn't. <rdar://problem/8895682>
Attachments
proposed patch (47.14 KB, patch)
2011-01-26 15:57 PST, Alexey Proskuryakov
darin: review+
Alexey Proskuryakov
Comment 1 2011-01-26 15:57:51 PST
Created attachment 80250 [details] proposed patch
Alexey Proskuryakov
Comment 2 2011-01-26 15:58:09 PST
This will temporarily break window.print().
Darin Adler
Comment 3 2011-01-26 16:29:49 PST
Comment on attachment 80250 [details] proposed patch 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.
Build Bot
Comment 4 2011-01-26 16:37:11 PST
Early Warning System Bot
Comment 5 2011-01-26 17:09:27 PST
WebKit Review Bot
Comment 6 2011-01-26 22:30:32 PST
Alexey Proskuryakov
Comment 7 2011-01-27 10:28:26 PST
> 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. Definitely agreed - hopefully, we can make it better when APIs we requested are added. > > 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. This is one WebKit coding style pattern that frequently annoys me. Whenever I go to a header file to look what a compound member variable is, I have to also hunt down its type's typedef. On the other hand, while an iterator type like "HashMap<WebCore::IntRect, Vector<uint8_t> >::iterator" adds significant visual noise, it's not something you need to actually read and understand as long as compiler doesn't complain. > Other people have been using OwnPtr and adoptPtr instead of delete. This allows you to do things like early returns if you like. Done, looks nice. > > 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. This one doesn't look so nice in this case - the context is passed to ComputedPagesCallback::create(), but then it's also changed. I would either have to call leakPtr later, or access context indirectly. Neither is beautiful. OwnPtr<IPCCallbackContext> context = adoptPtr(new IPCCallbackContext); RefPtr<ComputedPagesCallback> callback = ComputedPagesCallback::create(context.leakPtr(), pageDidComputePageRects); _expectedComputedPagesCallback = callback->callbackID(); callback->context->view = self; // Why not use context here? callback->context->callbackID = _expectedComputedPagesCallback; // Ditto.
Alexey Proskuryakov
Comment 8 2011-01-27 11:38:37 PST
Committed <http://trac.webkit.org/changeset/76821>. I also took out beginPrinting() change - I'm no longer sure that implicitly beginning printing when drawing PDF is a good idea. Layout is time consuming, and the client knows better when something changes that requires relayout.
Note You need to log in before you can comment on or make changes to this bug.