[Webkit-unassigned] [Bug 72952] [GTK] Implement DownloadClient in WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Martin Robinson <mrobinson at webkit.org>  2011-12-14 20:27:07 PST ---
(From update of attachment 117025)
View in context: https://bugs.webkit.org/attachment.cgi?id=117025&action=review

I tried running the tests for this patch and they seemed to freeze. Has it grown stale?

I have a couple comments below. This isn't a full review, but a few things I noticed when testing this patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:219
> +    WKRetainPtr<WKDownloadRef> wkDownload = WKContextDownloadURLRequest(priv->context.get(), wkRequest.get());
> +    GRefPtr<WebKitDownload> download = webkitDownloadCreate(wkDownload.get());

If a reference counted object is "just passing through" you don't need to use a smart pointer. You can just keep it in a raw pointer.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:49
> +    static void receivedResponseCallback(WebKitDownload* download, GParamSpec *, DownloadTest* test)

There's an exra space here after GParamSpec.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:185
> +    static const char* webkit1TestResoucesDir = "Source/WebKit/gtk/tests/resources";
> +    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(WEBKIT_EXEC_PATH));
> +    GRefPtr<GFile> parent = adoptGRef(g_file_get_parent(file.get()));
> +    file = g_file_get_parent(parent.get());
> +    GOwnPtr<char> rootDir(g_file_get_path(file.get()));
> +    GOwnPtr<char> resourcesDir(g_build_filename(rootDir.get(), webkit1TestResoucesDir, NULL));
> +    return resourcesDir.get();

I think this is actually written as if things are built into WebKitBuild directly, instead of WebKitBuild/Release. :) Hard-coding the relative path to this file will fail on somebody's system. For instance, Xan even builds on the other end of symmlink. I think a safer approach here is to just write a file into the temporary directory you create for the test. For the test, give the URL to that file you've written.

I like the tests in this patch.

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