Firefox data import improvements #622
Conversation
There was a problem hiding this comment.
Hey @samsymons! This code looks stellar and does the job. I tested with Firefox 101 and the import works flawlessly with and without a password. Also tested Chrome import and it still works as it should. Not approving the PR yet due to our private chat about little UI/UX issues (not caused by your patch) that you offered to have a look at.
| if newChildViewController === currentChildViewController { | ||
| return | ||
| } |
There was a problem hiding this comment.
This is to fix some cases where we were trying to add a child view controller repeatedly. You can still see this warning when navigating back and forth in the import flow, but this check improves it.
| @@ -66,6 +66,8 @@ final class FirefoxDataImporter: DataImporter { | |||
| case .decryptionFailed: completion(.failure(.logins(.cannotDecryptFile))) | |||
| case .failedToTemporarilyCopyFile: completion(.failure(.logins(.failedToTemporarilyCopyFile))) | |||
| } | |||
|
|
|||
| return | |||
There was a problem hiding this comment.
Without this return, the import data call continues ahead and hits the completion block at the end.
To be honest: I'm not happy with this API at all any more. 🙂 All of the code in here is synchronous, yet this function uses an async callback, so I would eventually like to replace this to use a return value instead of a callback. That would make it impossible to have bugs like this where callbacks are able to be called repeatedly by one invocation of this function.
There was a problem hiding this comment.
(The other thing I don't like is that an error with login import stops the function in its tracks, instead of allowing it to try to at least import bookmarks too. But, that's an improvement for another day... along with a SwiftUI rewrite of the UI 😬)
|
Added some tests to the I also took a look at converting this function from asynchronous to synchronous, but realized we have the HTML import case that does perform login in the background. Instead, it would be better to convert this class to Swift Concurrency in the future. Next Special Days I'd like to take a proper pass and fix all of these issues. |
There was a problem hiding this comment.
Thanks a lot @samsymons! I noticed that the newly added tests were failing because they relied on the real Firefox data directory (not present in the CI). I updated the tests to take fake data directory which fixed tests 👍
* develop: Update sparkle-sandbox.sh README to highlight proper usage Replace altool with notarytool in release build script (#623) Update embedded data Bump version to 0.26.4 Update homepage history feed after burning (#620) Firefox data import improvements (#622) Set version to 0.26.3 Microsoft logo for address bar animation (#618) Google spreadsheets login fix (#616)
# By Dominik Kapusta (5) and others # Via Dominik Kapusta (1) and GitHub (1) * develop: Use updated BSK branch (#627) Version 0.26.5 Embedded files updated Update BSK for iOS app group (#630) Recently Closed Menu Item (#617) Close dashboard when closing tab with cmd + w (#628) chore(deps): bump Autofill to 4.7.0 (#631) Handle legacy crash logs and use proper architecture in symbolicate.js (#621) Detect when Chrome login fails due to keychain prompt denial (#633) Correctly display unprotected sites on dashboard (#624) Update homefavicon (#625) Update sparkle-sandbox.sh README to highlight proper usage Replace altool with notarytool in release build script (#623) Update embedded data Bump version to 0.26.4 Update homepage history feed after burning (#620) Firefox data import improvements (#622) Set version to 0.26.3 # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj
Task/Issue URL: https://app.asana.com/0/1199230911884351/1202491428238974/f
Tech Design URL:
CC: @ayoy
Description:
This PR makes three changes:
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