...
Created attachment 451947 [details] Patch
Comment on attachment 451947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451947&action=review > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:181 > auto files = FileSystem::listDirectory(m_rootPath); Can we please add an assertion that this is called off the main thread given that it does file system operations? > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:193 > + FileSystem::deleteEmptyDirectory(currentIDBStoragePath); I will say it is fishy that a function called "isEmpty()" tries to remove a directory on disk (even an empty one). > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:247 > + if (!m_resolvedIDBStoragePath.isNull()) Can we please add an assertion that this is called off the main thread given that it does file system operations? > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:251 > + ASSERT(!(m_customIDBStoragePath.isEmpty() ^ m_rootPath.isEmpty())); Odd to be using ^ here. Would this be identical: `ASSERT(m_customIDBStoragePath.isEmpty() == m_rootPath.isEmpty()));` ? > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:359 > + ASSERT(!(m_customLocalStoragePath.isEmpty() ^ m_rootPath.isEmpty())); Ditto. Not very readable. > Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:79 > + bool m_useCustomPaths; Can we move all the booleans together and at the end for better packing? > Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:165 > + parameters.useCustomStoragePaths = WTFMove(*useCustomStoragePaths); No WTFMove() for a boolean. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:135 > + bool useCustomStoragePaths() const { return m_useCustomStoragePaths; } This is not a good name for a getter, this is why we usually have prefixes for getters in Webkit. For example, I think we would usually name this `shouldUseCustomStoragePaths`. Alternatively, at the WebKit API layer, we also use things like "usesCustomStoragePaths()". "useCustomStoragePage()" for a getter is confusing though. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:210 > + bool m_useCustomStoragePaths { true }; Ditto about naming. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:851 > + configuration = nil; These 3 commands should not be needed since they are scope local and we're about to go out of scope. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:934 > + configuration = nil; ditto.
Comment on attachment 451947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451947&action=review >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:181 >> auto files = FileSystem::listDirectory(m_rootPath); > > Can we please add an assertion that this is called off the main thread given that it does file system operations? Sure. >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:193 >> + FileSystem::deleteEmptyDirectory(currentIDBStoragePath); > > I will say it is fishy that a function called "isEmpty()" tries to remove a directory on disk (even an empty one). I will change it to check if the idb directory has any files in it. >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:247 >> + if (!m_resolvedIDBStoragePath.isNull()) > > Can we please add an assertion that this is called off the main thread given that it does file system operations? Sure. >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:251 >> + ASSERT(!(m_customIDBStoragePath.isEmpty() ^ m_rootPath.isEmpty())); > > Odd to be using ^ here. Would this be identical: > `ASSERT(m_customIDBStoragePath.isEmpty() == m_rootPath.isEmpty()));` ? Sure. >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:359 >> + ASSERT(!(m_customLocalStoragePath.isEmpty() ^ m_rootPath.isEmpty())); > > Ditto. Not very readable. Will change. >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:79 >> + bool m_useCustomPaths; > > Can we move all the booleans together and at the end for better packing? Okay. >> Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:165 >> + parameters.useCustomStoragePaths = WTFMove(*useCustomStoragePaths); > > No WTFMove() for a boolean. Will remove. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:135 >> + bool useCustomStoragePaths() const { return m_useCustomStoragePaths; } > > This is not a good name for a getter, this is why we usually have prefixes for getters in Webkit. > For example, I think we would usually name this `shouldUseCustomStoragePaths`. Alternatively, at the WebKit API layer, we also use things like "usesCustomStoragePaths()". "useCustomStoragePage()" for a getter is confusing though. Will change to shouldUseCustomStoragePaths. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:210 >> + bool m_useCustomStoragePaths { true }; > > Ditto about naming. Will change. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:851 >> + configuration = nil; > > These 3 commands should not be needed since they are scope local and we're about to go out of scope. Sure. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:934 >> + configuration = nil; > > ditto. Sure.
Created attachment 452130 [details] Patch for landing
Created attachment 452131 [details] Patch for landing
Committed r289878 (247316@main): <https://commits.webkit.org/247316@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452131 [details].
<rdar://problem/89008688>