...
rdar://80891555
Created attachment 458707 [details] Patch
Comment on attachment 458707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458707&action=review Nice > LayoutTests/storage/domstorage/sessionstorage/resources/window-open-remove-item.html:3 > +<script src="../../../../resources/js-test.js"></script> Is this really needed?
Created attachment 458749 [details] Patch for landing
Committed r293736 (250224@main): <https://commits.webkit.org/250224@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458749 [details].
Comment on attachment 458749 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review > Source/WebCore/storage/StorageMap.cpp:141 > + m_impl->map.remove(key); Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.
Comment on attachment 458749 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review >> Source/WebCore/storage/StorageMap.cpp:141 >> + m_impl->map.remove(key); > > Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice. I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove. (What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.)
(In reply to Sihui Liu from comment #7) > Comment on attachment 458749 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458749&action=review > > >> Source/WebCore/storage/StorageMap.cpp:141 > >> + m_impl->map.remove(key); > > > > Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice. > > I don't quite understand the proposal. Are you suggesting replace remove() > with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const > KeyType& key) is find and remove. > > (What StorageMap::removeItem does now is to makes a copy of map if the map > is shared and key exists, and removes key from the copy; the original map is > not changed.) As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap).
Comment on attachment 458749 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review >>>> Source/WebCore/storage/StorageMap.cpp:141 >>>> + m_impl->map.remove(key); >>> >>> Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice. >> >> I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove. >> >> (What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.) > > As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap). I see; the take could only be done if the reference count was 1. I would have written: if (m_impl->hasOneRef()) m_impl->map.remove(iter); else { m_impl = m_impl->copy(); m_impl->map.remove(key); }
(In reply to Darin Adler from comment #9) > Comment on attachment 458749 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458749&action=review > > >>>> Source/WebCore/storage/StorageMap.cpp:141 > >>>> + m_impl->map.remove(key); > >>> > >>> Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice. > >> > >> I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove. > >> > >> (What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.) > > > > As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap). > > I see; the take could only be done if the reference count was 1. I would > have written: > > if (m_impl->hasOneRef()) > m_impl->map.remove(iter); > else { > m_impl = m_impl->copy(); > m_impl->map.remove(key); > } Right, this would work and be a bit more efficient in the probably common case where the StorageMap isn't shared. I like it.
Committed r294124 (250502@trunk): <https://commits.webkit.org/250502@trunk>