Skip to content
This repository has been archived by the owner on Jun 6, 2019. It is now read-only.

Add scope to methods which modify content settings #68

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Oct 1, 2018

Fixes brave/brave-browser#1198

Known issue: content settings from public windows are NOT inherited into private windows. ex: if you visit https://www.dailymail.co.uk and disable shields in a tab and then open a new private window, that shields off setting is not propagated to the private window. This is a separate bug that needs to be tracked which was not caused by this PR.

If this PR is approved/merged, I will create an issue to track that problem

Reviewer setup steps

  1. Get latest brave-browser (master branch)
  2. Update package.json to have branch content-settings-add-scope for brave-core
  3. Run npm run init or npm run sync -- --all
  4. Build with npm run build
  5. Launch with npm start

Test plan

Part 1: Test that shields down/up works fine

  1. Have a fresh profile and launch Brave
  2. Open a new private window
  3. Navigate to https://www.dailymail.co.uk
  4. Click the shields icon in the URL bar
  5. Turn shields off and ensure page reloads and that ads are shown (icon should be gray)
  6. Open a new tab in this private window and visit the same URL (https://www.dailymail.co.uk)
  7. Verify shields are already be down, because content settings have been applied for this session.
  8. Close this second tab and go back to original tab (from step 3)
  9. Turn shields back on and ensure page reloads and that ads are NOT shown (icon should have block count)

Part 2: test other content_settings

  1. Repeat steps 3 - 9, but for the other specific content_settings (Ad Control, Cookie Control, Fingerprinting Protection, HTTPS Everywhere, Block Scripts)

Part 3: verify that settings are NOT persisted

  1. Make a specific settings change, like turning off shields for https://www.dailymail.co.uk
  2. Close the private window and open a new private window
  3. Navigate again to https://www.dailymail.co.uk
  4. Verify that the change you made in step 11 didn't persist
  5. Exit browser
  6. Re-launch Brave and open a new private window
  7. Navigate again to https://www.dailymail.co.uk
  8. Verify that the change you made in step 11 didn't persist

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

@bsclifton bsclifton merged commit 0a62825 into master Oct 1, 2018
@bsclifton bsclifton deleted the content-settings-add-scope branch October 1, 2018 19:15
bsclifton added a commit to brave/brave-core that referenced this pull request Oct 1, 2018
@bsclifton
Copy link
Member Author

Follow up issue (per my notes above) created:
brave/brave-browser#1373

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

Successfully merging this pull request may close these issues.

2 participants