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

New autoconsent rules and move tab cleanup out of onCommitted #489

Merged
merged 2 commits into from
Apr 1, 2022
Merged

New autoconsent rules and move tab cleanup out of onCommitted #489

merged 2 commits into from
Apr 1, 2022

Conversation

jdorweiler
Copy link
Collaborator

@jdorweiler jdorweiler commented Mar 29, 2022 •
edited
Loading

Task/Issue URL:
https://app.asana.com/0/11472677167855/1201985819843600
https://app.asana.com/0/11472677167855/1202006540839469

Tech Design URL:
CC:

Description:

  1. Update autoconsent rules from more CMP rules autoconsent#1
  2. I found a race condition that causes autoconsent to fail. This can happen when onCommitted fires after CMP detection has already started. My fix is to move the tab clean up (consent.removeTab) out of the onCommitted event and just run it before starting new CMP detection. I used this as my main browser for a day and didn't see any issues.

Steps to test this PR:

  1. The new were already tested. To check that they're working correctly try national-lottery.co.uk
  2. The race condition was easiest to repeat on a new tab
    • use the fire button to clear data
    • open a new tab
    • go to stackoverflow.com
    • the cookie popup should be handled. Try repeating this a few times to make sure it works every time

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired
Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@jdorweiler jdorweiler requested a review from tomasstrba March 29, 2022 20:25
@tomasstrba tomasstrba self-assigned this Mar 30, 2022
tomasstrba
tomasstrba approved these changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@tomasstrba tomasstrba assigned jdorweiler and unassigned tomasstrba Mar 31, 2022
@jdorweiler jdorweiler requested a review from adewes April 1, 2022 14:23
@jdorweiler jdorweiler merged commit b9017d5 into develop Apr 1, 2022
@jdorweiler jdorweiler deleted the jd/autoconsent-delete-tab branch April 1, 2022 15:19
samsymons added a commit that referenced this pull request Apr 4, 2022
@samsymons
Merge branch 'develop' into sam/password-generation
135076b 
# By Tomas Strba (5) and others
# Via GitHub
* develop:
  Refresh the Autofill panel after importing (#496)
  Require authentication for login export (#494)
  Observations database clearing improvements (#495)
  Fix handling external schemes in address bar (#493)
  New autoconsent rules and move tab cleanup out of onCommitted (#489)
  Safari CSV Passwords import +LastPass, 1Password (#483)
  Split preferences into multiple screens and add an option to show full URL (#487)
  Color of hover label fixed (#497)
  Add AMP links protection (#488)
  Enhancements of fire button options (#484)
  Rename Logins+ to Autofill (#492)
  new tab page low hanging fruit (#491)
  Fix threading issues in Autoconsent (#478)
  Navigation bar improvement: CMD + back or CMD + forward opens a new tab (#486)
  TextField refactored to TextView in order to make it scrollable (#481)
  More Xcode cleaning (#485)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
jdorweiler added a commit that referenced this pull request Apr 5, 2022
@jdorweiler
Revert "New autoconsent rules and move tab cleanup out of onCommitted (…
55de716 
…#489)"

This reverts commit b9017d5.
@jdorweiler jdorweiler mentioned this pull request Apr 5, 2022
Merged
tomasstrba pushed a commit that referenced this pull request Apr 6, 2022
@jdorweiler
Revert "New autoconsent rules and move tab cleanup out of onCommitted (…
83c52f6 
…#489)" (#502)

This reverts commit b9017d5.
samsymons added a commit that referenced this pull request Apr 10, 2022
@samsymons
Merge branch 'develop' into sam/password-generation
6e415fa 
* develop:
  Fill Add Bookmark modal view with current tab website (#515)
  Fix reloading data in BookmarkListViewController (#514)
  use the page url in the tracker object rather than the current web view url (#509)
  Version 0.22.2
  Revert the change to Arc4RandomUniformVariantRNG and silence SwiftLint warning (#510)
  Revert the change to Arc4RandomUniformVariantRNG and silence SwiftLint warning (#510)
  Version 0.22.1
  Improve Chromium Logins import (#501)
  Revert "New autoconsent rules and move tab cleanup out of onCommitted (#489)" (#502)
  Add fallback icons for password managers (#504)
  Make SwiftLint happy (#508)
  Default to Chrome or Safari when importing Logins and Bookmarks (#503)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomasstrba tomasstrba tomasstrba approved these changes

@adewes adewes Awaiting requested review from adewes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdorweiler @tomasstrba