[webkit-reviews] review granted: [Bug 72952] [GTK] Implement DownloadClient in WebKit2 GTK+ API : [Attachment 123552] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 72952: [GTK] Implement DownloadClient in WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=72952

Attachment 123552: Updated patch
https://bugs.webkit.org/attachment.cgi?id=123552&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.