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

clean up clear data code #2449

Merged
merged 24 commits into from
Feb 19, 2024
Merged

clean up clear data code #2449

merged 24 commits into from
Feb 19, 2024

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Feb 7, 2024

Task/Issue URL: https://app.asana.com/0/414235014887631/1206545081824897/f
Tech Design URL:
CC: @loremattei

Description:

  • Cleans up the clear data code and makes it use the async/await API.
  • Removes some unnecessary and unused abstractions.
  • Removes some obsolete diagnostics.
  • Moves UI refresh to the end of the animation to improve consistency (should have been like this initially tbh).
  • Fixes a bug where domains which remove all the cookies might get those cookies back later.
  • setcookie.net is a convenient site for testing, but feel free to use any site that will drop a cookie (e.g. after login).
  • It's important to test this on iOS 17 and an earlier version of iOS as there are two paths depending on OS version.
  • Added a couple of maestro tests

Steps to test this PR:

Maestro

  1. Run the tests under .maestro/data_clearing_tests

Migration

  1. Check out an old version of the code (main will do)
  2. Set some cookies on setcookie.net and fire proof the site
  3. Check the cookies are retained as expected
  4. Checkout this branch and run the code
  5. Check the cookies are still retained

General Usage

  1. Go to setcookie.net and set a cookie (save it as a favourite too for convenience). Refresh the page to confirm it has been set.
  2. Use the fire button
  3. Go back to setcookie.net - cookie should gone
  4. Set another cookie and fire proof the site
  5. Use the fire button
  6. Cookie should be retained
  7. Use the fire button a couple of times in a row
  8. Cookie should still be present
  9. Go to settings and remove the site from the list of fireproofed sites
  10. Refresh the site and the cookie should have been removed
  11. Do a search and on the SERP change a setting (e.g. safe search to off)
  12. Use the fire button
  13. Go back to the SERP - the setting should be retained
  14. Login to some websites and fireproof - check that you stay logged in.
  15. Remove fireproofing - check that you are logged out

Copy link

github-actions bot commented Feb 7, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against dd5b376

@brindy brindy requested review from a team, aataraxiaa and bwaresiak and removed request for a team and aataraxiaa February 8, 2024 12:07
@brindy brindy requested a review from loremattei February 8, 2024 16:48
Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

I've tested this a lot Today, and found two issues:

  1. When using Auto Clear data, fireproofing doesn't seem to work when I kill and restart the app.
  2. When data clear animation is set to "None" URL is not cleared from URL bar.

Other than this good job @brindy:

  • Tested both iOS 15 and 17.
  • Upgrading app works fine.
  • With regular animation I could not reproduce the problem with URL bar glitch/data.
  • Tested various scenarios, opening, navigating, clearing, reopening, restarting... all of these worked as expected.

@brindy brindy requested a review from bwaresiak February 16, 2024 15:51
Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

Tested it this morning found two problems:

  1. Auto clear doesn't seem to remove cookies on restart. :)
  2. Blank Snapshot view has empty space instead of search bar.
    IMG_0704

// Internally this thing has always called 'async' functions but just never waited for them
// so this is no different really.
// await autoClear?.applicationDidLaunch()
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove? :)

@@ -154,6 +154,7 @@ class MainViewController: UIViewController {

var postClear: (() -> Void)?
var clearInProgress = false
var dataStoreReadiness: DataStoreWarmup? = DataStoreWarmup()
Copy link
Collaborator

Choose a reason for hiding this comment

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

readiness or a warmup? ;)

@brindy brindy requested a review from bwaresiak February 19, 2024 11:32
@brindy
Copy link
Contributor Author

brindy commented Feb 19, 2024

@bwaresiak haven't addressed the missing bar on the empty view because I think that is not related to this change, right?

Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

Great job!

@brindy brindy merged commit dad7d33 into main Feb 19, 2024
13 checks passed
@brindy brindy deleted the brindy/clean-up-clear-data-code branch February 19, 2024 13:56
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.

2 participants