[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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123552|review?                     |review+
               Flag|                            |




--- Comment #22 from Martin Robinson <mrobinson at webkit.org>  2012-01-23 16:50:31 PST ---
(From update of attachment 123552)
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.

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