[Webkit-unassigned] [Bug 89873] [WK2][GTK] Implement new API to save a web page using MHTML

Tue Jun 26 00:41:08 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=89873

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #149284|review?                     |review-
               Flag|                            |

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-06-26 00:41:07 PST ---
(From update of attachment 149284)
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(simple));
> +    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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.