Bug 195474

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 Flags
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2 none

Description John Wilander 2019-03-08 12:11:21 PST
There are scenarios where localhost should not be classified as prevalent. We should support that.
Comment 1 John Wilander 2019-03-08 12:11:33 PST
<rdar://problem/47520577>
Comment 2 John Wilander 2019-03-08 13:32:43 PST
Created attachment 364055 [details]
Patch
Comment 3 Brent Fulgham 2019-03-08 13:47:12 PST
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.
Comment 4 John Wilander 2019-03-08 14:36:00 PST
(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.
Comment 5 John Wilander 2019-03-08 15:19:01 PST
Created attachment 364082 [details]
Patch
Comment 6 John Wilander 2019-03-08 18:19:09 PST
Test failures look unrelated.
Comment 7 EWS Watchlist 2019-03-08 18:22:57 PST
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
Comment 8 EWS Watchlist 2019-03-08 18:22:59 PST
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
Comment 9 Brent Fulgham 2019-03-11 09:38:29 PDT
(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 10 Brent Fulgham 2019-03-11 09:41:38 PDT
Comment on attachment 364082 [details]
Patch

r=me
Comment 11 John Wilander 2019-03-11 09:53:59 PDT
Thanks, Brent!
Comment 12 WebKit Commit Bot 2019-03-11 10:01:33 PDT
Comment on attachment 364082 [details]
Patch

Clearing flags on attachment: 364082

Committed r242712: <https://trac.webkit.org/changeset/242712>
Comment 13 WebKit Commit Bot 2019-03-11 10:01:35 PDT
All reviewed patches have been landed.  Closing bug.