[webkit-reviews] review granted: [Bug 236263] http/tests/cache-storage/cache-origins.https.html is flaky : [Attachment 451168] 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 236263: http/tests/cache-storage/cache-origins.https.html is flaky
https://bugs.webkit.org/show_bug.cgi?id=236263

Attachment 451168: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 451168
  --> https://bugs.webkit.org/attachment.cgi?id=451168
Patch

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

I like this as is, but I have some thoughts on little style tweaks.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:796
>      builder.append("{ \"path\": \"");
>      builder.append(m_rootPath);
>      builder.append("\", \"origins\": [");

We can merge these all into one line.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:797
> +    Vector<String> origins = WTF::map(m_caches, [](auto& keyValue) {

Could use auto here instead of Vector<String>. I think it would look a little
better if it’s built even before we define StringBuilder.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:803
> +	   originBuilder.append("\n{ \"origin\" : { \"topOrigin\" : \"");
> +	   originBuilder.append(keyValue.key.topOrigin.toString());
> +	   originBuilder.append("\", \"clientOrigin\": \"");
> +	   originBuilder.append(keyValue.key.clientOrigin.toString());
> +	   originBuilder.append("\" }, \"caches\" : ");

We can merge these all into fewer lines.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:811
> +    for (auto& origin : origins) {

Another way to write this:

    const char* divider = "";
    for (auto& origin : origins) {
	builder.append(divider, origin);
	divider = ",";
    }

A little less wordy than the isFirst version.