...
Created attachment 449898 [details] Patch
Comment on attachment 449898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449898&action=review > Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in:32 > + SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String value, String urlString) -> (uint64_t storageMapSeed, String key, bool quotaException) Async WantsConnection > + RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String urlString) -> (uint64_t storageMapSeed, String key) Async WantsConnection We don’t need these keys in the return values, can just capture them instead. See below ... > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692 > + completionHandler(storageMapSeed, key, hasQuotaError); Why not use WTFMove(key) here to save a tiny bit of reference count churn? Except if you take the suggestion of not sending the key back in the return value, then you can use that WTFMove(key) above instead of String { key }. > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:700 > storageArea->removeItem(connection.uniqueID(), implIdentifier, key, WTFMove(urlString)); May be able to use WTFMove(key) here if we get rid of the key below. But if the removeItem function takes a const String&, then I guess that won’t be helpful. > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:108 > + auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key, bool quotaException) mutable { > + if (weakThis) > + weakThis->didSetItem(storageMapSeed, key, quotaException); > + }; We could capture a copy of the key here and then we would not need the completion handler to pass the key back to us, saving some XPC. > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134 > + auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key) mutable { > + if (weakThis) > + weakThis->didRemoveItem(storageMapSeed, key); > + }; Ditto.
(In reply to Darin Adler from comment #2) > Comment on attachment 449898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449898&action=review Thanks for the review. > > > Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in:32 > > + SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String value, String urlString) -> (uint64_t storageMapSeed, String key, bool quotaException) Async WantsConnection > > + RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String urlString) -> (uint64_t storageMapSeed, String key) Async WantsConnection > > We don’t need these keys in the return values, can just capture them > instead. See below ... Ah right, will change. > > > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692 > > + completionHandler(storageMapSeed, key, hasQuotaError); > > Why not use WTFMove(key) here to save a tiny bit of reference count churn? > Except if you take the suggestion of not sending the key back in the return > value, then you can use that WTFMove(key) above instead of String { key }. > > > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:700 > > storageArea->removeItem(connection.uniqueID(), implIdentifier, key, WTFMove(urlString)); > > May be able to use WTFMove(key) here if we get rid of the key below. But if > the removeItem function takes a const String&, then I guess that won’t be > helpful. > > > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:108 > > + auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key, bool quotaException) mutable { > > + if (weakThis) > > + weakThis->didSetItem(storageMapSeed, key, quotaException); > > + }; > > We could capture a copy of the key here and then we would not need the > completion handler to pass the key back to us, saving some XPC. > > > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134 > > + auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key) mutable { > > + if (weakThis) > > + weakThis->didRemoveItem(storageMapSeed, key); > > + }; > > Ditto.
Created attachment 449902 [details] Patch
Committed r288551 (246383@main): <https://commits.webkit.org/246383@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449902 [details].
<rdar://problem/88025071>