-
Notifications
You must be signed in to change notification settings - Fork 473
app-services tabs component constructor now wants a database filename #11799
Conversation
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, just needs a changelog entry as well when we're ready to land this with the A-S upgrade.
/** | ||
* An interface which defines read/write methods for remote tabs data. | ||
*/ | ||
open class RemoteTabsStorage( | ||
private val storageDir: File, |
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.
We should add a changelog entry about this breaking API before landing.
@@ -27,7 +27,7 @@ import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged | |||
class SyncedTabsStorage( | |||
private val accountManager: FxaAccountManager, | |||
private val store: BrowserStore, | |||
private val tabsStorage: RemoteTabsStorage = RemoteTabsStorage() | |||
private val tabsStorage: RemoteTabsStorage, |
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.
Same for here too.
Thanks - added a couple of entries. |
This pull request has conflicts when rebasing. Could you fix it @mhammond? 🙏 |
1 similar comment
This pull request has conflicts when rebasing. Could you fix it @mhammond? 🙏 |
There are v92.0.* versions of app-services now available, so I guess this is ready to roll. I'm not sure how to make CI happy about this though unless I attempt to bump a-s in this PR, but I don't think that's the correct thing to do. cc @jhugman who is presumably also trying to get nimbus upgraded. |
This pull request has conflicts when rebasing. Could you fix it @mhammond? 🙏 |
Can we get an r+ for this one? I'm hoping to get a new app-services release out and we need this to to resolve the breaking changes. I wouldn't merge the code, just looking for approval. |
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.
Rubberstamping this since it looked good to Jonathan and the changelog entries were added.
This will need to be rebased to at least update the changelog but in such probably the CI will get happy too.
This pull request has conflicts when rebasing. Could you fix it @mhammond? 🙏 |
I just made #12044 which includes this commit and also bumps the app-services version to |
This is the matching PR for mozilla/application-services#4862. Marking as draft because there's no a-s release yet, but CC @grigoryk @jonalmeida