We should wipe out any leftover data when incrementing cache version.
Created attachment 254567 [details] patch
Attachment 254567 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:26: #ifndef header guard has wrong style, please use: NetworkCacheFileSystem_h [build/header_guard] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 254568 [details] patch
Attachment 254568 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:26: #ifndef header guard has wrong style, please use: NetworkCacheFileSystem_h [build/header_guard] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:57: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254568 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254568&action=review Looks like this does not build on GTK yet. > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:69 > + function(String(name), directoryEntryType(dp->d_type)); Doing String(name) here interprets the filename as Latin-1. That’s not right. This needs to be the reverse of WebCore::fileSystemRepresentation. I see some code using String::fromUTF8 for this; openTemporaryFile on the Mac for example. This only works because we are lucky enough to not have any non-ASCII filenames in the directory. Of course, decoding UTF-8 has the inverse problem if there are some file names that we can’t represent as String. I suppose we have to just skip those files. This can’t happen in normal circumstances on OS X, but might happen in other cases on other platforms perhaps. > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:57 > + function(String(name)); Same String(name) issue here. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:123 > + auto partitionPath = WebCore::pathByAppendingComponent(recordsPath, subdirName); > + WebCore::deleteEmptyDirectory(partitionPath); Not sure the local variable makes this easier to read > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:878 > + String versionString = subdirName.substring(strlen(versionDirectoryPrefix)); > + bool success; > + unsigned directoryVersion = versionString.toUIntStrict(&success); Should do this with a StringView. No need to copy out a new string just to convert to integer.
Comment on attachment 254568 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254568&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:62 > + struct dirent* dp; I think the "struct" is superfluous in C++. > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:69 > + function(String(name), directoryEntryType(dp->d_type)); Probably not a big deal for what you're using it for but this likely only works if the name only include latin1 characters. I think String::fromUTF8(name) would be more correct. > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:94 > + struct stat fileInfo; struct is superfluous I believe. > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:121 > + utimes(WebCore::fileSystemRepresentation(path).data(), 0); nullptr > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. Not commenting on this file as it seems it is the diff being confused. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876 > + String versionString = subdirName.substring(strlen(versionDirectoryPrefix)); We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime.
Comment on attachment 254568 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254568&action=review Setting review+ again. >> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876 >> + String versionString = subdirName.substring(strlen(versionDirectoryPrefix)); > > We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime. Modern compilers like clang constant fold calls to strlen, so this is resolved at compile time.
Comment on attachment 254568 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254568&action=review >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876 >>> + String versionString = subdirName.substring(strlen(versionDirectoryPrefix)); >> >> We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime. > > Modern compilers like clang constant fold calls to strlen, so this is resolved at compile time. Ah, I did not know that. This is good to know. Please disregard my comment then, strlen() is a lot more readable :)
Created attachment 254646 [details] patch
Attachment 254646 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:57: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
https://trac.webkit.org/r185412 (KaL promised to look into the gtk build issue)
> Should do this with a StringView. No need to copy out a new string just to > convert to integer. Didn't do this. It looked too ugly for a meaningless performance optimization (or maybe I just don't know how to use StringView correctly).