[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