http/tests/cache-storage/cache-origins.https.html is flaky because it relies on a particular HashMap ordering.
Created attachment 451157 [details] Patch
Comment on attachment 451157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451157&action=review We should do this sorting inside WebCore, I think. > LayoutTests/http/tests/cache-storage/cache-origins.https.html:23 > + representation.sort((a, b) => { return a.origin.clientOrigin > b.origin.clientOrigin; }); This isn’t right. It’s supposed to be a three way sort function, and needs to return -1, 0, +1 based on the ordering. Not like C++ standard library where they used boolean less than style sorting. > LayoutTests/http/tests/cache-storage/cache-origins.https.html:36 > + representation.sort((a, b) => { return a.origin.clientOrigin > b.origin.clientOrigin; }); Ditto.
Created attachment 451163 [details] Patch
Comment on attachment 451163 [details] Patch I still think we should fix this inside WebCore, but this is a good fix. We should also take out the "value_or(0)" we added to work around it.
Created attachment 451167 [details] Patch
Alternative approach, fixing it on native side, as suggested.
Created attachment 451168 [details] Patch
Comment on attachment 451168 [details] 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.
Created attachment 451243 [details] Patch
Committed r289419 (246982@main): <https://commits.webkit.org/246982@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451243 [details].
<rdar://problem/88648506>