Print preview still blocks the UI process, even with WebKit2. It shouldn't.
Created attachment 80250 [details]
This will temporarily break window.print().
Comment on attachment 80250 [details]
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.
> + 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”.
> + // 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.
> - 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.
> + HashMap<WebCore::IntRect, Vector<uint8_t> > _pagePreviews;
A typedef for this HashMap could make the code below easier to read.
> + HashMap<uint64_t, WebCore::IntRect> _expectedPreviewCallbacks;
> +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.
> + (*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.
> + 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?
> + RefPtr<DataCallback> cb = DataCallback::create(context, pageDidDrawToPDF);
How about splurging on a few extra letters and calling this callback instead of cb?
> +static void pageDidComputePageRects(const Vector<WebCore::IntRect>& pageRects, double totalScaleFactorForPrinting, WKErrorRef, void* ctx)
Still not a fan of ctx.
> + 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.
> + IPCCallbackContext* context = new IPCCallbackContext;
Maybe it’s a bit stilted, but I like the idea of using OwnPtr and leakPtr instead of raw pointers.
> + RefPtr<ComputedPagesCallback> cb = ComputedPagesCallback::create(context, pageDidComputePageRects);
Again, I prefer the full word name, callback.
> + _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.