[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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|75225                       |




--- Comment #17 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-12-26 04:19:31 PST ---
(In reply to comment #15)
> (From update of attachment 117025 [details])
> 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?

It seems r101607 broke it, see bug #75225.

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

Thanks for looking at it!

> > 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.

Ok.

> > 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.

Ok.

> > 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.

Ok, I'll try to create a custom file for this.

> I like the tests in this patch.

Thanks!

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