We should restrict the change in https://bugs.webkit.org/show_bug.cgi?id=189933 to when resource load statistics is enabled.
<rdar://problem/45349024>
Created attachment 352636 [details] Patch
Created attachment 352637 [details] Patch
Regressions: Unexpected text-only failures (1) http/tests/resourceLoadStatistics/capped-lifetime-for-cookie-set-in-js.html [ Failure ]
(In reply to Chris Dumez from comment #4) > Regressions: Unexpected text-only failures (1) > > http/tests/resourceLoadStatistics/capped-lifetime-for-cookie-set-in-js.html > [ Failure ] I moved the test last thing and forgot to update the relative path to cookie-utilities.js. :( New patch coming.
Created attachment 352654 [details] Patch
Comment on attachment 352654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352654&action=review > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:213 > + if (m_networkProcessCreated) Which network process? The current models is that a single WebsiteDataStore / WebResourceLoadStatisticsStore can be associated with several WebProcessPools and thus several network processes. As a result, this design relying on a single m_networkProcessCreated flag does not seem to make much sense. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1273 > + for (auto& processPool : processPools()) { See how you're iterating over several process pools here? That's right since a single WebsiteDataStore can be associated with several process pools.
(In reply to Chris Dumez from comment #7) > Comment on attachment 352654 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352654&action=review > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:213 > > + if (m_networkProcessCreated) > > Which network process? The current models is that a single WebsiteDataStore > / WebResourceLoadStatisticsStore can be associated with several > WebProcessPools and thus several network processes. > As a result, this design relying on a single m_networkProcessCreated flag > does not seem to make much sense. I see. It may be mostly a naming thing, i.e. the boolean is saying "the first network process has been created so it makes sense to send it data." But what I need to do is to think through how things are marked as "has already been sent to the network process." That's existing code that assumes that all network processes existed when the message went out. Maybe that state doesn't make sense and we should always send the complete set to all network processes. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1273 > > + for (auto& processPool : processPools()) { > > See how you're iterating over several process pools here? That's right since > a single WebsiteDataStore can be associated with several process pools. I think this part is OK. ITP doesn't know how many network processes there are. It only needs to know that the first one has been created. I will go through the code to see how this can make better sense.
Created attachment 352751 [details] Patch
Test failures seem to be text diff output because of the removal of the in-memory-only state.
Comment on attachment 352751 [details] Patch Attachment 352751 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9657774 New failing tests: http/tests/webAPIStatistics/font-load-data-collection.html http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html
Created attachment 352753 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 352751 [details] Patch Attachment 352751 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9659204 New failing tests: http/tests/webAPIStatistics/font-load-data-collection.html http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html
Created attachment 352765 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 352751 [details] Patch Attachment 352751 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9660495 New failing tests: http/tests/webAPIStatistics/font-load-data-collection.html http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html
Created attachment 352771 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 352804 [details] Patch
Comment on attachment 352804 [details] Patch Thanks, Alex (and Chris)!
Comment on attachment 352804 [details] Patch Clearing flags on attachment: 352804 Committed r237304: <https://trac.webkit.org/changeset/237304>
All reviewed patches have been landed. Closing bug.