[webkit-reviews] review granted: [Bug 168171] [GTK][WPE] All resource load statistics tests added in r212183 crash in GTK bots, timeout in GTK and WPE bots since r219049 : [Attachment 322806] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 5 09:02:58 PDT 2017


Chris Dumez <cdumez at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 168171: [GTK][WPE] All resource load statistics tests added in r212183
crash in GTK bots, timeout in GTK and WPE bots since r219049
https://bugs.webkit.org/show_bug.cgi?id=168171

Attachment 322806: Patch

https://bugs.webkit.org/attachment.cgi?id=322806&action=review




--- Comment #26 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 322806
  --> https://bugs.webkit.org/attachment.cgi?id=322806
Patch

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

r=me with review comments.

> Source/WebCore/platform/glib/FileMonitorGLib.cpp:38
> +    if (path.isEmpty() || !modificationHandler)

This is a bad check. !modificationHandler will always be true since you
WTFMove() it above..

I think this show this is not tested. This is supposed to be covered by
Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp API test which is probably
not enabled for GTK port?

> Source/WebCore/platform/glib/FileMonitorGLib.cpp:41
> +    GRefPtr<GFile> file =
adoptGRef(g_file_new_for_path(fileSystemRepresentation(path).data()));

auto file ?

> Source/WebCore/platform/glib/FileMonitorGLib.cpp:60
> +    if (event == G_FILE_MONITOR_EVENT_DELETED)

We may not want to call the callback if event ==
G_FILE_MONITOR_EVENT_ATTRIBUTE_CHANGED ?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:44
> +typedef void (*WKWebsiteDataStoreIsStatisticsPrevalentResourceFunction)(bool
isPrevalentResource, WKErrorRef error, void* functionContext);

Do we ever use those WKErrorRef parameters? If not, I would suggest dropping
them.

> Tools/WebKitTestRunner/TestController.cpp:2312
> +struct ResourceStatisticsCallbackContext {

I would give this a constructor that takes a TestController& in parameter and
initializes both done / result to false.
ResourceStatisticsCallbackContext(TestController& testController)
    : testController(&testController)
{ }

otherwise, the call sites are not very readable.

> Tools/WebKitTestRunner/TestController.cpp:2314
> +    bool done;

bool done { false };

> Tools/WebKitTestRunner/TestController.cpp:2315
> +    bool result;

bool result { false };

> Tools/WebKitTestRunner/TestController.cpp:2329
> +    ResourceStatisticsCallbackContext context = { this, false, false };

ResourceStatisticsCallbackContext context(*this);

> Tools/WebKitTestRunner/TestController.cpp:2345
> +    ResourceStatisticsCallbackContext context = { this, false, false };

ResourceStatisticsCallbackContext context(*this);

> Tools/WebKitTestRunner/TestController.cpp:2361
> +    ResourceStatisticsCallbackContext context = { this, false, false };

ResourceStatisticsCallbackContext context(*this);


More information about the webkit-reviews mailing list