Bug 89872 – [WK2] Add new C API to generate MHTML data from the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mario Sanchez Prada]

It would be good to have a way in WK2 to generate MHTML date for a page from the UIProcess, by providing a new C API wrapping the following functions from WebCore::MHTMLArchive:

    class MHTMLArchive : public Archive {
    public:
        [...]
        static PassRefPtr<SharedBuffer> generateMHTMLData(Page*);
        // Binary encoding results in smaller MHTML files but they might not work in other browsers.
        static PassRefPtr<SharedBuffer> generateMHTMLDataUsingBinaryEncoding(Page*);
        [...]
    };

This bug is for that.

[Mario Sanchez Prada]

Adding people that might be interested in this bug to CC

[Mario Sanchez Prada]

Created attachment 149283 [details]
Patch proposal

Patch implemented this wrapping with just one function and a 'bool useBinaryEncoding' parameter.

[Build Bot]

Comment on attachment 149283 [details]
Patch proposal

Attachment 149283 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13087169

[Mario Sanchez Prada]

Created attachment 149289 [details]
Patch proposal

Fixing issue in the mac EWS bot

[Build Bot]

Comment on attachment 149289 [details]
Patch proposal

Attachment 149289 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13089165

[Carlos Garcia Campos]

Comment on attachment 149289 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=149289&action=review

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:602
> +    WKPageGetContentsAsMHTMLData(pageRef, callGetContentsAsMHTMLDataBlockAndDispose, Block_copy(block));

You are missing the bool useBinaryEncoding parameter here. And the order is context, callback, you are passing the callback before the context.

[Mario Sanchez Prada]

Created attachment 149472 [details]
Patch proposal

(In reply to comment #6)
> (From update of attachment 149289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149289&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPage.cpp:602
> > +    WKPageGetContentsAsMHTMLData(pageRef, callGetContentsAsMHTMLDataBlockAndDispose, Block_copy(block));
> 
> You are missing the bool useBinaryEncoding parameter here. And the order is context, callback, you are passing the callback before the context.

Ah! That explains things. Attaching a new patch

[Mario Sanchez Prada]

Ping reviewers?

[Carlos Garcia Campos]

Comment on attachment 149472 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=149472&action=review

Patch looks good to me in general, but since it adds new C API it would be great if Anders or Sam could take a look at it too.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1845
> +    if (buffer)
> +        dataReference = CoreIPC::DataReference(reinterpret_cast<const uint8_t*>(buffer->data()), buffer->size());

I don't think buffer can be null

[Mario Sanchez Prada]

(In reply to comment #9)
> (From update of attachment 149472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149472&action=review
> 
> Patch looks good to me in general, but since it adds new C API it would be 
> great if Anders or Sam could take a look at it too.

I already pinged Anders in IRC, who already confirmed before me being on holidays that he would take a look to this.

[Anders Carlsson]

Comment on attachment 149472 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=149472&action=review

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:604
> +#ifdef __BLOCKS__
> +static void callGetContentsAsMHTMLDataBlockAndDispose(WKDataRef mhtmlData, WKErrorRef error, void* context)
> +{
> +    WKPageGetContentsAsMHTMLDataBlock block = (WKPageGetContentsAsMHTMLDataBlock)context;
> +    block(mhtmlData, error);
> +    Block_release(block);
> +}
> +
> +void WKPageGetContentsAsMHTMLData_b(WKPageRef pageRef, bool useBinaryEncoding, WKPageGetContentsAsMHTMLDataBlock block)
> +{
> +    WKPageGetContentsAsMHTMLData(pageRef, useBinaryEncoding, Block_copy(block), callGetContentsAsMHTMLDataBlockAndDispose);
> +}
> +#endif

We don't need this API, blocks don't work well with the C API since none of the WK objects are captured by value.

[Mario Sanchez Prada]

(In reply to comment #11)
> [...] 
> We don't need this API, blocks don't work well with the C API since none of
> the WK objects are captured by value.

Thanks for pointing out this issue (which I was not aware of).

I'll remove that API when landing. Thanks for the review.

[Mario Sanchez Prada]

Committed r125002: <http://trac.webkit.org/changeset/125002>