Bug 72952 – [GTK] Implement DownloadClient in WebKit2 GTK+ API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

To implement the WKDownloadClient.

[Carlos Garcia Campos]

Created attachment 116218 [details]
Patch

This is the downlaod client implementation. Unit tests are part of the following patch.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Created attachment 116229 [details]
New patch

I've tried to split the patches to make the review easier, bit it's not possible without breaking the build, so this is the previous patch merged with the WebContext API for start a download and including unit tests.

[Carlos Garcia Campos]

This depends on bug #72499, or it will crash.

[Carlos Garcia Campos]

Btw, I followed the approach of using a public client for consistency with the loader client, but I don't really like it. For downloads, I think the API would be easier to use following the approach I did for the UI client, keeping the client private and moving the relevant signals to WebContext and Download objects as signals or properties.

[Carlos Garcia Campos]

My proposal is something like: 

WebKitDownloadClient::started -> WebKitWebContext::download-started
WebKitDownloadClient::received-response -> WebKitDownload:response
WebKitDownloadClient::received-data -> WebKitDownload:bytes-received (or current-size, I'm not sure about the name)
WebKitDownloadClient::finished -> WebKitDownload::finished
WebKitDownloadClient::failed ->  WebKitDownload::failed
WebKitDownloadClient::decide-destination -> WebKitDownload::decide-destination
WebKitDownloadClient::created-destination -> WebKitDownload::created-destination
And make destination a property of WEbKitDownload so that it can be monitored.

The code would be mostly the same we just move the signals to download and web context objects. This way the API user have everything in the download object and doesn't need to keep a map of download objects.

[Martin Robinson]

(In reply to comment #6)
> My proposal is something like: 
> 
> WebKitDownloadClient::started -> WebKitWebContext::download-started
> WebKitDownloadClient::received-response -> WebKitDownload:response
> WebKitDownloadClient::received-data -> WebKitDownload:bytes-received (or current-size, I'm not sure about the name)
> WebKitDownloadClient::finished -> WebKitDownload::finished
> WebKitDownloadClient::failed ->  WebKitDownload::failed
> WebKitDownloadClient::decide-destination -> WebKitDownload::decide-destination
> WebKitDownloadClient::created-destination -> WebKitDownload::created-destination
> And make destination a property of WEbKitDownload so that it can be monitored.

I think this is a great proposal. The only thing I would change is WebKitDownloadClient::received-response  into WebKitDownloadClient::response-received.

[Martin Robinson]

(In reply to comment #7)

> I think this is a great proposal. The only thing I would change is WebKitDownloadClient::received-response  into WebKitDownloadClient::response-received.

I think I also prefer "data" to "bytes," but that's not a strong preference.

[Carlos Garcia Campos]

Created attachment 117025 [details]
New patch

I've removed the download client object and moved the code to the web context object.

[Gustavo Noronha (kov)]

(In reply to comment #7)
> I think this is a great proposal. The only thing I would change is WebKitDownloadClient::received-response  into WebKitDownloadClient::response-received.

What do you guys think of going with names similar to those used by soup? like got-response, or something? I think it makes sense to use "destination-created" to be consistent, too, and maybe "destination-requested"? Is the object implementing those signals in a different patch, btw?

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #7)
> > I think this is a great proposal. The only thing I would change is WebKitDownloadClient::received-response  into WebKitDownloadClient::response-received.
> 
> What do you guys think of going with names similar to those used by soup? like got-response, or something? I think it makes sense to use "destination-created" to be consistent, too, and maybe "destination-requested"? Is the object implementing those signals in a different patch, btw?

This names are consistent with current web loader client, received-server-redirect, for example, but I don't mind changing the loader client if you think it's better.

See also bug #72949

[Martin Robinson]

(In reply to comment #11)

> This names are consistent with current web loader client, received-server-redirect, for example, but I don't mind changing the loader client if you think it's better.
> 
> See also bug #72949

How closely we follow WebKit2 C API naming conventions might be a good topic for the WebKit2 API review session.

[Collabora GTK+ EWS bot]

Comment on attachment 117025 [details]
New patch

Attachment 117025 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10696450

[Carlos Garcia Campos]

The patch fails because it depends on bug #72949

[Martin Robinson]

Comment on attachment 117025 [details]
New patch

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.

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 117025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117025&action=review

Ah, one more comment. Do you mind moving the DownloadClient to a seperate file? I think it will keep things cleaner.

[Carlos Garcia Campos]

(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!

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 117025 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117025&action=review
> 
> Ah, one more comment. Do you mind moving the DownloadClient to a seperate file? I think it will keep things cleaner.

Sure!

[Carlos Garcia Campos]

(In reply to comment #15)

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

WEBKIT_EXEC_PATH already contains Relese/Debug, so it should work. Maybe we should move test resources to another common dir, but I think it's useful to use real files, maybe not for download tests, but we will need it for mime handling tests.

[Carlos Garcia Campos]

Created attachment 120552 [details]
New patch

Moved download client impl to its own file, and addressed other review comments.

[Carlos Garcia Campos]

Created attachment 123552 [details]
Updated patch

Updated to apply on current git master.

[Martin Robinson]

Comment on attachment 123552 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123552&action=review

Looks good. Please make the following fixes before landing. Using WEBKIT_TOP_LEVEL instead of walking up the directory chain is probably the most important thing below.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:214
> +    WebKitDownload* download = webkitDownloadCreate(wkDownload.get());
> +    downloadsMap().set(wkDownload.get(), download);

Could you use webkitWebContextGetOrCreateDownload here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:226
> +    return download.get();

I would ASSERT(download) here instead of doing it a bunch of times above.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:40
> +    enum DownloadEvents {

Nit: This should probably be called DownloadEvent intead of DownloadEvents.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:154
> +    void waitUntilDownloadFinished()

Nit: waitUntilDownloadFinishes or waitUntilDownloadIsFinished.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:159
> +    void checkDestinationAndDelete(WebKitDownload* download, const char* expectedName)

I'd probably call this checkDestinationAndDeleteFile, just to be clear.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:178
> +    static const char* webkit1TestResoucesDir = "Source/WebKit/gtk/tests/resources";

Merge this into the g_build_filename call below to avoid using Unixy slashes.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:181
> +    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());

It's probably better to use the WEBKIT_TOP_LEVEL approach that we use in WKTR and DumpRenderTree. Xan, for example, builds WebKit onto the other side of a symlink, so this test will fail for him.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:207
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), ==, 1.);

No need for the trailing . on 1. This is only necessary when forcing floating point division. This is a peculiarity of the WebKit style guide.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:211
> +class DownloadTestError: public DownloadTest {

Nit: Probably better to call this DownloadErrorTest.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:234
> +        g_assert(g_error_matches(error, WEBKIT_DOWNLOAD_ERROR, m_expectedError));

I think you should try to move this assertion out of the fixture. It's better to have the assertion in the body of the test since you can see exactly what the test is testing. What you could do is to have the fixture record the error it saw.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:263
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

No need for the . on 1.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:280
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:295
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:364
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

No need for the . in 1.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:378
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDownloads.cpp:393
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1.);

Ditto.

[Carlos Garcia Campos]

(In reply to comment #22)
> (From update of attachment 123552 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123552&action=review
> 
> Looks good. Please make the following fixes before landing. Using WEBKIT_TOP_LEVEL instead of walking up the directory chain is probably the most important thing below.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:214
> > +    WebKitDownload* download = webkitDownloadCreate(wkDownload.get());
> > +    downloadsMap().set(wkDownload.get(), download);
> 
> Could you use webkitWebContextGetOrCreateDownload here?

It's a new download, so we don't need to check whether it's in the map.

[Carlos Garcia Campos]

(In reply to comment #22)
> (From update of attachment 123552 [details])
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:226
> > +    return download.get();
> 
> I would ASSERT(download) here instead of doing it a bunch of times above.
> 

hmm, actually the ASSERT can just be removed, webkitWebContextGetOrCreateDownload will always return a valid pointer (well, expect in case OOM that we are not handling anyway).

[Carlos Garcia Campos]

Committed r105708: <http://trac.webkit.org/changeset/105708>