Bug 53197 – Make WebKit2 printing asynchronous

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[Alexey Proskuryakov]

Print preview still blocks the UI process, even with WebKit2. It shouldn't.

<rdar://problem/8895682>

[Alexey Proskuryakov]

Created attachment 80250 [details]
proposed patch

[Alexey Proskuryakov]

This will temporarily break window.print().

[Darin Adler]

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]

Attachment 80250 [details] did not build on win:
Build output: http://queues.webkit.org/results/7501331

[Early Warning System Bot]

Attachment 80250 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7565387

[WebKit Review Bot]

Attachment 80250 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7614364

[Alexey Proskuryakov]

> 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]

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.