[webkit-reviews] review granted: [Bug 16826] [Gtk] Implement WebKitDownload : [Attachment 28191] proposed implementation

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 16826: [Gtk] Implement WebKitDownload
https://bugs.webkit.org/show_bug.cgi?id=16826

Attachment 28191: proposed implementation
https://bugs.webkit.org/attachment.cgi?id=28191&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Please add bug URL to all ChangeLogs.

+    // The frame could be null is the ResourceHandle is not associated to any
+    // Frame, i.e. if we are downloading a file.

I think that's "e.g.", not "i.e."

+    // we could reuse the same handle, but our replacing of the
+    // client seems to make this impossible; the main load fails
+    // and is stopped

I don't understand this comment - there is a setClient() method in
ResourceHandle (and it is even used elsewhere in this patch). What is the
reason that makes reusing the handle impossible?

Also, we prefer full sentences in comments (starting with a capital letter,
ending with a period).

 #include <webkit/webkitnetworkrequest.h>
+#include <webkit/webkitdownload.h>

Please keep the list sorted. This problem is repeated in several files.

+extern "C" {
+
+class DownloadClient : Noncopyable, public ResourceHandleClient {

How can a class be extern "C"? There are no classes in C. I don't think extern
"C" is ever needed in .cpp files - API methods should have it on declarations,
and everything else shouldn't be exported anyway.

+    WebCore::ResourceResponse* network_response;
+    RefPtr<WebCore::ResourceHandle> resource_handle;

There is "using namespace WebCore" at the top of this file, are explicit
namespaces necessary here?

+static guint webkit_download_signals[LAST_SIGNAL] = { 0, };

I don't remember the standards precisely, but this trailing comma is either
forbidden or discouraged in various C dialects, please remove it.

+    PROP_TOTAL_SIZE,
+};

Same comment here.

+    if(error) {

There should be a space after if.

+    priv->timer = g_timer_new ();

But no space here.

+    /* FIXME can we have a better check? */

A FIXME like this should explain what's wrong with the current check.

+	 GFile* dest = g_file_new_for_uri(destination_uri);
+	 GError *error = NULL;

Misplaced star here, and NULL instead of 0.

+    } else {
+      g_free(priv->destination_uri);
+      priv->destination_uri = g_strdup(destination_uri);
+    }

Two-space indentation here.

+WebKitDownloadState webkit_download_get_state (WebKitDownload* download)

An extra space again (and in other functions below).

+    if (priv->current_size == 0) {
+	 priv->state = WEBKIT_DOWNLOAD_STATE_STARTED;
+    }

There should be no braces around single line blocks.

 #include <webkit/webkitwebframe.h>
 #include <webkit/webkitwebpolicydecision.h>
 #include <webkit/webkitwebnavigationaction.h>
+#include <webkit/webkitdownload.h>
 #include <webkit/webkitwebsettings.h>
 #include <webkit/webkitwebwindowfeatures.h>
 #include <webkit/webkitwebbackforwardlist.h>
@@ -44,9 +45,13 @@
 #include "InspectorClientGtk.h"
 #include "FrameLoaderClient.h"
 #include "WindowFeatures.h"
+#include "ResourceHandle.h"
+#include "ResourceResponse.h"

Please keep include lists sorted.
There is a number of C-style casts, Ñ-style comments and NULL variables
(instead of 0) in C++ files here. It is a bit of border case, as the
implemented functions are very C-style in nature, but our coding style asks for
C++ style in C++ files. Are C-style comments necessary for documentation
generator to work properly?

A number of misplaced stars, too.

Obviously, I cannot adequately review some Gtk-specific parts of the patch, but
from the above comments, they have been extensively discussed, so that's OK.

I had many comments, but they are mostly style nitpicks, so I'll say r=me
anyway. Please fix as many as you can when landing, and you can even consider
submitting an updated patch for another quick review round.