[webkit-reviews] review requested: [Bug 223578] Add logging in IndexedDB to help debug flaky quota tests : [Attachment 423921] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has asked  for review:
Bug 223578: Add logging in IndexedDB to help debug flaky quota tests
https://bugs.webkit.org/show_bug.cgi?id=223578

Attachment 423921: Patch

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




--- Comment #3 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 423921
  --> https://bugs.webkit.org/attachment.cgi?id=423921
Patch

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

I'll leave the real review for experts, but I couldn't stop myself from adding
a couple comments.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:763
> +uint64_t IDBServer::diskUsage(const String& rootDirectory, const
ClientOrigin& origin, bool shouldPrintDetail)

Nit: it would be good to align the terminology ("log" vs. "print detail" vs.
"print usage detail").

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1281
> +		  
WTFLogAlways("SQLiteIDBBackingStore::databasesSizeForDirectory filePath='%s',
database='%s', size=%" PRIu64, file.utf8().data(), databaseName.utf8().data(),
fileSize);

Are there no privacy concerns with logging these? Seems questionable even in
regular browsing, and almost certainly not OK in private browsing. Bur perhaps
that's how the top level caller decides whether to pass
ShouldPrintUsageDetail::Yes.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2533
> +	   usage += IDBServer::IDBServer::diskUsage(idbRootPath, origin,
shouldPrintusageDetail == StorageQuotaManager::ShouldPrintUsageDetail::Yes);

shouldPrintusageDetail - "usage" should be title case too.