Find in Page Hotfix + refactoring of forced unwrapping #450
Conversation
tomasstrba
requested review from
brindy,
sammacbeth and
mallexxx
and removed request for
brindy,
sammacbeth and
mallexxx
| @@ -109,20 +109,20 @@ final class AutoconsentUserScript: NSObject, UserScriptWithAutoconsent { | |||
| // push current privacy config settings to the background page | |||
| Self.background.updateSettings(settings: self.config.settings(for: .autoconsent)) | |||
| let cmp = await Self.background.detectCmp(in: self.tabId) | |||
| guard cmp?.result == true else { | |||
| guard let cmp = cmp, cmp.result == true else { | |||
There was a problem hiding this comment.
@sammacbeth, please, is the change here aligned with the auto-consent algorithm?
There was a problem hiding this comment.
Looks fine to me, this won't change anything in the autoconsent behavior.
| @@ -570,6 +570,7 @@ extension Tab: UserContentControllerDelegate { | |||
| userScripts.hoverUserScript.delegate = self | |||
| userScripts.autoconsentUserScript?.delegate = self | |||
|
|
|||
| findInPageScript = userScripts.findInPageScript | |||
| attachFindInPage() | |||
There was a problem hiding this comment.
Wouldn't it be more clear to have attachFindInPage() take in FindInPageUserScript argument and set findInPageScript variable inside (along with model assignments etc). I looks like that is supposed to be transactional anyway. Alternatively didSet of findInPageScript could call that method.
There was a problem hiding this comment.
No, attaching the find in page is separate from the user script. see didSet of findInPage var. This is so that we can have different models per tab and show the correct find page state when switching tab.
There was a problem hiding this comment.
No because attachFindInPage() is also called in didSet of weak var findInPage
There was a problem hiding this comment.
From what I see, we have different UserScripts instances per Tab, and same we do for FindInPageModel - so why not to guarantee these are always bound to each other?
Seeing how attachFindInPage() can be called on an optional user script may lead to inconsistency.
| @@ -109,20 +109,20 @@ final class AutoconsentUserScript: NSObject, UserScriptWithAutoconsent { | |||
| // push current privacy config settings to the background page | |||
| Self.background.updateSettings(settings: self.config.settings(for: .autoconsent)) | |||
| let cmp = await Self.background.detectCmp(in: self.tabId) | |||
| guard cmp?.result == true else { | |||
| guard let cmp = cmp, cmp.result == true else { | |||
There was a problem hiding this comment.
Looks fine to me, this won't change anything in the autoconsent behavior.
# By Tomas Strba (6) and others # Via GitHub (1) and Tomas Strba (1) * develop: Logins+ authentication (#438) Update privacy dashboard to main (#451) Version 0.19.1 Remove mouse event listeners on browser close (#448) Find in Page Hotfix + refactoring of forced unwrapping (#450) Memory leak fixed (#449) Version 0.19.0 Top autofill (#432) Option to add new notes or edit existing is disabled (#446) Use our own autoconsent fork (#444) New Feedback Form (#424) Update privacy dashboard (#440) # Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/SecureVault/View/PasswordManager.storyboard
* develop: Logins+ authentication (#438) Update privacy dashboard to main (#451) Version 0.19.1 Remove mouse event listeners on browser close (#448) Find in Page Hotfix + refactoring of forced unwrapping (#450) Memory leak fixed (#449) Version 0.19.0 Top autofill (#432) Option to add new notes or edit existing is disabled (#446) Use our own autoconsent fork (#444)
Task/Issue URL: https://app.asana.com/0/1177771139624306/1201892459973519/f
CC: @brindy @sammacbeth
Description:
Find in Page doesn't work because there is a missing reference to findinpage user script in Tab instance
Steps to test this PR:
Testing checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM