[webkit-reviews] review denied: [Bug 89873] [WK2][GTK] Implement new API to save a web page using MHTML : [Attachment 149284] Patch proposal + Unit test + Documentation

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request 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 149284: Patch proposal + Unit test + Documentation
https://bugs.webkit.org/attachment.cgi?id=149284&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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()

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

Use 0 instead of NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2192
> +	   return;

Don't return here, or the operation will never complete.

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

getContentsAsMHTMLDataCallback?

> 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

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

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

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

> 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

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2256
> +    WKPageGetContentsAsMHTMLData(wkPage, false, result,
webViewMHTMLDataGotCallback);

Just curiosity, why not using binary mode?

> 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

> 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

> 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

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

> 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

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

We have test->quitMainLoop() now.

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

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

Ditto.

> 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

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


> 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

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

Ditto.

> 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

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:937
> +    test->waitUntilMainLoopFinishes();
> +}

You should unlink and free the kSaveDestinationFilePath, or
g_rmdir(kTempDirectory); will fail