Bug 72952

Summary: [GTK] Implement DownloadClient in WebKit2 GTK+ API
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, gustavo.noronha, mrobinson, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 72499, 72949    
Bug Blocks: 73662    
Attachments:
Description Flags
Patch
none
New patch
none
New patch
gustavo.noronha: commit-queue-
New patch
none
Updated patch mrobinson: review+

Description Carlos Garcia Campos 2011-11-22 06:42:42 PST
To implement the WKDownloadClient.
Comment 1 Carlos Garcia Campos 2011-11-22 06:51:31 PST
Created attachment 116218 [details]
Patch

This is the downlaod client implementation. Unit tests are part of the following patch.
Comment 2 WebKit Review Bot 2011-11-22 06:57:37 PST
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
Comment 3 Carlos Garcia Campos 2011-11-22 08:23:25 PST
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.
Comment 4 Carlos Garcia Campos 2011-11-22 08:24:43 PST
This depends on bug #72499, or it will crash.
Comment 5 Carlos Garcia Campos 2011-11-22 23:37:02 PST
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.
Comment 6 Carlos Garcia Campos 2011-11-23 08:46:11 PST
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.
Comment 7 Martin Robinson 2011-11-23 09:24:48 PST
(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.
Comment 8 Martin Robinson 2011-11-23 09:25:28 PST
(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.
Comment 9 Carlos Garcia Campos 2011-11-29 12:51:33 PST
Created attachment 117025 [details]
New patch

I've removed the download client object and moved the code to the web context object.
Comment 10 Gustavo Noronha (kov) 2011-11-30 06:10:17 PST
(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?
Comment 11 Carlos Garcia Campos 2011-11-30 06:26:24 PST
(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
Comment 12 Martin Robinson 2011-11-30 06:28:18 PST
(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.
Comment 13 Collabora GTK+ EWS bot 2011-11-30 19:43:26 PST
Comment on attachment 117025 [details]
New patch

Attachment 117025 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10696450
Comment 14 Carlos Garcia Campos 2011-12-01 01:16:39 PST
The patch fails because it depends on bug #72949
Comment 15 Martin Robinson 2011-12-14 20:27:07 PST
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.
Comment 16 Martin Robinson 2011-12-14 20:28:24 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

Ah, one more comment. Do you mind moving the DownloadClient to a seperate file? I think it will keep things cleaner.
Comment 17 Carlos Garcia Campos 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!
Comment 18 Carlos Garcia Campos 2011-12-26 04:20:08 PST
(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!
Comment 19 Carlos Garcia Campos 2011-12-26 05:49:33 PST
(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.
Comment 20 Carlos Garcia Campos 2011-12-26 06:04:18 PST
Created attachment 120552 [details]
New patch

Moved download client impl to its own file, and addressed other review comments.
Comment 21 Carlos Garcia Campos 2012-01-23 06:46:10 PST
Created attachment 123552 [details]
Updated patch

Updated to apply on current git master.
Comment 22 Martin Robinson 2012-01-23 16:50:31 PST
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.
Comment 23 Carlos Garcia Campos 2012-01-24 00:55:02 PST
(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.
Comment 24 Carlos Garcia Campos 2012-01-24 00:58:14 PST
(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).
Comment 25 Carlos Garcia Campos 2012-01-24 01:45:49 PST
Committed r105708: <http://trac.webkit.org/changeset/105708>