Use a strongly typed identifier for StorageNamespace's identifier instead of uint64_t.
Created attachment 376705 [details] Patch
ping review?
Comment on attachment 376705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376705&action=review > Source/WebCore/storage/StorageNamespace.h:45 > + virtual Ref<StorageNamespace> copy(Page* newPage) = 0; // Only valid for session storage. Comment is not really clear to me. I am not sure how it relates to the current patch. If we want to precise the meaning of copy, maybe we should precise the name of the method? > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:195 > +StorageArea* StorageManager::createLocalStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue) We can probably remove WebCore:: here. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:205 > +StorageArea* StorageManager::createTransientLocalStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& topLevelOrigin, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue) Ditto. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:216 > +StorageArea* StorageManager::createSessionStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue) Ditto. > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:114 > + return makeObjectIdentifier<PageIdentifierType>(m_storageNamespaceID.toUInt64()); This is preexisting code but seems a bit odd to me. > Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp:54 > return storageNamespaceProvider; HashMap::ensure maybe
Comment on attachment 376705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376705&action=review >> Source/WebCore/storage/StorageNamespace.h:45 >> + virtual Ref<StorageNamespace> copy(Page* newPage) = 0; // Only valid for session storage. > > Comment is not really clear to me. I am not sure how it relates to the current patch. > If we want to precise the meaning of copy, maybe we should precise the name of the method? I noticed while writing this patch (because it was made obvious by the new code factoring) that the implementation of this function is only correct if the StorageNamespace object is for session storage. If the StorageNamespace object is used for local storage, then it does not do the right thing. I think that ideally, this would be moved to a subclass (e.g. SessionStorageNamespace which we would need to create) but this is a bit out of scope of this patch, so I merely put in a comment.
Maybe a FIXME would be more adequate then.
Created attachment 376773 [details] Patch
Comment on attachment 376773 [details] Patch Clearing flags on attachment: 376773 Committed r248901: <https://trac.webkit.org/changeset/248901>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54514978>
Comment on attachment 376773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376773&action=review > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:120 > + ASSERT(m_storageType == StorageType::Local || m_storageType == StorageType::TransientLocal); > + return m_storageNamespaceID.toUInt64(); Why is this returning the m_storageNamespaceID as a page group identifier?
Comment on attachment 376773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376773&action=review >> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:120 >> + return m_storageNamespaceID.toUInt64(); > > Why is this returning the m_storageNamespaceID as a page group identifier? This is among the many reasons I've been working to remove page groups from WebKit2.