Bug 195474 – Resource Load Statistics: Make it possible to exclude localhost from classification

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

There are scenarios where localhost should not be classified as prevalent. We should support that.

[John Wilander]

<rdar://problem/47520577>

[John Wilander]

Created attachment 364055 [details]
Patch

[Brent Fulgham]

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.

[John Wilander]

(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.

[John Wilander]

Created attachment 364082 [details]
Patch

[John Wilander]

Test failures look unrelated.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

(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.

[Brent Fulgham]

Comment on attachment 364082 [details]
Patch

r=me

[John Wilander]

Thanks, Brent!

[WebKit Commit Bot]

Comment on attachment 364082 [details]
Patch

Clearing flags on attachment: 364082

Committed r242712: <https://trac.webkit.org/changeset/242712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.