Summary: | Resource Load Statistics: Make it possible to exclude localhost from classification | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, darin, ews-watchlist, rniwa, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
John Wilander
2019-03-08 12:11:21 PST
Created attachment 364055 [details]
Patch
Comment on attachment 364055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364055&action=review Looks like you neglected to update the SQLite code paths, so r- to fix that. It also seems like you adjusted a number of test cases. Was that done because it's needed for this localhost behavior? Or did you notice the cause of test flakiness and decide to fix them at the same time? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:72 > + , m_shouldIncludeLocalhost { shouldIncludeLocalhost } It seems like this should be part of the base class, rather than defining and setting in the two children. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:180 > +} This whole method should be in the base class. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:186 > + continue; You should be making these same changes in the ResourceLoadStatisticsDatabaseStore class, too. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:471 > + return; Ditto. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:502 > + return false; Ditto. (In reply to Brent Fulgham from comment #3) > Comment on attachment 364055 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364055&action=review > > Looks like you neglected to update the SQLite code paths, so r- to fix that. Oh, I assumed classification only happened in the memory store. I'm not clear on why our persistence layer needs to do anything more than store and restore the memory store's view of the world. > It also seems like you adjusted a number of test cases. Was that done > because it's needed for this localhost behavior? Or did you notice the cause > of test flakiness and decide to fix them at the same time? I commented on this in the layout test change log. Test cases that set localhost as prevalent need to tell WebKit that localhost should be included. This is done through the new testRunner.setStatisticsIsRunningTest(bool). The changes in test cases are to make sure they all use setEnableFeature() in /resourceLoadStatistics/resources/util.js which does the work for them, asynchronously and all that good stuff. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:72 > > + , m_shouldIncludeLocalhost { shouldIncludeLocalhost } > > It seems like this should be part of the base class, rather than defining > and setting in the two children. If it's to be used in the DB class, then yes. :) > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:180 > > +} > > This whole method should be in the base class. Ditto. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:186 > > + continue; > > You should be making these same changes in the > ResourceLoadStatisticsDatabaseStore class, too. Yup. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:471 > > + return; > > Ditto. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:502 > > + return false; > > Ditto. Thanks! Will fix. Created attachment 364082 [details]
Patch
Test failures look unrelated. Comment on attachment 364082 [details] Patch Attachment 364082 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11434183 New failing tests: accessibility/mac/selection-notification-focus-change.html Created attachment 364102 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
(In reply to John Wilander from comment #4) > > Looks like you neglected to update the SQLite code paths, so r- to fix that. > > Oh, I assumed classification only happened in the memory store. I'm not > clear on why our persistence layer needs to do anything more than store and > restore the memory store's view of the world. The SQLite backend uses SQLite to store the data at all times, rather than having an in-memory HashTable that is periodically synced to disk. In effect, the SQLite implementation is a combination of the MemoryStore and Persistent store. Comment on attachment 364082 [details]
Patch
r=me
Thanks, Brent! Comment on attachment 364082 [details] Patch Clearing flags on attachment: 364082 Committed r242712: <https://trac.webkit.org/changeset/242712> All reviewed patches have been landed. Closing bug. |