[Reland] [Site Familiarity] Skip checks for same-site navigations [chromium/src : main]

0 views

Skip to first unread message

Javier Castro (Gerrit)

unread,

6:22 PM (3 hours ago) 6:22 PM

to Varun Khaneja, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Varun Khaneja

Javier Castro added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Javier Castro . resolved

Hi vakh@,

Trying this again. I've removed the browsertests because:

1. The query counting approach was brittle, it had different queries on linux-chromeos-rel versus linux-chromeos-chrome (unbranded versus branded builds) and if we ever made changes to SB queries, these tests might break.
2. Another approach I thought of was using histograms but the guidelines say we shouldn't be testing functionality of features using histograms (testing histogram functionality is of course ok).
3. I drafted some tests but the skip behavior isn't observable and the ultimate outcomes for scenarios are also impacted by the browsinginstance cache implementation. Basically, I couldn't check that the familiarity checks were skipped because the observable result of the jsopt state was the same as when the feature didn't exist.

I think the unit tests should be enough test support for now. If you have another idea I'm happy to try it, but I think it's more important that this is good rather than perfect.

Thanks for the review!

By the way, I filed crbug.com/537858159 to track the iframed sandbox issue you pointed out.

Open in Gerrit

Related details

Attention is currently required from:

Varun Khaneja

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Varun Khaneja (Gerrit)

unread,

7:18 PM (2 hours ago) 7:18 PM

to Javier Castro, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Javier Castro

Varun Khaneja added 1 comment

File chrome/browser/site_protection/site_familiarity_process_selection_deferring_condition_unittest.cc

Line 1103, Patchset 2 (Latest):class SiteFamiliaritySameSiteSkipFamiliarityCheckTest

Varun Khaneja . unresolved

The tests added for same-site skip logic only cover the case where V8 optimizations are enabled (which defaults to `kFamiliar` verdict). Would it be beneficial to add test coverage for the case where V8 optimizations are disabled, which should result in a `kUnfamiliar` verdict?