Print preview still blocks the UI process, even with WebKit2. It shouldn't. <rdar://problem/8895682>
Created attachment 80250 [details] proposed patch
This will temporarily break window.print().
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.
Attachment 80250 [details] did not build on win: Build output: http://queues.webkit.org/results/7501331
Attachment 80250 [details] did not build on qt: Build output: http://queues.webkit.org/results/7565387
Attachment 80250 [details] did not build on mac: Build output: http://queues.webkit.org/results/7614364
> 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.
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.