[webkit-reviews] review denied: [Bug 89873] [WK2][GTK] Implement new API to save a web page using MHTML : [Attachment 157139] Patch Proposal + Unit Tests + 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 157139: Patch Proposal + Unit Tests + Documentation
https://bugs.webkit.org/attachment.cgi?id=157139&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=157139&action=review


Patch looks good, r- only because of the unit tests

> Source/WebKit2/ChangeLog:8
> +	   Implemented new asynchronous API in WebKitWebView.

This sounds too generic, please explain better what this patch is about.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2458
> + * specified in #WebKitSaveMode.

specified in @save_mode, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2508
> +    ViewSaveAsyncData* data =
static_cast<ViewSaveAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpl
e));
> +    if (data) {
> +	   gsize length = WKDataGetSize(data->wkData.get());
> +	   g_memory_input_stream_add_data(G_MEMORY_INPUT_STREAM(dataStream),
g_memdup(WKDataGetBytes(data->wkData.get()), length), length, g_free);
> +    }

data can't be null here, you should probably check that length is not 0 before
adding data to the stream. 

ViewSaveAsyncData* data =
static_cast<ViewSaveAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpl
e));
gsize length = WKDataGetSize(data->wkData.get());
if (length)
    g_memory_input_stream_add_data(G_MEMORY_INPUT_STREAM(dataStream),
g_memdup(WKDataGetBytes(data->wkData.get()), length), length, g_free);

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2524
> + * specified in #WebKitSaveMode and writing it to @file.

specified in @save_mode, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2557
> + * Returns: %TRUE if the web page was successfully saved on disk or %FALSE
otherwise.

I would said save saved to file, and avoid the word "disk"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:124
> + * @WEBKIT_SAVE_MODE_MHTML: Save the current page using the MHTML
> + * format.

This could be a single line

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:127
> + * Enum values to specify the different ways in which a #WebKitWebView
> + * will be saved the current web page into a self-contained file.

"can save its current web page into a self-contained file" maybe

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:341
> +WEBKIT_API GInputStream*

GInputStream* -> GInputStream *

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:953
> +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:988
> +    // Check that the a minimum amount of bytes are received in the stream.
> +    g_assert_cmpint(totalBytesFromStream, >=, 2048);

Why 2048?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:992
> +    // Check that the file exists and that it contains at least the same
amount of bytes.
> +    GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(),
G_FILE_ATTRIBUTE_STANDARD_SIZE, static_cast<GFileQueryInfoFlags>(0), 0, 0));
> +    g_assert_cmpint(g_file_info_get_size(fileInfo.get()), >=, 2048);

It's a bit weird that you are checking the file exists in the save callback
instead of the save_to_file one.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:999
> +static void testWebViewSave(UIClientTest* test, gconstpointer)

Why is this a UIClientTest?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1018
> +    kSaveDestinationFilePath = g_build_filename(kTempDirectory,
"testWebViewSaveResult.mht", NULL);
> +    GRefPtr<GFile> file =
adoptGRef(g_file_new_for_path(kSaveDestinationFilePath));
> +    webkit_web_view_save_to_file(test->m_webView, file.get(),
WEBKIT_SAVE_MODE_MHTML, 0,
reinterpret_cast<GAsyncReadyCallback>(webViewSavedToFileCallback), test);
> +    test->waitUntilMainLoopFinishes();
> +
> +    webkit_web_view_save(test->m_webView, WEBKIT_SAVE_MODE_MHTML, 0,
reinterpret_cast<GAsyncReadyCallback>(webViewSavedCallback), test);
> +    test->waitUntilMainLoopFinishes();

I would add a new class deriving from WebViewTest withj helper methods:

GInputStream* save();
GFile* saveToFile();

and here in the test function you can check the results, and compare them or
whatever.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1050
> +    g_rmdir(kTempDirectory);

Here you should free kTempDirectory and kSaveDestinationFilePath I think