[webkit-reviews] review granted: [Bug 206433] Minor improvements to StorageAreaMap : [Attachment 388071] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 206433: Minor improvements to StorageAreaMap
https://bugs.webkit.org/show_bug.cgi?id=206433

Attachment 388071: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 388071
  --> https://bugs.webkit.org/attachment.cgi?id=388071
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.