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

Hide settings which do not apply to Brave #493

Merged
merged 2 commits into from
Sep 25, 2018
Merged

Hide settings which do not apply to Brave #493

merged 2 commits into from
Sep 25, 2018

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Sep 25, 2018

3rd party cookies is per site and block by default in Brave

We use our own ad block, not Google's adblock-ish, but not really
adblock.

Fix brave/brave-browser#402
Fix brave/brave-browser#1119

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:

a. ads

  1. Go to chrome://settings/content
  2. The ads section shouldn't exist

screen shot 2018-09-24 at 21 20 57

  1. Go to chrome://settings/content/ads
  2. It will be redirected to chrome://settings

b. javascript

  1. Go to chrome://settings/content/javascript
  2. Allow toggle shouldn't be there

screen shot 2018-09-24 at 20 52 39

c. cookie

  1. Go to chrome://settings/content/cookies
  2. blocked third-party toggle should disappear

screen shot 2018-09-24 at 20 52 24

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

3rd party cookies is per site and block by default in Brave

We use our own ad block, not Google's adblock-ish, but not really
adblock.

Fix brave/brave-browser#402
Fix brave/brave-browser#1119
@bbondy bbondy self-assigned this Sep 25, 2018
@bbondy bbondy requested a review from darkdh September 25, 2018 03:10
</paper-icon-button-light>
</div>
</template>
+</if>
Copy link
Member

Choose a reason for hiding this comment

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

can we merge the /if and if google_chrome here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was intentionally separate here because then if something is added in between it won't be hidden by the patch

@darkdh
Copy link
Member

darkdh commented Sep 25, 2018

do we need to block access to ad page when user type chrome://settings/content/ads in the url bar directly?

@darkdh
Copy link
Member

darkdh commented Sep 25, 2018

In order to achieve above comment, we have to modify chrome/browser/resources/settings/route.js

@bbondy
Copy link
Member Author

bbondy commented Sep 25, 2018

I don't think it's a big deal, I just don't want it to show up from browsing settings.

darkdh
darkdh approved these changes Sep 25, 2018
@darkdh darkdh merged commit fcefe04 into master Sep 25, 2018
darkdh added a commit that referenced this pull request Sep 25, 2018
Hide settings which do not apply to Brave
@darkdh
Copy link
Member

darkdh commented Sep 25, 2018

master: fcefe04
0.55.x: ccab82d


// TODO(tommycli): Find a way to refactor these repetitive category
// routes.
+<if expr="_google_chrome">
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't this have a comment indicator before it? The other things in this file do.

@bsclifton bsclifton deleted the hide-settings branch September 26, 2018 05:43
@bbondy
Copy link
Member Author

bbondy commented Sep 26, 2018

Added label merged/0.55.x

@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
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.

Hide Chromium Ads setting that conflicts with Brave ad blocking Remove Global Javascript and Cookie blocking
2 participants