-
Notifications
You must be signed in to change notification settings - Fork 14
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
Cookie prompt management #312
Conversation
Sam, please, would you like to have this reviewed? |
@tomasstrba I've just added remote config support, so I think it's good to go through a round of review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sammacbeth, thanks for creating this PR for getting the feedback sooner! 👍 In general, it is pretty good, I only found few tips for making the code more readable and maintainable in the future.
Plus, there are few inconsistencies in file management:
-
autoconsent folder shouldn't be commited to Submodules if it isn't true git submodule.
-
There are inconsistencies between Xcode file structure and real file structure (Autoconsent folder, Submodules folder). Ideally, it should be the same.
Thanks for the review @tomasstrba. I've addressed your inline comments. I've also:
Regarding file managment:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for butting in, but I've added a few comments to help make this clear and align with our (granted implied) code style.
@sammacbeth, sorry for later responses to your questions:
|
bb25b41
to
474509e
Compare
18e6c49
to
dd14037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sam, this is really nice code! 🥇 🎖️
Please, could merge develop into this branch so I can compile with the latest Xcode? Then I will test the functionality.
a82720f
to
82f7c22
Compare
4d69127
to
637bea9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sammacbeth, nice work! 👏 Can't wait to have this feature live 🚀
Code in general looks good and I like the architecture. There are few polishing tips so the style is aligned with Swift conventions. Sorry if they are a bit annoying
The feature worked for me after visiting theguardian.com, unfortunately these are the issues I found:
- After loading of the website, there is a period of time I can't do anything with the site. It feels like the main thread is blocked, but only the current webview seems to be unresponsive
- If you open a new tab, start loading theguardian.com and immediately switch the tab, you still get "Popups managed" info
- After popup is dismissed, the website scroll position is reset to the beginning (probably known issue)
cb66ba0
to
915a0d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀 🚀 🚀
Thanks for making the code more readable! If all reviews including the product review are finished, feel free to merge 🥇
Good job! 👏
Increase test timeout for CI Updated cookie-consent copy. Fix error when trying to run an action on a closed tab. Break autoconsent popover message Make autoconsent popover open underneath the urlbar. Move preferences scrollbar to the edge of the window Move cookie settings above GPC. Update copy
915a0d5
to
8fa0712
Compare
…ntication # By Alexey Martemyanov (1) and others * sam/logins-filtering-and-sorting: Disable the search field when editing. Waiting for ContentBlockingRules to be applied before navigation (#402) Cookie prompt management (#312) Pass config data to Autofill UserScript (#418) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Common/Utilities/UserDefaultsWrapper.swift # DuckDuckGo/Preferences/Model/PreferenceSections.swift # DuckDuckGo/Preferences/View/PrivacySecurityPreferencesTableCellView.xib
* Consent popup management feature Increase test timeout for CI Updated cookie-consent copy. Fix error when trying to run an action on a closed tab. Break autoconsent popover message Make autoconsent popover open underneath the urlbar. Move preferences scrollbar to the edge of the window Move cookie settings above GPC. Update copy * Another copy tweak. * Code tidying. * Extract protocol for autoconsent cache clear. * Final copy
Task/Issue URL: https://app.asana.com/0/72649045549333/1201033079931464/f
Tech Design URL: https://app.asana.com/0/481882893211075/1201233831195884/f
CC: @tomasstrba
Description:
Implements cookie prompt management using @cliqz/autoconsent. This is currently without a UI as the design project is not yet underway, but I'm opening this PR to get early feedback on the implementation.
We're running that library in a background Webview and exposing an API for it to native code. This API allows:
The JS code component is currently in the
Submodules
folder as a simple project which provides the glue between the native messaging and autoconsent library. This is not currently an actual git submodule, but it could make sense to have it as one if we wish to using this implementation on iOS too.Steps to test this PR:
The test page in duckduckgo/privacy-test-pages#65 has a more comprehensive set of test pages.
Testing checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM