Bug 177002 – Use vector map routine in WebCore CacheStorage implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

For increased readability

[youenn fablet]

Created attachment 320902 [details]
Patch

[Build Bot]

Attachment 320902 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:150:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320907 [details]
Patch

[Build Bot]

Attachment 320907 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:130:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320908 [details]
Patch

[Build Bot]

Attachment 320908 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:130:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320911 [details]
Patch

[Build Bot]

Attachment 320911 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:130:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 320911 [details]
Patch

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

> Source/WebCore/Modules/cache/CacheStorage.cpp:77
> +static inline Ref<DOMCache> copyCache(const Ref<DOMCache>& cache)

This is annoying; just because Ref doesn’t like to be copied without an explicit call, eh?

> Source/WebCore/Modules/cache/CacheStorage.cpp:130
> +    auto position = m_caches.findMatching([&] (const auto& cache) { return info.identifier == cache->identifier(); });

Seems like we have the wrong data structure here if we have to loop over all of these.

[youenn fablet]

(In reply to Darin Adler from comment #9)
> Comment on attachment 320911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320911&action=review
> 
> > Source/WebCore/Modules/cache/CacheStorage.cpp:77
> > +static inline Ref<DOMCache> copyCache(const Ref<DOMCache>& cache)
> 
> This is annoying; just because Ref doesn’t like to be copied without an
> explicit call, eh?

Yes, this is one of the negative side of uncopyable Ref.
The other workaround would be to make Vector<Ref<>> copyable.

> 
> > Source/WebCore/Modules/cache/CacheStorage.cpp:130
> > +    auto position = m_caches.findMatching([&] (const auto& cache) { return info.identifier == cache->identifier(); });
> 
> Seems like we have the wrong data structure here if we have to loop over all
> of these.

The spec mandates to keep the order of the keys (cache names) hence why we are using a Vector.

We can probably go with a HashMap though and sorting the keys when needed. That may be a nice follow-up.

[WebKit Commit Bot]

Comment on attachment 320911 [details]
Patch

Rejecting attachment 320911 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 320911, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
king copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file Source/WebCore/Modules/cache/DOMCache.cpp
patching file Source/WebCore/Modules/cache/DOMCache.h
patching file Source/WebCore/Modules/cache/DOMCacheEngine.cpp
patching file Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4725818

[Darin Adler]

Comment on attachment 320911 [details]
Patch

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

>>> Source/WebCore/Modules/cache/CacheStorage.cpp:130
>>> +    auto position = m_caches.findMatching([&] (const auto& cache) { return info.identifier == cache->identifier(); });
>> 
>> Seems like we have the wrong data structure here if we have to loop over all of these.
> 
> The spec mandates to keep the order of the keys (cache names) hence why we are using a Vector.
> 
> We can probably go with a HashMap though and sorting the keys when needed. That may be a nice follow-up.

ListHashSet implements this kind of data structure, a set with an ordering on its members. We can make a variant that is a map as well as the one that is a set. But perhaps the memory use or other performance properties would be unacceptable for this use?

[WebKit Commit Bot]

Comment on attachment 320911 [details]
Patch

Rejecting attachment 320911 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 320911, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
king copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file Source/WebCore/Modules/cache/DOMCache.cpp
patching file Source/WebCore/Modules/cache/DOMCache.h
patching file Source/WebCore/Modules/cache/DOMCacheEngine.cpp
patching file Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4725882

[youenn fablet]

Created attachment 322440 [details]
Patch for landing

[Build Bot]

Attachment 322440 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:130:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:132:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 322440 [details]
Patch for landing

Clearing flags on attachment: 322440

Committed r222774: <http://trac.webkit.org/changeset/222774>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34786553>