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

Disable motion sensors and background sync by default #2802

Merged
merged 3 commits into from
Sep 23, 2019

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Jun 25, 2019

Fixes brave/brave-browser#4709 and brave/brave-browser#4789.

Submitter Checklist:

Test Plan:

For background sync:

  1. Load https://deanhume.github.io/Service-Workers-BackgroundSync/
  2. Open the devtools console.
  3. Press the "Make an HTTP request - do it!" button.
  4. Look for "Unable to fetch image." in the console (and not "sync registered").

For either feature:

  1. Load https://brave.com
  2. Click the padlock icon in the URL bar.
  3. Click "Site settings".
  4. Look for "Motion sensors" and "Background sync". They should both be labeled "Block (default)".

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

After-merge Checklist:

@fmarier fmarier requested a review from bsclifton June 25, 2019 21:29
@fmarier fmarier self-assigned this Jun 25, 2019
@bsclifton
Copy link
Member

bsclifton commented Jun 25, 2019

Adding @bridiver to review, since this involves patching

Taking a look myself, not sure how to avoid patching in a clean way (as Register will throw a DCHECK if you try to call it twice with the same key)

bsclifton
bsclifton previously approved these changes Jun 26, 2019
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Test plan works great! Change looks good to me. Let's wait until @bridiver can review before merging 😄

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

see comments

@bsclifton
Copy link
Member

cc: @fmarier per the feedback above (also needs a rebase)

@fmarier fmarier force-pushed the francois-disable-sensors-background-sync branch from 94de613 to 7d970c1 Compare September 12, 2019 01:25
@fmarier fmarier force-pushed the francois-disable-sensors-background-sync branch from 7d970c1 to 6d8b3fe Compare September 12, 2019 01:33
@fmarier fmarier removed the request for review from iefremov September 12, 2019 01:33
@fmarier fmarier force-pushed the francois-disable-sensors-background-sync branch 2 times, most recently from a12a1a3 to 540dec7 Compare September 13, 2019 21:03
@fmarier fmarier added this to the 0.72.x - Nightly milestone Sep 13, 2019
@fmarier fmarier force-pushed the francois-disable-sensors-background-sync branch from 540dec7 to f577340 Compare September 13, 2019 22:25
@fmarier fmarier dismissed bridiver’s stale review September 14, 2019 00:12

Addressed in the latest revision.

@fmarier fmarier force-pushed the francois-disable-sensors-background-sync branch from f577340 to 06f1f35 Compare September 18, 2019 23:24
@fmarier fmarier merged commit f526e8b into master Sep 23, 2019
@fmarier fmarier deleted the francois-disable-sensors-background-sync branch September 23, 2019 22:46
bsclifton added a commit that referenced this pull request Sep 24, 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.

Background Sync should be disabled by default
3 participants