Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move cookie list override logic to account for uninitialized preferences #11730

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

antonok-edm
Copy link
Collaborator

The previous implementation only iterated through lists that existed in the preferences already, so in practice it would only override lists that had enabled: false - i.e. lists that had been enabled and disabled again before the "touched" preference was introduced.

This fix clones the preferences dictionary and adds the cookie list with enabled: true to the cloned dictionary if the override should occur.

Resolves brave/brave-browser#20308

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Same as #11621

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just had one minor comment - up to you if you make that change, though.

The previous implementation only iterated through lists that existed in
the preferences already, so in practice it would only override lists
that had `enabled: false` - i.e. lists that had been enabled and
disabled again before the "touched" preference was introduced.

This fix clones the preferences dictionary and adds the cookie list with
`enabled: true` to the cloned dictionary if the override should occur.
@antonok-edm
Copy link
Collaborator Author

Android CI failure is unrelated, merging.

@antonok-edm antonok-edm merged commit e81869f into master Jan 4, 2022
@antonok-edm antonok-edm deleted the cookielist-default-hotfix branch January 4, 2022 07:26
@antonok-edm antonok-edm added this to the 1.36.x - Nightly milestone Jan 4, 2022
brave-builds pushed a commit that referenced this pull request Jan 4, 2022
@stephendonner
Copy link
Contributor

Verified PASSED using

Brave 1.36.14 Chromium: 97.0.4692.71 (Official Build) nightly (64-bit)
Revision adefa7837d02a07a604c1e6eff0b3a09422ab88d-refs/branch-heads/4692@{#1247}
OS Windows 10 Version 20H2 (Build 19042.1415)

Followed the testplan from #11621.

Steps:

  1. Visit https://digikey.com and confirm that a cookie notice is shown. It will appear either as a banner at the top, or as a popup along the bottom depending on window size.
  2. Visit brave://flags and enable the #brave-adblock-cookie-list-default flag, then restart the browser.
  3. Visit https://digikey.com and confirm that no cookie notice is shown.
  4. Visit brave://adblock and confirm that the entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices under Additional filters exists and is checked.
  5. Return to brave://flags and set #brave-adblock-cookie-list-default to Disabled, and restart the browser.
  6. Visit https://digikey.com and confirm that the cookie notice is shown again.
  7. Visit brave://adblock and confirm that the entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is no longer checked.
  8. Manually toggle the Easylist-Cookie List - Filter Obtrusive Cookie Notices entry to be enabled, then repeat steps 2-5.
  9. Visit brave://adblock and confirm that the entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is checked.
  10. Manually toggle the Easylist-Cookie List - Filter Obtrusive Cookie Notices entry to be disabled.
  11. Visit brave://flags and enable the #brave-adblock-cookie-list-default flag, then restart the browser.
  12. Visit brave://adblock and confirm that the entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is unchecked.
  13. Visit https://digikey.com and confirm that the cookie notice is shown.
step1 step 2 step 3 step 4 step 5 step 6 step 7
PR-11730-2 PR-11730-1 PR-11730-3 PR-11730-4 PR-11730-5 PR-11730-6 PR-11730-7
step 8a step 8b step 8c step 9 step 10 step 11 step 12 step 13
PR-11730-8 PR-11730-9 PR-11730-10 PR-11730-11 PR-11730-12 PR-11730-13 PR-11730-14 PR-11730-15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie list default flag doesn't work in some cases
3 participants