[Webkit-unassigned] [Bug 89873] [WK2][GTK] Implement new API to save a web page using MHTML
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 26 03:09:43 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=89873
Mario Sanchez Prada <msanchez at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #149284|0 |1
is obsolete| |
Attachment #149501| |review?
Flag| |
--- Comment #6 from Mario Sanchez Prada <msanchez at igalia.com> 2012-06-26 03:09:41 PST ---
Created an attachment (id=149501)
--> (https://bugs.webkit.org/attachment.cgi?id=149501&action=review)
Patch Proposal + Unit Tests + Documentation
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(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
>
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list