[webkit-reviews] review requested: [Bug 89873] [WK2][GTK] Implement new API to save a web page using MHTML : [Attachment 149501] Patch Proposal + Unit Tests + Documentation

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 89873: [WK2][GTK] Implement new API to save a web page using MHTML
https://bugs.webkit.org/show_bug.cgi?id=89873

Attachment 149501: Patch Proposal + Unit Tests + Documentation
https://bugs.webkit.org/attachment.cgi?id=149501&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching new patch addressing these issues. See comments below...

(In reply to comment #5)
> (From update of attachment 149284 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=149284&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2184
> > +static void webViewFileSavedCallback(GObject *object, GAsyncResult
*result, gpointer data)
> 
> The * are placed wrongly here. Also, I would call this something like
replaceContentsCallback to make it clear this is the callback of
g_file_replace_contents_async()

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2190
> > +	 if (!g_file_replace_contents_finish(file, result, NULL, &error)) {
> 
> Use 0 instead of NULL

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2192
> > +	     return;
> 
> Don't return here, or the operation will never complete.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2198
> > +static void webViewMHTMLDataGotCallback(WKDataRef wkData, WKErrorRef,
void* context)
> 
> getContentsAsMHTMLDataCallback?

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2208
> > +	     /* We need to retain the data until the asyncronous process
> > +		initiated by the user has finished completely. */
> 
> Use C++ style comments

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2211
> > +	     /* If we are saving to a file we need to write the data on disk
before finishing. */
> 
> Ditto.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2215
> > +		 GFile* file = g_file_new_for_path(data->filePath.get());
> 
> Use GRefPtr for the file, since you are leaking it.
g_file_replace_contents_async() will take a reference of the file, so you can
just adopt the ref here.
> 

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2217
> > +		 g_file_replace_contents_async(file, reinterpret_cast<const
gchar*>(WKDataGetBytes(data->wkData.get())), WKDataGetSize(data->wkData.get()),
NULL, FALSE, G_FILE_CREATE_REPLACE_DESTINATION,
> > +					       data->cancellable.get(),
webViewFileSavedCallback, g_object_ref(result.get()));
> 
> Use 0 instead of NULL.
> 

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2241
> > +void webkit_web_view_save(WebKitWebView *webView, WebKitWebViewSaveMode
saveMode, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer
userData)
> 
> WebKitWebView *webView -> WebKitWebView* webView
> 

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2247
> > +	 /* We only support MHTML at the moment */
> > +	 if (saveMode != WEBKIT_SAVE_MODE_MHTML)
> > +	     return;
> 
> Use g_return_if_fail for this check.
> 

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2256
> > +	 WKPageGetContentsAsMHTMLData(wkPage, false, result,
webViewMHTMLDataGotCallback);
> 
> Just curiosity, why not using binary mode?
> 

It will work either way in WebKit based browers, both for writing a valid .mht
file and for being able to read it later on with a MHTML capable browser.
However, considering the .mht is a file with content type text/html, I think
it's better not to use binary encoding for the resources, so you can easily
inspect it with a text editor (as the content type seems to suggest you should
be able to do it).

Also, as per the following comment in Source/WebKit/public/WebPageSerializer.h,
it seems binary encoding could be unsupported in some other browsers:

    [...]
    // Serializes the WebView contents to a MHTML representation.
    WEBKIT_EXPORT static WebCString serializeToMHTML(WebView*);

    // Similar to serializeToMHTML but uses binary encoding for the MHTML
parts.
    // This results in a smaller MHTML file but it might not be supported by
other browsers.
    WEBKIT_EXPORT static WebCString
serializeToMHTMLUsingBinaryEncoding(WebView*);
    [...]

So, my vote goes for using printable characters for the .mht file.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2288
> > +	 GInputStream* dataStream = 0;
> > +	 ViewSaveAsyncData* data =
static_cast<ViewSaveAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpl
e));
> > +	 if (data) {
> > +	     gsize length = WKDataGetSize(data->wkData.get());
> > +	     dataStream =
g_memory_input_stream_new_from_data(g_memdup(WKDataGetBytes(data->wkData.get())
, length), length, g_free);
> > +	 }
> > +
> > +	 return dataStream;
> 
> We are saying that the function returns NULL in case of error, but here it
can return NULL for a valid empty page. If data is NULL, we could just return
an empty memory stream
> 

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2294
> > + * @path: a string containing a relative or absolute path for the file
where the current web page should be saved to. The string must be encoded in
the glib filename encoding.
> 
> I'm wondering whether we should receive a GFile instead, and use it directly
in g_file_replace_contents
>  

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2316
> > +	 /* We only support MHTML at the moment */
> > +	 if (saveMode != WEBKIT_SAVE_MODE_MHTML)
> > +	     return;
> 
> Use g_return_if_fail() for this check
>  

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:130
> > +typedef enum {
> > +	 WEBKIT_SAVE_MODE_MHTML
> > +} WebKitWebViewSaveMode;
> 
> Maybe WebKitSaveMode? or WebKitWebSaveMode and WEBKIT_WEB_SAVE_MODE_MHTML?
>   

Fixed. I used WebKitSaveMode.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:867
> > +static void webViewSavedToFileCallback(GObject *object, GAsyncResult *res,
WebViewTest* test)
> 
> GObject *object -> GObject* object
> GAsyncResult *res -> GAsyncResult* result
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:873
> > +	 g_main_loop_quit(test->m_mainLoop);
> 
> We have test->quitMainLoop() now.
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:879
> > +	 GInputStream* inputStream =
webkit_web_view_save_finish(test->m_webView, res, &error);
> 
> Use GRefPtr and adopt the ref to not leak it.
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:884
> > +	 GFile* file = g_file_new_for_path(kSaveDestinationFilePath);
> 
> Ditto.
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:885
> > +	 GInputStream* savedFileInputStream = G_INPUT_STREAM(g_file_read(file,
NULL, &error));
> 
> Ditto. And use 0 instead of NULL
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:894
> > +	 GOwnPtr<gchar> buffer1(g_new0(gchar, 512));
> > +	 GOwnPtr<gchar> buffer2(g_new0(gchar, 512));
> 
> Why not stack allocated buffers? 512 is small enough to use the stack, I'd
say.
> 

When I first started writing this unit test I considered bigger sizes for that
buffer and so that's why I ended up using the heap here. But I agree that 512
bytes is not that big, specially considering this is just an unit test, and
code is simpler.  

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:898
> > +	 while (readBytes = g_input_stream_read(inputStream, buffer1.outPtr(),
512, NULL, &error)) {
> 
> Use 0 instead of NULL
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:901
> > +	     totalBytesFromFile += g_input_stream_read(savedFileInputStream,
buffer2.outPtr(), 512, NULL, &error);
> 
> Ditto.
>   

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:913
> > +	 // Check that the same minimum amount of bytes are received.
> > +	 g_assert_cmpint(totalBytesFromStream, ==, totalBytesFromFile);
> > +	 g_assert_cmpint(totalBytesFromStream, >=, 2048);
> > +
> > +	 g_input_stream_close(inputStream, NULL, NULL);
> > +	 g_input_stream_close(savedFileInputStream, NULL, NULL);
> 
> Since you are not going to compare the contents, I think this could be a lot
simpler if you just get the size of the file without reading it and only read
from the input stream
> 

Ok. I changed this test following your suggestions.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:937
> > +	 test->waitUntilMainLoopFinishes();
> > +}
> 
> You should unlink and free the kSaveDestinationFilePath, or
g_rmdir(kTempDirectory); will fail

Fixed.