Bug 200895 – Use a strongly typed identifier for StorageNamespace's identifier

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Use a strongly typed identifier for StorageNamespace's identifier instead of uint64_t.

[Chris Dumez]

Created attachment 376705 [details]
Patch

[Chris Dumez]

ping review?

[youenn fablet]

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

[Chris Dumez]

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.

[youenn fablet]

Maybe a FIXME would be more adequate then.

[Chris Dumez]

Created attachment 376773 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376773 [details]
Patch

Clearing flags on attachment: 376773

Committed r248901: <https://trac.webkit.org/changeset/248901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54514978>

[Simon Fraser (smfr)]

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?

[Alex Christensen]

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.