Minor improvements to StorageAreaMap

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Minor improvements to StorageAreaMap.

[Chris Dumez]

Created attachment 388071 [details]
Patch

[Darin Adler]

Comment on attachment 388071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388071&action=review

> Source/WebCore/storage/StorageMap.h:65
> +    unsigned m_iteratorIndex { UINT_MAX };

We’re typically writing this as std::numeric_limits<unsigned>::max() instead of UINT_MAX.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:84
> +    ASSERT(storageMap.hasOneRef());

I would really like to understand this assertion. Why is it important? Would it be bad if the map has more references? What’s the relevance to the code below?

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:119
> +        RELEASE_LOG_ERROR(Storage, "StorageAreaMap::removeItem fails because storage map ID is invalid");

The use of "fails" here is strange grammar. It should be "failed".

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134
> +        RELEASE_LOG_ERROR(Storage, "StorageAreaMap::clear fails because storage map ID is invalid");

The use of "fails" here is strange grammar. It should be "failed".

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:159
> +            // The StorageManagerSet::GetValues() IPC may be very slow because it may need to fetch the values from disk and there may be a lot of data.

Would be a better comment if it said "need to use unbounded IPC scope because". Otherwise the comment seems oblique. Saying something may be "very slow" but not why that matters to the code below. Maybe to someone more expert it’s more obvious.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:232
> +        for (auto& change : m_pendingValueChanges) {
> +            auto& key = change.key;

We have another idiom for this in HashMap, I think:

    for (auto& key : m_pendingValueChanges.keys()) {

That should work.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:292
> +    for (auto* frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +        auto* document = frame->document();

Consider using Page::forEachDocument instead?

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:322
> +    for (auto* page : pageGroup.pages()) {
> +        for (auto* frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +            auto* document = frame->document();

Consider using Page::forEachDocument instead?

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h:49
> +class StorageAreaMap final : private IPC::MessageReceiver, public CanMakeWeakPtr<StorageAreaMap> {

Various members of this class seem to repeat the word "storage". I would consider making them shorter by leaving that word out since in the context of a "storage area map", you would think "storage" would be implicit context.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h:66
>      // IPC::MessageReceiver
> -    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
> +    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;

Make this private?

> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:37
> +#include <wtf/UniqueRef.h>

Should not add this. It must have been from an earlier refactoring step.

[Chris Dumez]

Comment on attachment 388071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388071&action=review

>> Source/WebCore/storage/StorageMap.h:65
>> +    unsigned m_iteratorIndex { UINT_MAX };
> 
> We’re typically writing this as std::numeric_limits<unsigned>::max() instead of UINT_MAX.

Will fix.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:84
>> +    ASSERT(storageMap.hasOneRef());
> 
> I would really like to understand this assertion. Why is it important? Would it be bad if the map has more references? What’s the relevance to the code below?

I believe it is important because StorageMap uses copy-on-write pattern. If we're not the only ones holding a ref to the StorageMap, then the call to setItem() below would create a new StorageMap and return it. The current code does not deal with this and never updates m_storageMap after calling setItem() on it.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:119
>> +        RELEASE_LOG_ERROR(Storage, "StorageAreaMap::removeItem fails because storage map ID is invalid");
> 
> The use of "fails" here is strange grammar. It should be "failed".

Will fix.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134
>> +        RELEASE_LOG_ERROR(Storage, "StorageAreaMap::clear fails because storage map ID is invalid");
> 
> The use of "fails" here is strange grammar. It should be "failed".

Will fix.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:159
>> +            // The StorageManagerSet::GetValues() IPC may be very slow because it may need to fetch the values from disk and there may be a lot of data.
> 
> Would be a better comment if it said "need to use unbounded IPC scope because". Otherwise the comment seems oblique. Saying something may be "very slow" but not why that matters to the code below. Maybe to someone more expert it’s more obvious.

Ok, will fix.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:232
>> +            auto& key = change.key;
> 
> We have another idiom for this in HashMap, I think:
> 
>     for (auto& key : m_pendingValueChanges.keys()) {
> 
> That should work.

I had tried that but it did not work. m_pendingValueChanges is a HashCountedSet, not a HashMap.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:292
>> +        auto* document = frame->document();
> 
> Consider using Page::forEachDocument instead?

Oh, I was not familiar with this one, I will look into using it.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:322
>> +            auto* document = frame->document();
> 
> Consider using Page::forEachDocument instead?

Ok

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h:49
>> +class StorageAreaMap final : private IPC::MessageReceiver, public CanMakeWeakPtr<StorageAreaMap> {
> 
> Various members of this class seem to repeat the word "storage". I would consider making them shorter by leaving that word out since in the context of a "storage area map", you would think "storage" would be implicit context.

Ok.

>> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h:66
>> +    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
> 
> Make this private?

iirc, I could not make that change because there is a call site calling didReceiveMessage() on a StorageAreaMap instance. I did not want to add a cast to base class at call site.

>> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:37
>> +#include <wtf/UniqueRef.h>
> 
> Should not add this. It must have been from an earlier refactoring step.

Indeed, thanks.

[Chris Dumez]

Created attachment 388306 [details]
Patch

[Chris Dumez]

Comment on attachment 388306 [details]
Patch

Clearing flags on attachment: 388306

Committed r254859: <https://trac.webkit.org/changeset/254859>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58762680>