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.

Sign up for GitHub

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

Jump to bottom

Allow users to be able to change content settings set by shield extension. #439

Merged
merged 3 commits into from
Sep 29, 2018
Merged

Allow users to be able to change content settings set by shield extension. #439

merged 3 commits into from
Sep 29, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Sep 12, 2018 •
edited
Loading

If a setting is not managed by user, editing isn't allowed by the most of user interface.
Because of this reason, users can't edit javascript block settings from settings webui or page info bubble.

To enable editing the shields setting, newly introduced BraveShields.ContentSetting will store shields's content settings to user preference store.

Note: We are using two different apis for storing shields's setting.
In shields, ContentsSettingsStore::SetExtensionContentSetting is used by ContentSettingsContentSettingSetFunction.
In other places, methods of HostContentSettingsMap are used.
Each uses different persistent storage.
With this PR, we will use one api -HostContentSettingsMap for setting.

This PR doesn't change any behavior w/o brave/brave-extension#63.

Issue: brave/brave-browser#232

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

yarn test brave_browser_tests --filter=BraveShieldsAPIBrowserTest.ContentSetting*

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@simonhong simonhong self-assigned this Sep 12, 2018
@simonhong simonhong force-pushed the brave_shields_content_settings branch 8 times, most recently from 3dfed08 to 86913dc Compare September 19, 2018 03:51
@simonhong simonhong changed the title WIP: Allow users to be able to change content settings set by shield extension. Allow users to be able to change content settings set by shield extension. Sep 19, 2018
@simonhong
Copy link
Member Author

simonhong commented Sep 19, 2018 •
edited
Loading

@yrliou BraveContentSettingsStore is valid when shields doesn't use chrome.contentSettings api?
With this PR, shields's settings will not be stored via ContentSettingsStore anymore.

This was referenced Sep 20, 2018
Merged
Closed
@yrliou
Copy link
Member

yrliou commented Sep 24, 2018 •
edited
Loading

@yrliou BraveContentSettingsStore is valid when shields doesn't use chrome.contentSettings api?
With this PR, shields's settings will not be stored via ContentSettingsStore anymore.

They're not needed if we're not using content settings store anymore.

@bsclifton
Copy link
Member

This PR might also fix brave/brave-browser#1198 - going to build locally and verify 😄

@bsclifton
Copy link
Member

Confirmed this does NOT fix brave/brave-browser#1198 ☹️ I thought that perhaps since a full schema (that includes Scope) for the extension was provided, it might treat the content settings as per-session (which it doesn't seem to)

bbondy
bbondy previously approved these changes Sep 26, 2018
View reviewed changes
@bbondy bbondy dismissed their stale review September 27, 2018 04:01

Spending some more time with this given global shield settings landed.

@simonhong
Copy link
Member Author

Rebased and BraveContentSettingsStore is deleted.

@simonhong @bbondy
Add Get/Set contentSettings api to BraveShields
8697769 
Newly addied apis set content settings to user preference instead of
ContentSettingsStore.
Also, previously set value in extension's ContentSettingsStore is
deleted when user changes current setting.
@simonhong @bbondy
Remove BraveContentSettingsStore
28e1ef0 
brave-extension doesn't use ContentSettingsStore for shields configuration.
@bbondy bbondy force-pushed the brave_shields_content_settings branch from 040a3a2 to 28e1ef0 Compare September 28, 2018 19:47
@bbondy
Copy link
Member

bbondy commented Sep 28, 2018

Force pushed rebase on master tip fyi.

@bbondy bbondy merged commit d160950 into master Sep 29, 2018
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

Code looks good, tested and it works great too. Tested against default shield settings and it is good too.

bbondy added a commit that referenced this pull request Sep 29, 2018
@bbondy
Merge pull request #439 from brave/brave_shields_content_settings
8de1937 
Allow users to be able to change content settings set by shield extension.
bbondy added a commit that referenced this pull request Sep 29, 2018
@bbondy
Merge pull request #439 from brave/brave_shields_content_settings
fd63dd2 
Allow users to be able to change content settings set by shield extension.
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

master: d160950
0.56.x: 8de1937
0.55.x: fd63dd2

bbondy added a commit that referenced this pull request Sep 29, 2018
@bbondy
Update deps for #439
f8150eb 
Auditors: @simonhong
bbondy added a commit that referenced this pull request Sep 29, 2018
@bbondy
Update deps for #439
cd5eff8 
Auditors: @simonhong
bbondy added a commit that referenced this pull request Sep 29, 2018
@bbondy
Update deps for #439
cab9e09 
Auditors: @simonhong
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

And DEPs updated commit here:
master: f8150eb
0.56.x: cd5eff8
0.55.x: cab9e09

@simonhong simonhong deleted the brave_shields_content_settings branch September 29, 2018 21:37
@simonhong simonhong mentioned this pull request Oct 30, 2018
Merged
18 tasks
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
fmarier pushed a commit that referenced this pull request Oct 29, 2019
@mbacchi
Merge pull request #439 from brave/bump_version
cf481e3 
Bump version (v0.50.6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy approved these changes

@yrliou yrliou Awaiting requested review from yrliou

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
0.55.x - Release
Development

Successfully merging this pull request may close these issues.
4 participants
@simonhong @yrliou @bsclifton @bbondy