When activating resource load statistics, we should avoid forcing the creation of a new process pool if it doesn't yet exist. We will properly set the value when initializing the first web view, so there is no point in paying the construction cost for the process pool during initial launch.
<rdar://problem/35545639>
Created attachment 328382 [details] Patch
Comment on attachment 328382 [details] Patch Reading the code, I don't understand what it does or what it means. It's very not straightforward and if I wrote more code I would make this same mistake. This has no tests.
(In reply to Alex Christensen from comment #3) > Comment on attachment 328382 [details] > Patch > > Reading the code, I don't understand what it does or what it means. It's > very not straightforward and if I wrote more code I would make this same > mistake. > This has no tests. I'm not sure I follow the r- here. You're saying the patch is bad because it uses the existing (but confusing) API to fix the bug I was trying to resolve? It might be clearer if you said "please create a brand new API that is not confusing". The functionality of activating resource load statistics is covered by existing tests, which should not fail after this patch. We do not have testing infrastructure (supported by our LayoutTest) for measuring startup cost, which is what this is trying to resolve.
Comment on attachment 328382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328382&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1323 > + for (auto& processPool : processPools(1, false)) You can make this code clearer by putting 1 and false into local named variables that reflect their meanings. It's weird to write this code as a loop since the 1 you've passed to processPools guarantees there will be only one item. Maybe you can just reuse the processPoolForCookieStorageOperations() function here. But you're not doing cookie operations, so it needs a clearer name for its purpose. Maybe "anyProcessPool()".
Created attachment 328386 [details] Patch
Comment on attachment 328382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328382&action=review >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1323 >> + for (auto& processPool : processPools(1, false)) > > You can make this code clearer by putting 1 and false into local named variables that reflect their meanings. > > It's weird to write this code as a loop since the 1 you've passed to processPools guarantees there will be only one item. > > Maybe you can just reuse the processPoolForCookieStorageOperations() function here. But you're not doing cookie operations, so it needs a clearer name for its purpose. Maybe "anyProcessPool()". Yeah -- this is wrong. I do want to get whatever pools exist -- I just don't want to create a process pool if it doesn't exist yet. I was thinking about making a new method wrapper called "existingProcessPools()" that would do this: return processPools(std::numeric_limits<size_t>::max(), false);
Created attachment 328388 [details] Patch
Committed r225508: <https://trac.webkit.org/changeset/225508>
Still think it would be nice to name the arguments here.