diff options
Diffstat (limited to 'mobile/android/android-components/docs/rfcs')
15 files changed, 1952 insertions, 0 deletions
diff --git a/mobile/android/android-components/docs/rfcs/0000-template.md b/mobile/android/android-components/docs/rfcs/0000-template.md new file mode 100644 index 0000000000..0f5dbac9a7 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0000-template.md @@ -0,0 +1,46 @@ +--- +layout: page +title: your title +permalink: /docs/rfcs/file-name +--- + +* RFC PR: [PR #](https://github.com/mozilla-mobile/android-components/pull/#) +* Start date: YYYY-MM-DD (Day of proposal posting.) +* End date: YYYY-MM-DD (Last day for general feedback. However, the proposal can be merged immediately after all stakeholders approve.) +* Stakeholders: github-username, github-username + +## Summary + +This section should include a brief description of the proposal. + +## Motivation + +This section should include reasoning about why the proposal is useful. Examples, specific scenarios, +open bugs, records of performance metrics, and other empirical data are beneficial to include here if available. + +## Guide-level explanation + +This section should include a high-level walkthrough of the steps required to implement the proposal. + +## Reference-level explanation (optional) + +This section should include a detailed walkthrough of technical steps or code changes that +will be required to implement the proposal. Code samples and prototypes are beneficial to include here. + +## Drawbacks (optional) + +This section should include any drawbacks to the proposal. + +## Rationale and alternatives + +This section should include any alternative proposals considered, as well as the rationale for why +they were not selected for the proposal. + +## Resources and Docs (optional) + +- Any (internal or external) similar proposals or other documentation that shares concepts with the proposal. +- Links to artifacts generated as part of the proposal, such as additional documentation or follow-up bugs. + +## Unresolved questions + +Questions from the proposal author or from reviewers that are not yet resolved. diff --git a/mobile/android/android-components/docs/rfcs/0001-rfc-process.md b/mobile/android/android-components/docs/rfcs/0001-rfc-process.md new file mode 100644 index 0000000000..0991947546 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0001-rfc-process.md @@ -0,0 +1,70 @@ +--- +layout: page +title: RFC process +permalink: /rfc/0001-rfc-process +--- + +* Start date: 2020-07-07 +* RFC PR: [#7651](https://github.com/mozilla-mobile/android-components/pull/7651) + +## Summary + +Introducing a lightweight RFC ("request for comments") process for proposing and discussing "substantial" changes and for building consensus. + +## Motivation + +The existing workflow of opening and reviewing pull requests is fully sufficient for many smaller changes. + +For substantially larger changes (functionality, behavior, architecture), an RFC process prior to writing any code may help with: + +* Publicly discussing a change proposal with other maintainers and consumers of components. +* Gathering and integrating feedback into a proposal. +* Documenting why specific changes and decisions were made. +* Building consensus among the team before potentially writing a lot of code. + +A change is substantial if it + +* affects multiple components; +* affects how components interact through either their public or internal APIs; +* fundamentally changes how a component is implemented, how it manages state or reacts to changes, in a way that isn't self-explanatory or a direct result of a bug fix. + +## Guide-level explanation + +The high-level process of creating an RFC is: + +* Create an RFC document (like this one) using the template. +* Open a pull request for the RFC document. +* Ask for feedback on the pull request, via the [mailing list]() or in [chat](https://chat.mozilla.org/#/room/#android-components:mozilla.org). + +During the lifetime of an RFC: + +* Discussion happens asynchronously on the pull request. Anyone is allowed to engage in this discussion. +* Build consensus and integrate feedback. + +After the discussion phase has concluded: + +* If a consensus has been reached, then the RFC is considered "accepted" and gets merged into the repository for documentation purposes. +* If a consensus has not been reached, then the RFC is considered "rejected" and the pull request gets closed. The rejected RFC proposal may get revived should the requirements change in the future. + +Once the RFC is accepted, then authors may implement it and submit the feature as a pull request. + +## Drawbacks + +* Writing an RFC is an additional overhead and may feel slower or cumbersome. The assumption is that the advantages still outnumber this drawback. In the hopes of better fitting the needs of the team at this time, this RFC process is simplified and deigned to be lightweight compared to other existing projects. + +## Rationale and alternatives + +Discussions about changes have been present without this process - mostly happening in more real-time means of communication such as Zoom, Slack or Riot. BThis resulted in forced synchronicity and closed platforms for those interested yet not involved parties. A slower RFC process allows more people to participate, avoids "secrets" and documents the discussion publicly. + +## Prior art + +Many other open-source projects are using an RFC process. Some examples are: + +* [Rust RFCs - Active RFC List](https://rust-lang.github.io/rfcs/) +* [Bors: About the Draft RFCs category](https://forum.bors.tech/t/about-the-draft-rfcs-category/291) +* [seL4: The RFC Process](https://docs.sel4.systems/processes/rfc-process.html) +* [The TensorFlow RFC process](https://www.tensorflow.org/community/contribute/rfc_process?hl=en) + +## Unresolved questions + +* Is this process lightweight enough that it will be used? diff --git a/mobile/android/android-components/docs/rfcs/0002-browser-search-rewrite.md b/mobile/android/android-components/docs/rfcs/0002-browser-search-rewrite.md new file mode 100644 index 0000000000..23530d7800 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0002-browser-search-rewrite.md @@ -0,0 +1,128 @@ +--- +layout: page +title: Replacing `browser-search` component with state in `browser-store` +permalink: /rfc/0002-search-state-in-browser-store +--- + +* Start date: 2020-07-07 +* RFC PR: [#7655](https://github.com/mozilla-mobile/android-components/pull/7655) + +## Summary + +Moving all search related state to `BrowserState` and replacing `SearchEngineManager` with a `BrowserStore` middleware that deals with saving and loading state; better satisfying the requirements of [Fenix](https://github.com/mozilla-mobile/fenix) and [Focus](https://github.com/mozilla-mobile/focus-android). + +## Motivation + +The integration of `browser-search` into [Fenix](https://github.com/mozilla-mobile/fenix) and [Firefox for Fire TV](https://github.com/mozilla-mobile/firefox-tv/) showed that the current API is not enough to satisfy all use cases. Some examples include: + +* Fenix supports custom search engines. This can only be achieved with `browser-search` by passing a custom `SearchEngineProvider` to `SearchEngineManager`. Everything else is left to the app. +* `SearchEngineManager` initially only provided synchronous methods for accessing search engines. Later asynchronous methods were added. The result is a hard to understand mix that can cause blocking I/O to happen on the wrong threads. +* The `browser-search` component does not follow our abstraction model of implementing a `concept-search` component. This makes it impossible to switch or customize implementations. +* The wrapper code in Fenix makes it hard to argue about the state that is split over two implementations. In addition to that it makes it hard to add functionality to `SearchEngineManager` since it is not used directly. +* Nowadays Android Components bundles all state in `BrowserState` observable through `BrowserStore` (provided by `browser-state`). On the application side we are using an UI-scoped `Store` (provided by `lib-store`). `SearchEngineManager` creates a secondary source of truth just for search and circumvents this architecture pattern. +* In "Firefox for Fire TV" and "Firefox for Echo Show" we needed to override the default search configuration, adding and replacing loaded search engines. This turned out to be quite complicated and required wrapping multiple classes that deal with loading the search configuration and engines. + +## Guide-level explanation + +Instead of making `SearchEngine` instances accessible through `SearchEngineManager` via a `browser-search` component, we want to make `SearchEngine(State)` available in `BrowserState`. A middleware on the `BrowserStore` will transparently take care of loading `SearchEngine` instances. Whenever the app dispatches an action to change the list of search engines (e.g. adding a custom search engine) then the middleware will take care of saving the new state to disk. + +This will allow the app to use the already proven pattern of observing a store to update UI and makes it easy to mix search state with other state without having to query multiple sources. In addition to that the app no longer needs to take care of the storage and fallbacks (e.g. slow MLS query) since that can be handled completely by the middleware. + +Using the `BrowserStore` for state will make the app use asynchronous patterns to observe the state and with that avoid blocking the main thread, as well as force the app to deal with the initial state of having no (default) `SearchEngine` yet. + +Since the state will move to `browser-state`, additional functionality (querying suggestions, storage, middleware) can move to `feature-search`. This will make `browser-search` obsolete. With that introducing a `concept-search` is not required. An app can use a custom implementation by replacing the middleware, storage or handling search state completely differently. + +## Reference-level explanation + +The new implementation will use `browser-state` to model all search state. + +Functionality on top of the state (querying suggestions, storage, middleware) will live in `feature-search`. With that `browser-search` will no longer be needed and can be removed after following the [deprecation process](https://mozac.org/contributing/deprecating). + +### State + +`BrowserState` will get a new `search` property with `SearchState` type that lives in `browser-state`: + +```Kotlin +data class BrowserState( + // ... + val search: SearchState +) + +/** + * Value type that represents the state of available search engines. + * + * @property searchEngines List of loaded search engines. + * @property defaultSearchEngineId ID of the default search engine. + */ +data class SearchState( + val searchEngines: List<SearchEngine>, + val defaultSearchEngineId: String +) +``` + +`SearchState` will contain all `SearchEngine`s (bundled and custom). Additional extension methods will make it easier to select specific subsets or the default `SearchEngine`. + +`SearchEngine` will be turned into a data class and moved to `browser-store`. A `type` property will make custom and provided default search engines distinguishable. Other methods like `buildSearchUrl()` will be implemented as extension methods. + +### Storage + +There will be two storage classes: + +* `SearchEngineStorage`: A persistent storage for `SearchEngine`s. The primary use case is for persisting custom search engines added by users. +* `SearchEngineDefaults`: A provider of a list of default search engines (for the user's region) based on `list.json` and the search plugins currently included in `browser-search`. + +Those storage classes will have visibility `internal` and will not be used by the app directly. + +All storage access methods will be suspending functions to avoid thread-blocking access: + +```Kotlin +internal class SearchEngineStorage { + suspend fun getSearchEngines() = withContext(Dispatchers.IO) { + // ... + } +} +``` + +### Middleware + +A `SearchMiddleware` that will be installed on `BrowserStore` will be responsible for: + +* Loading the initial set of search engines (from the two storages above) and adding them to `BrowserState` by dispatching actions on `BrowserStore`. +* Persisting state changes it observes in the specific storage: + * Adding search engines + * Removing search engines + * Changing the default search engine +* Updating search engines at runtime (e.g. region or locale change) + +Fallback behavior (e.g. region cannot be determined) will be handled by the middleware. + +## Drawbacks + +* Reimplementing the functionality of `browser-search` will be a project that will take time. Refactoring consuming apps to read the state from `BrowserStore` will take additional time. From the `browser-session` to `browser-state` migration we know that this process can take a long time. Although the scope of `browser-search` is much smaller. + +## Rationale and alternatives + +* Initially we tried extending the API of `SearchEngineManager` to add additional asynchronous access methods. But this made the API more complicated and did not solve some of the problems of the API itself. In addition to that keeping the old API functional stopped us from fundamentally refactoring some internals. +* We considered directly upstreaming functionality from Fenix to Android Components. While technically possible, this would only improve maintainability but not address some of the API flaws that using the component in Fenix uncovered. +* We considered creating a new `SearchEngineManager` implementation with an asynchronous API (e.g. `Flow`). But this would still create a second source of truth in addition to `BrowserStore` and will make it complicated to observe and mix state from both sources. + +## Prior art + +We have implemented similar functionality a lot of times: + +* [SearchEngineManager in Fennec](https://searchfox.org/mozilla-esr68/source/mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java) +* [SearchEngineManager in Android Components](https://github.com/mozilla-mobile/android-components/blob/08880314f56d73691b3cd909d5dee199bba4ed0b/components/browser/search/src/main/java/mozilla/components/browser/search/SearchEngineManager.kt#L28) +* Focus had its own implementation of `SearchEngineManager` too, but was migrated to use `browser-search` already. +* [FenixSearchEngineProvider](https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt) in Fenix +* [SearchService](https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm) in Firefox (desktop) +* [SearchEngines.swift](https://github.com/mozilla-mobile/firefox-ios/blob/main/Client/Frontend/Browser/SearchEngines.swift) in Firefox for iOS + +## Unresolved questions + +* Do the proposed interfaces satisfy the requirements of current consumers of `browser-search`? + * [Firefox Preview (Fenix)](https://github.com/mozilla-mobile/fenix) + * [Firefox Focus/Klar](https://github.com/mozilla-mobile/focus-android) + * [Firefox Reality](https://github.com/MozillaReality/FirefoxReality) + * [Firefox for Fire TV](https://github.com/mozilla-mobile/firefox-tv/) + * [Firefox for Echo Show](https://github.com/mozilla-mobile/firefox-echo-show/) +* Some of the naming (e.g. `SearchEngineDefaults`) is not great yet. But that may be unrelated to the proposed change. diff --git a/mobile/android/android-components/docs/rfcs/0003-concept-base-component.md b/mobile/android/android-components/docs/rfcs/0003-concept-base-component.md new file mode 100644 index 0000000000..bac4385057 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0003-concept-base-component.md @@ -0,0 +1,33 @@ +--- +layout: page +title: Adding a `concept-base` component +permalink: /rfc/0003-concept-base-component +--- + +* Start date: 2020-07-20 +* RFC PR: [#7776](https://github.com/mozilla-mobile/android-components/pull/7776) + +## Summary + +Adding a `concept-base` component for basic interfaces needed by multiple components. + +## Motivation + +We already have a `support-base` component that contains basic building blocks, like a logger, that other components may need. Some of those building blocks are interfaces, like `CrashReporting` that are implemented by other components (e.g. `lib-crash`). This works well in most cases, but becomes problematic once a `concept` component requires such an interface. Having a `concept` component depend on actual code with `support-base` is breaking our contract of concepts only depending on other concepts. + +* In [#7689](https://github.com/mozilla-mobile/android-components/issues/7689) I want to introduce a `Profiler` interface that allows other components to add profiler markers. The Firefox profiler provides this functionality in our `browser-engine-gecko*` components (exposed by `concept-engine`). If this interface lives in `support-base` then `concept-engine` would need to depend on `support-base`. +* In [#7775](https://github.com/mozilla-mobile/android-components/issues/7775) I want to introduce an interface that allows components to make leaks detectable. + +## Reference-level explanation + +We introduce a `concept-base` component that contains those "basic" interfaces. Other components and concepts can depend on this component. This component will be the home for the `Profiler` interface ([#7689](https://github.com/mozilla-mobile/android-components/issues/7689)) and leak detection interface ([#7775](https://github.com/mozilla-mobile/android-components/issues/7775)). + +In addition to that we can move interface-only pieces from `support-base` to `concept-base`: `CrashReporting`, `MemoryConsumer`. + +## Drawbacks + +* This introduces another base component that most other components will depend on. Since it only contains interfaces the impact of that will be low though. + +## Rationale and alternatives + +* Alternatively we could create distinct `concept` components for every interface theme. This would end up with a very fine-grained list of components that mostly may contain only a single interface. This would also break with the idea that a concept describes a component that will have an actual implementation component (e.g. `concept-toolbar` -> `browser-toolbar`). Interfaces like `MemoryConsumer` are not describing a full component, but instead just one unified aspect of it. diff --git a/mobile/android/android-components/docs/rfcs/0004-top-sites-feature.md b/mobile/android/android-components/docs/rfcs/0004-top-sites-feature.md new file mode 100644 index 0000000000..28107ed4a8 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0004-top-sites-feature.md @@ -0,0 +1,123 @@ +--- +layout: page +title: Introduce a Top Sites Feature +permalink: /rfc/0004-top-sites-feature +--- + +* Start date: 2020-07-30 +* RFC PR: [#7931](https://github.com/mozilla-mobile/android-components/pull/7931) + +## Summary + +Create a TopSitesFeature that abstracts out the co-ordination between the multiple data storages. + +## Motivation + +A large part of the top sites implementation is spread across A-C and Fenix. There is a lot of co-ordination between the History API and the current Top Sites API to manage the final state of what sites should be displayed that can exist as a single implementation. + +## Guide-level explanation + +To begin, we should rename the `TopSitesStorage` to `PinnedSitesStorage` and create a `TopSitesFeature` that would orchestrate the different storages and present them to the app. + +We can then follow the architecture of existing features by using the presenter/interactor/view model, and introduce a `TopSitesStorage` which abstract out the complexity of the multiple data sources from `PinnedSitesStorage` and `PlacesHistoryStorage`. + +To allow the ability for adding a top site from different parts of the app (e.g. context menus), we introduce the `PinnedSitesUseCases` that acts on the storage directly. If the `TopSitesFeature` is started, it will then be notified by the `Observer` to update the UI. + +```kotlin + +/** + * Implemented by the application for displaying onto the UI. + */ +interface TopSitesView { + /** + * Update the UI. + */ + fun displaySites(sites: List<TopSite>) + + interface Listener { + /** + * Invoked by the UI for us to add to our storage. + */ + fun onAddTopSite(topSite: TopSite) + + /** + * Invoked by the UI for us to remove from our storage. + */ + fun onRemoveTopSite(topSite: TopSite) + } +} + +/** + * Abstraction layer above the multiple storages. + */ +interface TopSitesStorage { + + /** + * Return a unified list of top sites based on the given number of sites desired. + * If `includeFrecent` is true, fill in any missing top sites with frecent top site results. + */ + suspend fun getTopSites(totalNumberOfSites: Int, includeFrecent: Boolean): List<TopSite> + + interface Observer { + /** + * Invoked when changes are made to the storage. + */ + fun onStorageUpdated() + } +} + +// Already exists in browser-storage-sync. +interface PlacesHistoryStorage { + fun getTopFrecentSites(num: Int) +} + +class DefaultTopSitesStorage( + val pinnedSitesStorage: PinnedSitesStorage, + val historyStorage: PlacesHistoryStorage +) : TopSitesStorage { + + /** + * Merge data sources here, return a single list of top sites. + */ + override suspend fun getTopSites(totalNumberOfSites: Int, includeFrecent: Boolean): List<TopSite> +} + +/** + * Use cases can be used for adding a pinned site from different places like a context menu. + */ +class PinnedSitesUseCases(pinnedSitesStorage: PinnedSitesStorage) { + val addPinnedSites: AddPinnedSiteUseCase + val removePinnedSites: RemovePinnedSiteUseCase +} + +/** + * View-bound feature that updates the UI when changes are made. + */ +class TopSitesFeature( + val storage: TopSitesStorage, + val presenter: TopSitesPresenter, + val view: TopSitesView, + val defaultSites: () -> List<TopSites> +) : LifecycleAwareFeature { + override fun start() + override fun stop() +} +``` + +## Drawbacks + +* `TopSitesStorage` always pulls sites from a persistent storage (Room). This could cause an excessive amount of reads from disk - this is drawback that exists in today's implementation as well. Places has an in-memory cache that should not be affected by this. + +## Rationale and alternatives + +- This will remove our dependency on `LiveData` in Fenix by updating the `TopSitesView` when we have the top sites available to display; see [#7459](https://github.com/mozilla-mobile/android-components/issues/7459). +- We can reduce the multiple implementations of the TopSitesFeature by introducing it into Android Components. + +## Prior art + +* Fenix abstraction of Top Sites: [TopSiteStorage.kt](https://github.com/mozilla-mobile/fenix/blob/3d3153039ce794df09a243968b888ae7cb856d9b/app/src/main/java/org/mozilla/fenix/components/TopSiteStorage.kt) +* The RFC structure can be see in other components such as, [FindInPageFeature](https://github.com/mozilla-mobile/android-components/blob/main/components/feature/findinpage/src/main/java/mozilla/components/feature/findinpage/FindInPageFeature.kt) & [SyncedTabsFeature](https://github.com/mozilla-mobile/android-components/blob/main/components/feature/syncedtabs/src/main/java/mozilla/components/feature/syncedtabs/SyncedTabsFeature.kt). + +## Unresolved questions + +* When the user removes a top site from the UI that comes from `TopSitesStorage`, we remove it from the storage. When a top site comes from frecent, what do we do? Should we be removing the frecent result? diff --git a/mobile/android/android-components/docs/rfcs/0005-migrate-sitepermission-ac-store-geckoview-store.md b/mobile/android/android-components/docs/rfcs/0005-migrate-sitepermission-ac-store-geckoview-store.md new file mode 100644 index 0000000000..a3ce8ca64c --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0005-migrate-sitepermission-ac-store-geckoview-store.md @@ -0,0 +1,34 @@ +--- +layout: page +title: Migrate feature-sitepermissions to be compatible with the new GeckoView permission API. +permalink: /rfc/0005-migrate-sitepermission-ac-store-geckoview-store +--- + +* Start date: 2020-07-26 +* RFC PR: [#7855](https://github.com/mozilla-mobile/android-components/pull/7855) + +## Summary + +Migrate our generic [SitePermissionsStorage](https://github.com/mozilla-mobile/android-components/blob/56e6f5ba55734014804b3f3b0af5c32414fa1d16/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsStorage.kt#L33) to be compatible with the new [GeckoView permissions API](https://docs.google.com/document/d/1KUq0gejnFm5erkHNkswm8JsT7nLOmWvs1KEGFz9FWxk/edit#heading=h.ls1dr18v7zrx). + +## Motivation + +We are currently storing all site permissions on disk in AC (both GeckoView and WebView). [With the new GeckoView API](https://docs.google.com/document/d/1KUq0gejnFm5erkHNkswm8JsT7nLOmWvs1KEGFz9FWxk/edit#heading=h.ls1dr18v7zrx), GeckoView is now going to store all the permissions on their end instead of storing it ourselves. They will expose APIs for managing permissions(query, add, delete, and update). + +That put us in a situation where we have to migrate the existing user data to the new [GeckoView APIs](https://docs.google.com/document/d/1KUq0gejnFm5erkHNkswm8JsT7nLOmWvs1KEGFz9FWxk/edit#heading=h.ls1dr18v7zrx). + +## Reference-level explanation + +#### Doing a one-shot migration. +We decide on the timing (i.e. app startup) to do a background operation to transfer all the data to GeckoView in a one-shot. Until this operation is not finished, all the site permissions operations will be put on hold until the migration is over. + +We transform [SitePermissionsStorage](https://github.com/mozilla-mobile/android-components/blob/a93eae2cbf81a17e80a98ec234db414d1f7e84f5/components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsStorage.kt#L33) into an interface, with which we can have two distinctive implementations - one for GeckoView and the other for other engines. Inside the GeckoView implementation, we can have a preference that indicates if the migration is over or not. When the `SitePermissionsFeature.start()` gets called, we start the migration by guarding all the operations(read/write) on GeckoViewStorage and holding its response until the +migration is over. + +To mark when the migration will be over, we can define a time frame and after that we can remove the code for the migration. + +## Drawbacks +We are going to do a fast migration but depending on how much data the user has, it could impact the user's experience, as users could load a page that requires permissions and this functionality will no be active until we finish the migration. + +## Rationale and alternatives +**Re-prompt**, we could start from the scratch and ask again users to grant permissions to sites. One drawback is users will lost their previous data. diff --git a/mobile/android/android-components/docs/rfcs/0006-search-defaults.md b/mobile/android/android-components/docs/rfcs/0006-search-defaults.md new file mode 100644 index 0000000000..2fbea706ae --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0006-search-defaults.md @@ -0,0 +1,101 @@ +--- +layout: page +title: Determining the set of default search engines +permalink: /rfc/0006-search-defaults +--- + +* Start date: 2020-09-D8 +* RFC PR: [#8437](https://github.com/mozilla-mobile/android-components/pull/8437) + +## Summary + +Determining the set of default search engines based on the user's home region. + +## Motivation + +Our configuration of default search engines ([list.json](https://github.com/mozilla-mobile/android-components/blob/main/components/browser/search/src/main/assets/search/list.json)) is based on the region and language or locale of the user. + +To provide our users with the best choice of search engines and to satisfy the needs of our partnerships, we want to determine (and recognize changes of) the user's region as accurately as possible. + +The scope of this document is limited to the resolving and caching of the user's region and using it to determine the default search engines. + +### Problems and constraints + +* **Fast**: A search engine needs to be available on application start. The search engine icon is used in the application UI and once the user starts typing we need to be able to provide search suggestions and ultimately perform the search. + +* **Accurate**: While the locale can be queried from the system, the actual region can be obscured or just harder to determine accurately. + +* **Change**: So far, on mobile, we have been doing only a single region lookup. In reality the region of a user can change over time. Especially when the initial installation happened outside of the "home region" of the user then this user remained in the "wrong" region until the app was reinstalled. + +* **Privacy**: Not all sources can be accessed at any time. While GPS may be the most accurate source for the user's region, it also requires a runtime permission, that we wouldn't want to ask for on the first app launch. + +## Guide-level explanation + +### Determining the region + +While there are many available sources for determining the region of the user, we will focus on using the Mozilla Location Service (MLS) as the primary source for now. MLS can determine the region of the user by doing a GeoIP lookup. Additionally it supports determining the location based on Bluetooth, cell tower and WiFI access point data. However so far on mobile we have only been using the GeoIP lookup and have not sent any data to MLS. + +### Fallback + +Other than previously in Fenix we are **not** using the locale as a fallback provider for the region. Instead we will fallback to use the default from the configuration (e.g. `default` in `list.json`). + +### Validation + +The region returned by MLS may be skewed obscured by proxies, VPNs or inaccuracies in the geo IP databases. To accommodate for this, we want to perform additional validation to verify if the provided region is plausible. + +The first _validator_ that we want to ship uses the timezone of the user. Similar to the implementation in the desktop version of Firefox ([[1]](https://searchfox.org/mozilla-central/rev/f82d5c549f046cb64ce5602bfd894b7ae807c8f8/toolkit/modules/Region.jsm#213-224), [[2]](https://searchfox.org/mozilla-central/rev/f82d5c549f046cb64ce5602bfd894b7ae807c8f8/toolkit/modules/Region.jsm#763-779)) users with a `US` region and not a `US` timezone will not get `US` assigned as _current_ or _home_ region. + +### Home region and updates + +We will use a similar mechanism to track and update the home region of a user as the desktop version of Firefox does. + +We will determine between a _home_ region and the _current_ region. Eventually the _current_ region may become the _home_ region of the user. Only the _home_ region will be used for determining the default search engines. + +* On the very first app launch we will determine the _current_ region of the user and set it as _home_ region. Until we get a valid response from MLS a user may be on the default search configuration. Since the application usually shows onboarding screens on the first launch, this delay may not be visible to the user. +* On the next app starts we will automatically use the _home_ region. This region is immediately available to the application. Additionally we will query the different providers in the background. + * If the result matches the _home_ region then no further action is needed. + * If the result different than the _current_ region then we save the new value as the _current_ region along with the current timestamp. + * If the result matches the _current_ region and the timestamp of the _current_ region is older than 2 weeks (meaning the _current_ region has not changed in two weeks) then we set the result as the new _home_ region. + +With a new _home_ region the user may get a different set of default search engines. + +The default search engine of the user will be determined by: + +* If the user has not made an active choice: We use the `default` in the configuration (`list.json`) for the _home_ region of the user. When the _home_ region of the uer changes then the default search engine may change too. +* If the user has made an active choice (setting a default in the application's setting) then this search engine will remain the user's default search engine - even if this search engine is not in the list of search engines of the (new) _home_ region of the user. + +### Tests + +Like for our other components, we want to have a high test coverage for this critical implementation. For the search engine defaults we want to cover some partnership requirements as unit tests to avoid regressions in the affected areas. + +## Reference-level explanation + +### Region + +We will introduce a new (internal) `RegionManager` class that will take care of tracking and updating the _current_ and _home_ region of the user. A `RegionMiddleware` will listen to the `InitAction` of the `BrowserStore` and dispatch an action to add a `RegionState` to the `BrowserState`, as well as asking the `RegionManager` to check whether a region change has happened (by internally querying MLS). + +The new `SearchMiddleware` (see [RFC 2 about the new search architecture](https://mozac.org/rfc/0002-search-state-in-browser-store)) will listen to the `RegionState` update, load the matching default search engines using the search storage, and dispatch an action updating the `SearchState`. + +## Rationale and alternatives + +* An earlier draft of this RFC described using multiple region sources (GNSS, Network and SIM country, Google Play Services) and weighting them. This requires collecting more real world data about the accuracy of those sources and how they drift from what MLS determines, before shipping an implementation and therefore this idea was discarded here. It may be the topic for a separate, future RFC. + +## Prior art + +* [Timezone check in Firefox desktop](https://searchfox.org/mozilla-central/source/toolkit/modules/Region.jsm#180) +* [Update interval in Firefox desktop](https://searchfox.org/mozilla-central/source/toolkit/modules/Region.jsm#75) +* [RFC about moving state to new AC component](https://mozac.org/rfc/0002-search-state-in-browser-store) +* [Fuzzy location provider idea](https://github.com/mozilla-mobile/android-components/issues/1720) +* [SearchEngineManager in Fennec](https://searchfox.org/mozilla-esr68/source/mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java) +* Current [SearchEngineManager in Android Components](https://github.com/mozilla-mobile/android-components/blob/08880314f56d73691b3cd909d5dee199bba4ed0b/components/browser/search/src/main/java/mozilla/components/browser/search/SearchEngineManager.kt#L28) +* [FenixSearchEngineProvider](https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt) in Fenix +* [SearchService](https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm) in Firefox (desktop) +* [SearchEngines.swift](https://github.com/mozilla-mobile/firefox-ios/blob/main/Client/Frontend/Browser/SearchEngines.swift) in Firefox for iOS + +## Unresolved questions + +* What kind of telemetry could or should we collect? What data would help us tweak this? How can we confirm accuracy? + +* Is the 2 weeks period before switching the _home_ region, that we borrowed from desktop, acceptable for mobile too? + +* Would it help to expose the _current_ and _home_ region in Fenix in a debug screen or in the "about" screen? diff --git a/mobile/android/android-components/docs/rfcs/0007-synchronized-releases.md b/mobile/android/android-components/docs/rfcs/0007-synchronized-releases.md new file mode 100644 index 0000000000..e978943957 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0007-synchronized-releases.md @@ -0,0 +1,114 @@ +--- +layout: page +title: Synchronizing the branching and versioning of Android Components with the Mozilla release trains +permalink: /rfc/0007-synchronized-releases +--- + +* Start date: 2021-03-22 +* RFC PR: [#9973](https://github.com/mozilla-mobile/android-components/pull/9973) + +## Summary + +Synchronizing the branching and versioning of Android Components with the Mozilla release trains (GeckoView) to simplify release processes and integration by: + +* Having just one component implementing `concept-engine` with GeckoView +* Having the `main` branch track only GeckoView Nightly +* Creating branches and releases tracking matching GeckoView releases (e.g. GeckoView 89.0 -> Android Components 89.0) + +## Motivation + +Initially we introduced three `concept-engine` implementations for GeckoView, one for each GeckoView release channel: + +* `browser-engine-gecko-nightly` +* `browser-engine-gecko-beta` +* `browser-engine-gecko` + + This allowed us to release Android Components independently from GeckoView and consumers could use the latest AC release regardless of which GeckoView channel they wanted to use it with. + +This flexibility comes at a cost and makes our release process and maintenance more difficult ([complex merge day process in the AC repository](https://github.com/mozilla-mobile/android-components/blob/d6fbb014667871504aadfdc712ca0313aabc1e46/docs/contribute/merge_day.md), multiple GeckoView components to maintain). For every new GeckoView release across all channels (nightly, beta, release) we want to ship a new release of Fenix (and with that Android Components). So there is already an implicit synchronization of releases, but it is manual and confusing. By making this dependency explicit through synchronized releases we can simplify our processes. + +## Explanation + +### One GeckoView component + +Instead of having three components using GeckoView (`browser-engine-gecko-nightly`, `browser-engine-gecko-beta`, `browser-engine-gecko`), we will have just one (`browser-engine-gecko`) and on `main` it will track the latest GeckoView Nightly from `mozilla-central`. + +### Releases + +Nightly releases of Android Components will come with the latest Nightly version of GeckoView. + +For every major version from the Mozilla repositories, we will have a major version release of Android Components. Those versions and releases will be synchronized across our whole stack: Fenix 89 will use Android Components 89, which will use GeckoView 89. + +### Branches + +On the Android Components side we will continue to have a `main` branch, which will track GeckoView Nightly and from which we will ship Android Components Nightly versions. + +For every GeckoView version we will have a matching Android Components release branch (e.g. `releases/89.0`), that we cut from `main` the day the GeckoView Nightly version becomes Beta. This branch will continue to track the matching GeckoView version from beta builds to release builds. + +### Merge day + +On merge day the current Nightly version of GeckoView will become the new Beta version. At this point we will cut a matching Android Components release branch, which will continue to track the GeckoView version, while `main` will move on to track the next Nightly version. + +Moving code between components will no longer be needed and the merge day procedure will be reduced to branching and versioning, which will be easier to automate. + +Example: On the day GeckoView 89 Nightly becomes GeckoView Beta 89, we will cut an Android Components 89 release branch which will track GeckoView Beta 89 (which will eventually become the release version of GeckoView 89). The `main` branch of Android Components will continue to track GeckoView Nightly 90. + +### Automation & Bots + +We can continue to bump GeckoView and Android Component versions with our bots. The aligned version numbers simplify the process and even allow us to automate releasing and branching too. + +* On `main` a bot will continue to update to the latest GeckoView Nightly version. With this change no other version bumps on `main` are needed. +* On a release branch (e.g. `releases/89.0`) a bot can update to the latest matching GeckoView version. Over time the branch will be bumped from Beta (e.g. 89.0 Beta) to Release versions of GeckoView (e.g. 89.0 release). + +Additionally we could automate: + +* Whenever the GeckoView Nightly major version changes on merge day and a new GeckoView Beta is available, a bot could cut a matching Android Components release branch and switch to the GeckoView Beta version (e.g. when GeckoView Nightly 89 switches to Nightly 90 a bot could cut a `releases/89.0` branch, bump it to GeckoView Beta 89 and cut a release). + +### Localization + +The localization workflow will be very similar to the process in Fenix. We would do the following: + +* Frequently import strings to `main` through the `mozilla-l10n-bot`. +* Sync strings from `main` to `releases_v89.0` while `89` is in _Beta_. We would do this up to the merge day. + +For the import to `main` no changes are needed. For the string sync between `main` and the _Beta branch_ we should be able to use the same code as we use for Fenix. (As of this writing, that code is in progress - having more similarity between Fenix and A-C would definitely simplify it.) + +### Fenix integration + +The integration into Fenix would look similar as today: +* `master` tracks AC Nightly with that GeckoView Nightly +* A release branch tracks the matching AC version. The aligned versions will avoid confusion (Fenix 89 release branch will use AC 89). + +Fenix would still have three different product flavors: Nightly, Beta, Release. Those would continue to exist for supporting different branding and build configurations. However, the GeckoView version (and release channel) used by Fenix can only be controlled through the Android Component versions i.e., Fenix using AC 89 would always use GeckoView 89 regardless of product flavor. + +In theory release branching (for code freeze and releases) could be automated on the Fenix side too with this change: For every major version bump (AC/GeckoView 89 -> 90) we could automatically cut the release branch and automate updating versions. + +### Example + + + +* `main` tracks GeckoView Nightly 89.0 (`browser-engine-gecko`) +* We ship AC 89.x Nightly versions from `main` every day +* On merge day: + * We cut a `releases/89.0` release branch, which will continue to track GeckoView 89, now as a Beta version. + * `main` continues with the next Nightly version (90.0) +* On the `releases/89.0` branch: + * We update to the latest GeckoView 89.0 beta versions and eventually to GeckoView 89.0 release versions. + * We ship AC 89.0.x versions from the release branch. Initially those will come with Beta versions of GeckoView 89.0 and eventually release versions. + +## Drawbacks + +* _Slower releases_: The initial design of having one component per GeckoView channel allowed independent and fast releases of Android Components. Aligning with the release trains of Mozilla repositories at first seems to cause slower releases. However at this time we do not require faster releases and create them for specific GeckoView releases to be used for specific Fenix releases already. In addition to that we can still uplift and release patches to skip trains - similar to how it is done in Mozilla repositories. It may feel more painful to uplift more complex patches. But at the same time this may be a good mechanism for identifying risky patches that should *not* skip the trains. + +* _Versioning_: While we align on major versions, we will not be fully aligned at first. With the proposal above Android Components 89.0.0 will ship with a GeckoView 89 Beta version and eventually with a later patch version 89.0.x ship GeckoView 89.0 Release version. Right now we do not have any mechanism for marking an Android Components release as "Beta" version. This is something we could add in the future for dot releases. But since it is not required for *this* proposal, it is deliberately not covered here. + +## Rationale and alternatives + +* _Future Changes_: This simplified process will work nicely for our existing products (Fenix and Focus) and in theory should work for other apps too - especially if they have a dependency on GeckoView. However making the proposed changes does not prevent us from making changes in the future. + +## Prior art & resources + +* [Documentation: Current versioning and release process](https://mozac.org/contributing/versioning) +* [Documentation: Current merge day process](https://mozac.org/contributing/merge-day) +* [Firefox/GeckoView release calendar](https://wiki.mozilla.org/Release_Management/Calendar) +* [The Firefox/GeckoView release process](https://wiki.mozilla.org/Release_Management/Release_Process) diff --git a/mobile/android/android-components/docs/rfcs/0008-tab-groups.md b/mobile/android/android-components/docs/rfcs/0008-tab-groups.md new file mode 100644 index 0000000000..1304863ba3 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0008-tab-groups.md @@ -0,0 +1,301 @@ +--- +layout: page +title: Adding tab partitions and groups to BrowserState +permalink: /rfc/0008-tab-groups +--- + +* Start date: 2021-10-25 +* RFC PR: [#11172](https://github.com/mozilla-mobile/android-components/pull/11172) + +## Summary + +Fenix has recently introduced automatic tab groups, as well as a new section for inactive tabs. Both concepts are essentially partitioning tabs for display purposes. This RFC describes a proposal to move these tab partitions and groups to our `BrowserState` so they are stored and managed in the same place as tabs. + +## Motivation + +Currently consumers of Android Components have to manage their own state for tab groups, which has the following disadvantages: + +* Redundant code needs to be written and maintained for managing any grouping of tabs. + +* As the tab groups are detached from the `BrowserState`, this code is also harder to write e.g., consuming applications have to take care of keeping the group state in sync with tab state in the `BrowserStore`. + +* Consuming applications need to persist tab groups and their state separately from the existing tab/session storage, and keep these storages in sync. Note that Fenix doesn't need to do this yet, as groups are inferred based on tab state that is already persisted as part of the Android Component storage, but it's easy to see future use cases where this will be required e.g., user-managed groups. + +* Functionality in Android Components can interfere with partitioning and grouping in applications. Android components currently have no knowledge of how the consuming application decides to group tabs, which leads to feature gaps and bugs that cannot be addressed (or only with suboptimal workarounds.) One example of this is the overlap between inactive tabs and the tab selection logic in our `BrowserState` reducer. Our reducer logic makes sure that there's always a selected tab, so when a tab is closed, another one will be selected. However, in Fenix, when the last active tab is closed, we don't want to select an inactive tab [1]. This is especially true as selecting an inactive tab will cause the `LastAccessMiddleware` in Android Components to update the `lastAccess` time of this tab, which will incorrectly mark it as active again. While a solution to this problem will always involve consuming code i.e., only the application really understands how grouping should work, it becomes much easier if Android Components has a concept of groups. A feature could then be configured not to apply to tabs in a specific group for instance. + +[1] [https://github.com/mozilla-mobile/fenix/issues/21785](https://github.com/mozilla-mobile/fenix/issues/21785) + +## Guide-level explanation + +The proposal is to add a data structure to the existing `BrowserState` that is capable of representing generic tab groups and also allows for preventing undesired overlap of groups targeting independent features. Identifying groups by name isn't sufficient, as an application could use the same name for groups targeting different concepts e.g., an "inactive" group could be used for the inactive tab feature, but "inactive" could also be a search term group. The data structure and APIs we choose should make it hard to get this wrong and always require consuming applications to explicitly specify the "type" of group being targeted. + +To separate these different types of groups, and to prevent a group of groups concept, the proposal is to introduce the concept of a tab *partition*. A partition has a unique name and holds groups for a specific feature e.g., the current search term groups in Fenix would be contained in a partition named `AUTOMATIC_SEARCH_TERM_GROUPS` or similar, with the actual tab groups nested inside this partition grouped by search term. + +In addition to the data structure, we want to provide a set of actions to manipulate (create, update, delete) groups, and to add/remove tabs from groups. It should be easy to implement both automatic grouping mechanisms, as well as manual ones carried out by the user. These actions can be triggered by middlewares for automatic grouping and via `UseCases` when carried out by the user, matching our current architecture. + +## Reference-level explanation + +We will add tab partitions and groups to our browser state without affecting the existing data structures. + +### State + +`BrowserState` will get a new `tabPartitions` property typed as `Map<String, TabPartition>`, mapping the partition ID to a newly introduced type called `TabPartition`. Partitions are used to associate tab groups with a specific feature or use case: + +```Kotlin +data class BrowserState( + val tabs: List<TabSessionState> = emptyList(), // Remains unchanged + val tabPartitions: Map<String, TabPartition> = emptyMap(), // Newly added + .. +) + +/** + * Value type representing a tab partition. Partitions can overlap i.e., a tab + * can be in multiple partitions at the same time. + * + * @property id The ID of a tab partition. This should uniquely identify + * the feature responsible for managing those groups. + * @property tabGroups The groups of tabs in this partition. A partition can + * have one or more groups, depending on use case. Empty partitions will be + * removed by the system. + */ +data class TabPartition( + val id: String, + val tabGroups: List<TabGroup> +) + +/** + * Value type representing a tab group. + * + * @property id The unique ID of this tab group, default to a generated UUID. + * @property name The name of this tab group for display purposes. + * @property persist Whether or not this tab group should be persisted, defaults to false. + * @property tabIds The IDs of all tabs in this group, defaults to empty list. + */ +data class TabGroup( + val id: String = UUID.randomUUID().toString(), + val name: String = "", + val persist: Boolean = false, + val tabIds: List<String> = emptyList() +) +``` + +So, a `TabPartition` is used to relate tab groups to a specific feature, thereby structurally separating it from groups of other features. They are not disjoint i.e., we will allow overlapping partitions. As an example, a tab could be in an "inactive" group, but also retain a group association provided by the user. So if this tab was opened and became active again, it would rejoin its original group. A partition could have one or more groups e.g. for a feature like inactive tabs we could use a single `default` group holding just inactive tabs, or two groups for `active` and `inactive` tabs. This ultimately depends on the use case and how the groups are supposed to be displayed. + +A `TabGroup` contains a list of tabs. We only store the tab IDs, not for performance reasons, but because most of our `TabActions`s are based on IDs and this makes it much easier to use, compared to having to look up the `TabSessionState` every time. We will provide extension functions on `TabGroup` to accept and return `TabSessionState` objects so that consumers can easily look up (or add/remove) the actual tab objects in the group. Note that we're using a `List` (as opposed to a `Set`) to support custom ordering (see [Future Work](#future-work)). A `TabGroup` further has a unique ID and name for display purposes. The `persist` flag can be used to configure whether or not a group should be persisted and restored after an app restart. + +We will also provide extension functions to look up tab groups by name or ID via its partition: + +```Kotlin +/** + * Returns the first tab group matching the provided [name]. Note that we allow + * multiple groups with the same name in a partition but disambiguation needs + * to be handled on a feature level. + */ +fun TabPartition.getGroupByName(name: String) = this.tabGroups.first { + it.name.equals(name, ignoreCase = true) +} + +/** + * Returns the tab groups for the provided [id]. + */ +fun TabPartition.getGroupById(id: String) = this.tabGroups.first { + it.id == id +} +``` + +Here's an example state for illustration purposes. There will be no need to ever write it out this way, except maybe in unit tests: + +```Kotlin +BrowserState( + tabPartitions = mapOf( + AUTOMATIC_SEARCH_TERM_GROUPS to TabPartition( + id = AUTOMATIC_SEARCH_TERM_GROUPS, + tabGroups = listOf( + TabGroup( + name = "running shoes", + tabIds = listOf(...) + ), + TabGroup( + name = "toronto population", + tabIds = listOf(...) + ) + ) + ), + INACTIVE_TABS to TabPartition( + id = INACTIVE_TABS, + tabGroups = listOf( + TabGroup( + name = "default", + tabIds = listOf(...) + ) + ) + ) + ) +) +``` + + +### Actions + +We will add the following actions to manage partitions and groups. This isn't necessarily a complete list but should illustrate the idea: + +```Kotlin +/** + * [BrowserAction] implementations related to updating tab partitions and groups inside [BrowserState]. + */ +sealed class TabGroupAction : BrowserAction() { + + /** + * Adds a new group to [BrowserState.tabPartitions]. If the corresponding partition + * doesn't exist it will be created. + * + * @property partition the ID of the partition the group belongs to. + * @property group the [TabGroup] to add. + */ + data class AddTabGroupAction( + val partition: String, + val group: TabGroup + ) : TabGroupAction() + + /** + * Removes a group from [BrowserState.tabPartitions]. Note that empty partitions + * will be removed. + * + * @property partition the ID of the partition the group belongs to. + * @property group the ID of the group to remove. + */ + data class RemoveTabGroupAction( + val partition: String, + val group: String + ) : TabGroupAction() + + /** + * Adds the provided tab to a group in [BrowserState]. + * + * @property partition the ID of the partition the group belongs to. If the corresponding + * partition doesn't exist it will be created. + * @property group the ID of the group. + * @property tabId the ID of the tab to add to the group. + */ + data class AddTabAction( + val partition: String, + val group: String, + val tabId: String, + ) : TabGroupAction() + + /** + * Adds the provided tabs to a group in [BrowserState]. + * + * @property partition the ID of the partition the group belongs to. If the corresponding + * partition doesn't exist it will be created. + * @property group the ID of the group. + * @property tabIds the IDs of the tabs to add to the group. + */ + data class AddTabsAction( + val partition: String, + val group: String, + val tabIds: List<String> + ) : TabGroupAction() + + /** + * Removes the provided tab from a group in [BrowserState]. + * + * @property partition the ID of the partition the group belongs to. If the corresponding + * partition doesn't exist it will be created. + * @property group the ID of the group. + * @property tabId the ID of the tab to remove from the group. + */ + data class RemoveTabAction( + val partition: String, + val group: String, + val tabId: String + ) : TabGroupAction() + + /** + * Removes the provided tabs from a group in [BrowserState]. + * + * @property partition the ID of the partition the group belongs to. If the corresponding + * partition doesn't exist it will be created. + * @property group the ID of the group. + * @property tabIds the IDs of the tabs to remove from the group. + */ + data class RemoveTabsAction( + val partition: String, + val group: String, + val tabIds: List<String> + ) : TabGroupAction() +} +``` + +The purpose of these actions is described in the KDocs above, but should not deviate from our current way of doing things. See next chapters for more details. + +### Automatic grouping with middlewares + +Middlewares can be used to implement automatic tab grouping functionality, as they can react to state changes in the system and dispatch the corresponding group actions. Here is an example of how automatic search term grouping could be implemented today using the actions described above: + +```Kotlin +/** + * This [Middleware] manages tab groups for search terms. + */ +class SearchTermTabGroupMiddleware : Middleware<BrowserState, BrowserAction> { + override fun invoke( + context: MiddlewareContext<BrowserState, BrowserAction>, + next: (BrowserAction) -> Unit, + action: BrowserAction + ) { + when (action) { + is HistoryMetadataAction.SetHistoryMetadataKeyAction -> { + action.historyMetadataKey.searchTerm?.let { searchTerms -> + context.dispatch(TabGroupAction.AddTabAction(SEARCH_TERM_GROUPS, searchTerms, action.tabId)) + } + } + is HistoryMetadataAction.DisbandSearchGroupAction -> { + val group = context.state.tabPartitions[SEARCH_TERM_GROUPS]?.getGroupByName(action.searchTerm) + group?.let { + context.dispatch(TabGroupAction.RemoveTabGroupAction(SEARCH_TERM_GROUPS, it.id)) + } + } + } + + next(action) + } + +``` + +This middleware automatically adds and removes tabs once they're associated/disassociated with search terms based on history metadata. `AUTOMATIC_SEARCH_TERM_GROUPS` is the ID of the partition, the search terms (e.g. "Toronto") are used as group names. + +Using middlewares provides all the flexibility we could need e.g., a middleware can put tabs in one or multiple groups, and generally is capable of observing all state changes in the system for grouping/ungrouping purposes. + +### Manual grouping with UseCases + +I am not adding a specific code example here, as this is following our existing architecture e.g., `TabsUseCases` dispatches a `TabListAction.RemoveTabAction(tabId)` when the `removeTab` use case is invoked. We would provide a set of similar use cases that in turn dispatch the corresponding actions described above. So, `TabGroupUseCases` would dispatch a `RemoveTabAction(partition, group, tabId)` when the corresponding `removeTab` use case is invoked. These `UseCases` can be used by multiple features to implement manual grouping mechanisms. This will also allow for implementing overlapping use cases between automatic and manual groups e.g., a tab could be automatically grouped, but moved to another group by the user. + +### Reducers + +We will need to introduce a new `Reducer` for the tab group actions outlined above. We will also need to either update our existing `TabListReducer` or handle `TabListAction`s in our new `TabGroupReducer`to make sure we disassociate tabs from their groups when they get deleted. Keeping this state in sync is not something we should let every single middleware or feature to deal with itself. It's relatively easy to handle this as part of our reducers. + +### Persistence + +We should also support persisting tab partitions and groups using our `BrowserStateWriter` and `BrowserStateReader`. We currently plan to make this configurable on a group level. For some groups it may make more sense to recreate them in memory as needed e.g., for search term groups we already store the required metadata as part of tabs so it would be easy to recreate the groups on startup without having to persist additional data. The same is true for inactive tabs. + + +## Alternatives + +* We could consider flattening this data structure. So, instead of the nested Partition/Group structure, we could use a single `Map<GroupKey, List<String>>` where the `GroupKey` contains both the name of the group, as well as a `type` or `feature` identifier. This would make the actions somewhat easier, as we could remove the `partition` parameter, but it would also be easier to make mistakes. An application could more easily look up or provide the wrong group, and it feels a bit unorganized :). + + +## Future Work + +* We have discussed separating private tabs into their own collection in `BrowserState`. This work could also be combined with the proposal here to have normal and private tab groups within a partition. Alternatively, we could also introduce new top level types for `PrivateTabs` and `NormalTabs` with the corresponding partitions nested inside. Then the data structure described here wouldn't need to change. This discussion and refactoring may be best done separately though. + +* We have recently introduced actions to move / re-order tabs [2]. Today these actions can not be used in combination with tab groups. The proposal here would allow for it though, as we could introduce versions of these actions that apply to partitions/groups. + +[2] [https://github.com/mozilla-mobile/android-components/pull/10936/](https://github.com/mozilla-mobile/android-components/pull/10936/) + + +## Questions + +* Is this design too simple? Does it cover all use cases we currently anticipate? + * We worked through this in this RFC and decided that this would cover our current use cases as far as we know. One case that is not covered with this is supporting an ordered but mixed display of tabs and groups. We have no way of positioning a group within a list of tabs and vice versa, but this is outside the scope of this RFC. +* Do we need to implement persistence now or should we punt on this and handle in a later iteration? + * We decided to make persistence configurable on a group level. The actual implementation of this can land in multiple iterations as we currently don't need to persist groups. diff --git a/mobile/android/android-components/docs/rfcs/0009-remove-interactors-and-controllers.md b/mobile/android/android-components/docs/rfcs/0009-remove-interactors-and-controllers.md new file mode 100644 index 0000000000..9514154adb --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0009-remove-interactors-and-controllers.md @@ -0,0 +1,401 @@ +--- +layout: page +title: Remove Interactors and Controllers +permalink: /rfc/0009-remove-interactors-and-controllers +--- + +* Start date: 2022-03-29 +* RFC PR: [1466](https://github.com/mozilla-mobile/firefox-android/pull/1466) + +## Summary + +Now that Fenix has been fully migrated to rely on Android Component's `lib-state` library, Fenix's architecture can be further simplified by refactoring view components to directly use our Redux-like store, instead of going through various layers of `Interactor`s and `Controller`s. + +## Motivation + +The `Interactor/Controller` types, as a design pattern, have recently had their usefulness and clarity come into question in reviews and conversations with some regularity. `Interactor`s in particular seem to often be an unnecessary abstraction layer, usually containing methods that call similarly named methods in a `Controller` that share a similar name to the interactor calling them. Interactors that are used solely to delegate to other classes will be referred to as 'passthrough' interactors throughout this document. + +The [definition](../../fenix/docs/architecture-overview.md#interactor) of interactors in our architecture overview indicate that this is at least partially intentional: 'Called in response to a direct user action. Delegates to something else'. This is even referenced as a [limitation at the end of the overview](../../fenix/docs/architecture-overview.md#known-limitations). + +Historically, the interactor/controller pattern evolved from a Presenter/Controller/View pattern that originated in Android Components. The underlying motivation of that pattern was to separate code presenting the state from the view (Presenters) and the code that updated the state from the view (Controllers). + +Generally, the interactor/controller pattern is also intended to move business logic out of classes tied to the Android framework and into classes that are more testable, reusable, composable, digestible, and which handle a single responsibility. More reading on architecture goals is available [in our Architecture Decisions](../../fenix/docs/Architecture-Decisions.md#goals). + +These goals are all met reasonably well by the existing pattern, but it also contributes to unnecessary class explosion, general code complexity, and confusion about responsibility for each architectural type. This becomes especially true when wondering how responsibility should be split between interactors and controllers. It seems that interactors are often included as a matter of precedent and not to necessarily facilitate all the goals mentioned above, and this has lead to a large amount passthrough interactors. + +### Proposal goals + +1. Increase code comprehensibility +2. Have clear delineation of responsibility between architectural components +3. Retain ability to manage state outside of fragments/activities/etc +4. Ability for proposal to be adopted incrementally + +### Further contextual reading + +An investigation was previously done to provide [further context](https://docs.google.com/document/d/1vwERcAW9_2LkcYENhnA5Kb-jT_nBTJNwnbD5m9zYSA4/edit#heading=h.jjoifpwhhxgk) on the current state of interactors. To summarize findings _in Fenix specifically_: +1. 29/36 concrete `Interactor` classes are strict passthrough interactors or are passthrough interactors that additionally record metrics. +2. Other interactors seem to have mixed responsibilities, including but not limited to: + - dispatching actions to `Store`s + - initiating navigation events + - delegating to `UseCase`s or directly to closures instead of `Controller`s. + +--- + +## Guide-level explanation + +The proposal is to remove interactors and controllers completely from the codebase. Their usages will be replaced with direct Store observations and direct action dispatches to those Stores. All state changes would be handled by reducers, and side-effects like telemetry or disk writes would be handled in middlewares. + +This would address all the goals listed above: +1. Code comprehensibility should be improved by having a single architectural pattern. All business logic would be discoverable within `Reducer`s, and changes to `State` would be much more explicit. +2. Responsibility would be clearly delineated as all business logic would be handled by `Reducer`s and all side-effects by `Middleware`, instead of being scattered between those components as well as `Interactor`s, `Controller`s, and various utility classes. +3. `State` management would happen within `Reducer`s and would still accomplish the usual goals around testability. +4. Refactoring interactors/controllers to instead dispatch actions and react to state changes should be doable on a per-component basis. + +Additionally, there are tons of prior art discussing the benefits of a unidirectional data flow, including the [Redux documentation](https://redux.js.org/tutorials/fundamentals/part-2-concepts-data-flow). + +--- + +## Reference-level explanation + +Throughout the app are examples of chains of interactors and controllers which lead to a method that dispatches events to a Store anyway. Simplifying this mental model should allow faster code iteration and code navigation. + +For one example, here is the code path that happens when a user long-clicks a history item: + +```kotlin +// 1. in LibrarySiteItemView +setOnClickListener { + val selected = holder.selectedItems + when { + selected.isEmpty() -> interactor.open(item) + item in selected -> interactor.deselect(item) + // "select" is the path we are following + else -> interactor.select(item) + } +} + +// 2. Clicking into `interactor.select` will lead to SelectionInteractor<T>. +// 3. Searching for implementers of that interface will lead us to another interface, `HistoryInteractor : SelectionInteractor<History>` +// 4. DefaultHistoryInteractor implements HistoryInteractor +override fun open(item: History) { + historyController.handleSelect(item) +} + +// 5. Clicking into `handleSelect` leads to `HistoryController` +// 6. DefaultHistoryController implements `HistoryController::handleSelect` +override fun handleSelect(item: History) { + if (store.state.mode === HistoryFragmentState.Mode.Syncing) { + return + } + + store.dispatch(HistoryFragmentAction.AddItemForRemoval(item)) +} + +// 7. reducer handles state update +private fun historyStateReducer( + state: HistoryFragmentState, + action: HistoryFragmentAction, +): HistoryFragmentState { + return when (action) { + is HistoryFragmentAction.AddItemForRemoval -> + state.copy(mode = HistoryFragmentState.Mode.Editing(state.mode.selectedItems + action.item)) + ... + } +} +``` + +Following this proposal, the above would change to something like: + +```kotlin +// 1. in LibrarySiteItemView +setOnClickListener { + when { + selected.isEmpty() -> store.dispatch(HistoryFragmentAction.AddItemForRemoval(item)) + ... + } +} + +// 2. reducer handles state +private fun historyStateReducer( + state: HistoryFragmentState, + action: HistoryFragmentAction, +): HistoryFragmentState { + return when (action) { + is HistoryFragmentAction.AddItemForRemoval(item) -> when (state.mode) { + HistoryFragmentState.Mode.Syncing -> state + else -> state.copy( + mode = HistoryFragmentState.Mode.Editing(state.mode.selectedItems + action.item) + ) + } + ... + } +} +``` + +This reduces the number of layers in the code path. Note also that business logic around whether to update based on Sync status is incorporated locally in the Reducer. In this example, the details of observing state from the store are complicated by the additional view hierarchies required by recycler views, so they have been omitted for brevity. + +--- + +### Reacting to changes: state observations and side-effects in XML + +Generally speaking, there are two opportunities to launch side-effects in a Redux pattern: +1. Reacting to a user-initiated action +2. Reacting to the result of a state change + +The first will be covered in more detail below and is handled by middleware, where side-effects are started in reaction to some dispatched action. + +The second requires a mechanism of observation for a Store's state, triggering some logic or work in response to state updates. On Android these observations are often started in the UI layer in order to tie them to lifecycle events. However, UI framework components like activities, fragments, and views are notoriously difficult to test and we want to avoid a situation where these classes become too large or complex. + +There are already some existing mechanisms for doing this included in the lib state library: + +1. `Fragment.consumeFrom` will setup a Store observation that is triggered on _every_ state update in the Store. The [HistoryFragment updating its view hierarchy with all state updates](https://github.com/mozilla-mobile/firefox-android/blob/910afa889ebc5222a1b9a877837de5c55244d441/fenix/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt#L196) is one example. +2. `Flow.distinctUntilChanged` can be combined with things like `Fragment.consumeFlow` or `Store.flowScoped` for finer-grained state observations. This will allow reactions to specific state properties updates, like when the [HistorySearchDialogFragment changes the visibility of the AwesomeBar](https://github.com/mozilla-mobile/firefox-android/blob/910afa889ebc5222a1b9a877837de5c55244d441/fenix/app/src/main/java/org/mozilla/fenix/library/history/HistorySearchDialogFragment.kt#L189). + +While both of these options are fine in many situations, they would both be difficult to setup to test. For a testable option, consider inheriting from [AbstractBinding](https://github.com/mozilla-mobile/firefox-android/blob/1cd69fec56725410af855eda923ab1afe99ae0b2/android-components/components/lib/state/src/main/java/mozilla/components/lib/state/helpers/AbstractBinding.kt#L23). This allows for defining a class that accepts dependencies and is isolated, making unit tests much easier. For some examples of that, see the [SelectedItemAdapterBinding](https://github.com/mozilla-mobile/firefox-android/blob/910afa889ebc5222a1b9a877837de5c55244d441/fenix/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt#L61) or the [SwipeToDeleteBinding](https://github.com/mozilla-mobile/firefox-android/blob/910afa889ebc5222a1b9a877837de5c55244d441/fenix/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTrayList.kt#L35-L37) which are both used in the XML version of the Tabs Tray. + +For more details, see the example refactor in the [example refactor section](#an-example-refactor-removing-interactors-and-controllers-from-historyfragment). + +--- + +### Moving into the future: using lib-state with Compose + +Ideally, fragments or top-level views/Composables would register observers of their Store's state and send updates from those observers to their child Composables along with closures containing dispatches to those Stores. We are already close to being familiar with this pattern in some places. The following is a current example that has been edited for brevity and clarity: + +```kotlin +class PocketCategoriesViewHolder( + composeView: ComposeView, + viewLifecycleOwner: LifecycleOwner, + private val interactor: PocketStoriesInteractor, +) : ComposeViewHolder(composeView, viewLifecycleOwner) { + + @Composable + override fun Content() { + val categories = components.appStore + .observeAsComposableState { state -> state.pocketStoriesCategories }.value + val categoriesSelections = components.appStore + .observeAsComposableState { state -> state.pocketStoriesCategoriesSelections }.value + + PocketTopics( + categoryColors = categoryColors, + categories = categories ?: emptyList(), + categoriesSelections = categoriesSelections ?: emptyList(), + onCategoryClick = interactor::onCategoryClicked, + ) + } +} +``` + +Here, we already see a view registering Store observers appropriately. The only thing that would need to change is the line calling the interactor, `onCategoryClick = interactor::onCategoryClicked,`. Following this line leads down a similar chain of abstraction layers, and eventually leads to `DefaultPocketStoriesController::handleCategoryClicked`. This function contains business logic which would need to be moved to the reducer and telemetry side-effects that would need to be moved to a middleware, but the ultimate intent is to toggle the selected category. We skip many layers of indirection by dispatching that action directly from the view: + +```kotlin +class PocketCategoriesViewHolder( + composeView: ComposeView, + viewLifecycleOwner: LifecycleOwner, +) : ComposeViewHolder(composeView, viewLifecycleOwner) { + + @Composable + override fun Content() { + val categories = components.appStore + .observeAsComposableState { state -> state.pocketStoriesCategories }.value + val categoriesSelections = components.appStore + .observeAsComposableState { state -> state.pocketStoriesCategoriesSelections }.value + + PocketTopics( + categoryColors = categoryColors, + categories = categories ?: emptyList(), + categoriesSelections = categoriesSelections ?: emptyList(), + onCategoryClick = { name -> + components.appStore.dispatch(AppAction.TogglePocketStoriesCategory(name)) + }, + ) + } +} +``` + +This should simplify the search for underlying logic by: +- moving business logic into an expected and consistent component (the reducer) +- moving side-effects into a consistent and centralized component (a middleware, which could even handle telemetry for the entire store) + +--- + +### Extending the example: separating state and side-effects + +To demonstrate the bullets above, here is the method definition in the `DefaultPocketStoriesController` that currently handles the business logic initiated from the `interactor::onCategoryClicked` call above. + +```kotlin + override fun handleCategoryClick(categoryClicked: PocketRecommendedStoriesCategory) { + val initialCategoriesSelections = appStore.state.pocketStoriesCategoriesSelections + + // First check whether the category is clicked to be deselected. + if (initialCategoriesSelections.map { it.name }.contains(categoryClicked.name)) { + appStore.dispatch(AppAction.DeselectPocketStoriesCategory(categoryClicked.name)) + Pocket.homeRecsCategoryClicked.record( + Pocket.HomeRecsCategoryClickedExtra( + categoryName = categoryClicked.name, + newState = "deselected", + selectedTotal = initialCategoriesSelections.size.toString(), + ), + ) + return + } + + // If a new category is clicked to be selected: + // Ensure the number of categories selected at a time is capped. + val oldestCategoryToDeselect = + if (initialCategoriesSelections.size == POCKET_CATEGORIES_SELECTED_AT_A_TIME_COUNT) { + initialCategoriesSelections.minByOrNull { it.selectionTimestamp } + } else { + null + } + oldestCategoryToDeselect?.let { + appStore.dispatch(AppAction.DeselectPocketStoriesCategory(it.name)) + } + + // Finally update the selection. + appStore.dispatch(AppAction.SelectPocketStoriesCategory(categoryClicked.name)) + + Pocket.homeRecsCategoryClicked.record( + Pocket.HomeRecsCategoryClickedExtra( + categoryName = categoryClicked.name, + newState = "selected", + selectedTotal = initialCategoriesSelections.size.toString(), + ), + ) + } +``` + +If we break this down into a pseudocode algorithm, we get the following steps: +1. Read the current state from the store +2. If the category is being deselected + 1. Dispatch an action to update the state + 2. Record a metric that the category was deselected + 3. Return early +3. If the number of categories selected would be over max + 1. Dispatch an action deselecting the oldest +4. Dispatch an action with the newly selected category +5. Record a metric that a category was selected + +In order to transform this algorithm into an appropriate usage of the Redux pattern, we can separate this list into two parts: steps that have side-effects and steps which can be represented by pure state updates. In this case, only the steps that record metrics represent side-effects. This will mean that those steps should be moved into a middleware, and the rest can be moved into a reducer. + +Here's how that might look: +```kotlin +// add a new AppAction +data class TogglePocketStoriesCategory(val categoryName: String) : AppAction() + +// add a case to handle it in a middleware. For example, let's put the following in MetricsMiddleware: +private fun handleAction(action: AppAction) = when (action) { + ... + is AppAction.TogglePocketStoriesCategory -> { + val currentCategoriesSelections = currentState.pocketStoriesCategoriesSelections.map { it.name } + // check if the category is being deselected + if (currentCategoriesSelections.contains(action.categoryName)) { + Pocket.homeRecsCategoryClicked.record( + Pocket.HomeRecsCategoryClickedExtra( + categoryName = action.categoryName, + newState = "deselected", + selectedTotal = currentCategoriesSelections.size.toString(), + ), + ) + } else { + Pocket.homeRecsCategoryClicked.record( + Pocket.HomeRecsCategoryClickedExtra( + categoryName = action.categoryName, + newState = "selected", + selectedTotal = currentCategoriesSelections.size.toString(), + ), + ) + } + } +} + + +// finally, we can shift all the state updating logic into the reducer +fun reduce(state: AppState, action: AppAction): AppState = when (action) { + ... + is AppAction.TogglePocketStoriesCategory -> { + val currentCategoriesSelections = state.pocketStoriesCategoriesSelections.map { it.name } + val updatedCategoriesState = when { + currentCategoriesSelections.contains(action.categoryName) -> { + state.copy( + pocketStoriesCategoriesSelections = state.pocketStoriesCategoriesSelections.filterNot { + it.name == action.categoryName + }, + ) + } + currentCategoriesSelections.size == POCKET_CATEGORIES_SELECTED_AT_A_TIME_COUNT -> { + state.copy( + pocketStoriesCategoriesSelections = state.pocketStoriesCategoriesSelections + .minusOldest() + .plus( + PocketRecommendedStoriesSelectedCategory( + name = action.categoryName, + ) + ), + ) + } + else -> { + state.copy( + pocketStoriesCategoriesSelections = + state.pocketStoriesCategoriesSelections + + PocketRecommendedStoriesSelectedCategory(action.categoryName) + ) + } + } + // Selecting a category means the stories to be displayed needs to also be changed. + updatedCategoriesState.copy( + pocketStories = updatedCategoriesState.getFilteredStories(), + ) + } +} +``` + +--- + +### Interacting with the storage layer through middlewares + +What if a feature requires some async data for its initial state, or needs to write changes to disk? There are no restrictions on running impure methods in middleware, so we can respond to an action that would make changes to the storage layer from the middleware. A good example is [ContainerMiddleware](https://github.com/mozilla-mobile/firefox-android/blob/5d773ae7710265ab98e15efc43b1afb17ab2e404/android-components/components/feature/containers/src/main/java/mozilla/components/feature/containers/ContainerMiddleware.kt) + +Note that the `InitAction` here subscribes to a `Flow` from the database, which will continue to collect updates from the storage layer and dispatch actions to change the state accordingly. + + +Overall, this should convey the following improvements +- All state updates are guaranteed to be pure and in a single component, allowing for easy testing and reproduction. +- All side-effects are logically grouped into middlewares, allowing for testing strategies specific to the type of side-effect. +- Several indirect abstraction layers are removed, minimizing mental model of code. + +--- + +### An example refactor: removing interactors and controllers from HistoryFragment + +A [small example refactor](https://github.com/mozilla-mobile/firefox-android/pull/2347) was done to demonstrate some of the concepts presented throughout the document by removing interactors and controllers from the `HistoryFragment` and package. This is not necessarily an idealized version of that area, but is instead intended to demonstrate the first steps in creating architectural consistency. + +--- + +## Drawbacks + +Redux-like patterns have a bit of a learning curve, and some problems can be more difficult to solve than others. For example, handling side-effects like navigation or loading state from disk. The benefits of streamlining our architecture should outweigh this, especially once demonstrative examples of solving these problems are common in the codebase. + +Additionally, the current implementation of our `Store` requires that dispatches are handled on a separate thread specific to the Store. This may change in the future, but also introduces some particular drawbacks: +- it can be harder to conceptualize the concurrency model, since there are no synchronous actions. +- thread switching has a performance impact. +- testing is more cumbersome by requiring additional async methods. + +--- + +## Proposal acceptance and future plans + +To demonstrate some of the concepts discussed in this proposal in more detail, an example patch will be produced that shows a refactor of a prominent feature of the app - history. + +Following that, should this proposal be accepted the following suggestions are made for adoption with a focus on increasing the team's familiarity and knowledge of the pattern: + +1. Architecture documentation should be updated with guidelines matching this proposal. +1. A number (1-3) of refactors should be planned to help demonstrate the pattern, especially in regards to things like using middleware to handle side-effects, how to structure local vs global state, and how to propagate state and actions to Composables and XML views. +1. Where possible, new feature work should require refactoring to the established architecture before feature implementation. +1. Investigate further improvements to the architecture. For example, a "single Store" model that more closely follows Redux patterns. + +### Consolidating Stores + +Multiple stores allows us to manage memory with less investment. For example, state for a Settings UI screen can be held in a `SettingsStore` which can be cleared from memory (garbage collected) when no longer in that UI context. + +This will mean that we will continue to have "global" stores like the `AppStore` and the `BrowserStore` co-existing with "local" stores like the `HistoryFragmentStore`. This can introduce complexity in terms of determining the best place to add state, middlewares, and actions. + +Moving into the future, we will investigate options for consolidating our stores. It should be relatively straightforward to at least combine our global stores. Investigation is also ongoing into whether we can create "scoped" stores that combine a local store with the global store such that the local portion of the store can still automatically fall out of scope and respect the constraints of developing on mobile. diff --git a/mobile/android/android-components/docs/rfcs/0010-add-state-based-navigation.md b/mobile/android/android-components/docs/rfcs/0010-add-state-based-navigation.md new file mode 100644 index 0000000000..a0de762244 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0010-add-state-based-navigation.md @@ -0,0 +1,167 @@ +--- +layout: page +title: Add State-based navigation +permalink: /rfc/0010-add-state-based-navigation +--- + +* Start date: 2023-10-17 +* RFC PR: [4126](https://github.com/mozilla-mobile/firefox-android/pull/4126) + +## Summary + +Following the acceptance of [0009 - Remove Interactors and Controllers](0009-remove-interactors-and-controllers), Fenix should have a method of navigation that is tied to the `lib-state` model to provide a method of handling navigation side-effects that is consistent with architectural goals. This architecture is defined at a high-level [here](https://github.com/mozilla-mobile/firefox-android/blob/9edfde0a1382b4d5fb4342792d98d4c1d4d41bef/fenix/docs/architecture-overview.md) and has example code in [this folder](https://github.com/mozilla-mobile/firefox-android/tree/9edfde0a1382b4d5fb4342792d98d4c1d4d41bef/fenix/docs/architectureexample). + +## Motivation + +Currently, methods of navigation throughout the app are varied. The `SessionControlController` provides [3 examples](https://searchfox.org/mozilla-mobile/rev/aa6bee71a6e0ea73f041a54ddf4d5d4e2f603429/firefox-android/fenix/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt#180) alone: + +- `HomeActivity::openToBrowserAndLoad` +- Calling a `NavController` directly +- Callbacks like `showTabTray()` + +To move to a more consistent Redux-like model, we need a way for features to fire `Action`s and have that result in navigation. This will help decouple our business logic from the Android platform, where a key example of this would be references to the `HomeActivity` throughout the app in order to access the `openToBrowserAndLoad` function. + +## Proposal + +Moving forward, navigation should be initiated through middlewares that respond to `Action`s. How these middleware handle navigation side-effects can be addressed on a per-case basis, but this proposal includes some generalized advice for 3 common use cases. + +### 1. Screen-based navigation + +For screen-based navigation between screens like the settings pages or navigation to the home screen, middlewares should make direct use of a `NavController` that is hosted by the fragment of the current screen's scope. + +For a hypothetical example: +```kotlin + +sealed class HistoryAction { + object HomeButtonClicked : HistoryAction() + data class HistoryGroupClicked(val group: History.Group) : HistoryAction() +} + +class HistoryNavigationMiddleware(private val getNavController: () -> NavController) : Middleware<HistoryState, HistoryAction> { + override fun invoke( + context: MiddlewareContext<HistoryState, HistoryAction>, + next: (HistoryFragmentAction) -> Unit, + action: HistoryFragmentAction, + ) { + next(action) + when (action) { + is HomeButtonClicked -> getNavController().navigate(R.id.home_fragment) + is HistoryGroupClicked -> getNavController().navigate(R.id.history_metadata_fragment) + } + } +} +``` + +This should translate fairly easily to the Compose world. This example intentionally ignores passing the `group` through the navigation transition. It should be fairly trivial to convert data types to navigation arguments, or consider creating Stores with a scope large enough to maintain state across these transitions. + +Note also the use of a lambda to retrieve the `NavController`. This should help avoid stale references when Stores outlive their parent fragment by using a `StoreProvider`. + +### 2. Transient effects + +Transient effects can be handled by callbacks provided to a middleware. To build on our previous example: + +```kotlin +sealed class HistoryAction { + object HomeButtonClicked : HistoryAction() + data class HistoryGroupClicked(val group: HistoryItem.Group) : HistoryAction() + data class HistoryItemLongClicked(val item: HistoryItem) : HistoryAction() +} + +class HistoryUiEffectMiddleware( + private val displayMenuForItem: (HistoryItem) -> Unit, +) : Middleware<HistoryState, HistoryAction> { + override fun invoke( + context: MiddlewareContext<HistoryState, HistoryAction>, + next: (HistoryFragmentAction) -> Unit, + action: HistoryFragmentAction, + ) { + next(action) + when (action) { + is HistoryItemLongClicked -> displayMenuForItem(action.item) + is HomeButtonClicked, HistoryGroupClicked -> Unit + } + } +} +``` + +### 3. The special case of `openToBrowserAndLoad` + +Finally, we want a generally re-usable method of opening a new tab and navigating to the `BrowserFragment`. Fragment-based Stores can re-use a (theoretical) delegate to do so. + +```kotlin +sealed class HistoryAction { + object HomeButtonClicked : HistoryAction() + data class HistoryGroupClicked(val group: History.Group) : HistoryAction() + data class HistoryItemLongClicked(val item: HistoryItem) : HistoryAction() + data class HistoryItemClicked(val item: History.Item) : HistoryAction() +} + +class HistoryNavigationMiddleware( + private val browserNavigator: BrowserNavigator, + private val getNavController: () -> NavController, +) : Middleware<HistoryState, HistoryAction> { + override fun invoke( + context: MiddlewareContext<HistoryState, HistoryAction>, + next: (HistoryFragmentAction) -> Unit, + action: HistoryFragmentAction, + ) { + next(action) + when (action) { + is HomeButtonClicked -> navController.navigate(R.id.home_fragment) + is HistoryGroupClicked -> navController.navigate(R.id.history_metadata_fragment) + is HistoryItemClicked -> browserNavigator.openToBrowserAndLoad(action.item) + is HistoryItemLongClicked -> Unit + } + } +} +``` + +This delegate would wrap the current behavior exposed by `HomeActivity::openToBrowserAndLoad`, looking something roughly like: + +```kotlin +class BrowserNavigator( + private val addTabUseCase: AddNewTabUseCase, + private val loadTabUseCase: DefaultLoadUrlUseCase, + private val searchUseCases: SearchUseCases, + private val navController: () -> NavController, +) { + // logic to navigate to browser fragment and load a tab +} +``` + +## Alternatives + +### 1. Observing navigation State from a AppStore through a binding in HomeActivity. + +This was the previous proposal for this RFC. An example would roughly be: + +```kotlin +sealed class AppAction { + object NavigateHome : AppAction() +} + +data class AppState( + val currentScreen: Screen +) + +fun appReducer(state: AppState, action: AppAction): AppState = when (action) { + is NavigateHome -> state.copy(currentScreen = Screen.Home) +} + +// in HomeActivity +private val navigationObserver by lazy { + object : AbstractBinding<AppState>(components.appStore) { + override suspend fun onState(flow: Flow<AppState>) = flow + .distinctUntilChangedBy { it.screen } + .collectLatest { /* handleNavigation */ } + } +} +``` + +However, this implies some several issues: +1. We end up replicating the state of a `NavController` manually in a our custom State, risking out-of-sync issues. +2. We lose specificity of Actions by generalizing them globally. For example, instead of a `ToolbarAction.HomeClicked`, it would encourage re-use of a single `AppAction.NavigateHome`. Though seemingly convenient at first, it implies downstream problems for things like telemetry. To know where the navigation to home originated from, we would need to include additional properties (like direction) in the `Action`. Any future changes to the behavior of these Actions would need to be generalized for the whole app. + +### 2. Global navigation middleware attached to the AppStore. + +This carries risk of the 2 issue listed above, and runs into immediate technical constraints. When the `AppStore` is constructed in `Core`, we do not have reference to an `Activity` and cannot retrieve a `NavController`. This could be mitigated by a mutable property or lazy getter that is set as Fragments or the Activity come into and out of scope. The current proposal will localize navigation transitions to feature areas which should keep them isolated in scope. diff --git a/mobile/android/android-components/docs/rfcs/0011-decouple-home-activity-and-external-app-browser-activity.md b/mobile/android/android-components/docs/rfcs/0011-decouple-home-activity-and-external-app-browser-activity.md new file mode 100644 index 0000000000..e429889f9c --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0011-decouple-home-activity-and-external-app-browser-activity.md @@ -0,0 +1,163 @@ +--- +layout: page +title: Decouple HomeActivity and ExternalAppBrowserActivity +permalink: /rfc/0011-decouple-home-activity-and-external-app-browser-activity +--- + +* Start date: 2023-12-06 +* RFC PR: [#4732](https://github.com/mozilla-mobile/firefox-android/pull/4732) + +## Summary +The Custom Tabs feature is partially implemented by `ExternalAppBrowserActivity` and `AuthCustomTabActivity`. The use of inheritance in these `Activity`s contributes to a closely coupled system. + +The aim of this RFC is to strive towards a single `Activity` architecture as is [recommended for Modern Android Development (MAD)](https://developer.android.com/topic/architecture/recommendations#ui-layer). + +The initial tasks will aim to: +1. reduce the dependencies between `ExternalAppBrowserActivity` and `HomeActivity` +2. establish clear responsibilities in `HomeActivity` +3. provide a foundation on which to continue refactoring to a single `Activity` + +⚠️ This work is exclusively a refactor of the current implementation, there should be no changes to existing behavior or features. + +## Motivation +Fenix has [numerous open bugs and feature requests relating to the Custom Tabs feature](https://bugzilla.mozilla.org/show_bug.cgi?id=1815918). To facilitate efficient implementation & debugging of these (and future) bugs we should create code that is easy to maintain. Some notable issues: + +1. A close coupling of `ExternalAppBrowserActivity` & `AuthCustomTabActivity` with the `HomeActivity`, which increases the likelihood of: + * difficult to detect bugs + * difficultly debugging + * unexpected behavior + * making the area resistant to change + * confusion of responsibility + +2. `HomeActivity` has a mix of responsibilities, is bloated and monolithic. It contains unused fields, some incorrect visibility modifiers and generally in need of some house-keeping. These issues may be partly due to it's own downstream dependencies. See [0010-add-state-based-navigation.md](https://github.com/mozilla-mobile/firefox-android/blob/1c8de2d2fa7226f568cd49b2161bb9b5471cbcb5/docs/rfcs/0010-add-state-based-navigation.md) for reading around the navigation issue which includes `HomeActivity`. + +3. Inheritance has potential to introduce confusion, for example `ExternalAppBrowserFragment` already has duplicate dependencies defined for `UserInteractionHandler`. + +4. `ExternalAppBrowserActivity` contains multiple no-op functions. + +### Activity architecture overview +**Note**: This is an abridged abstraction of the mentioned *classes* focusing on only the relevant `public` and `protected` APIs. + +```mermaid +classDiagram + + class LocaleAwareAppCompatActivity { + override fun attachBaseContext() calls super + override fun onCreate() calls super + } + AppCompatActivity <|-- LocaleAwareAppCompatActivity + + class NavHostActivityInterface { + fun getSupportActionBarAndInflateIfNecessary() + } + + class HomeActivity { + override fun getSupportActionBarAndInflateIfNecessary() + override fun onCreate() calls super + override fun onResume() calls super + override fun onStart() calls super + override fun onStop() calls super + override fun onPause() calls super + override fun onProvideAssistContent() calls super + override fun onDestroy() calls super + override fun onConfigurationChanged() calls super + override fun recreate() calls super + override fun onNewIntent() calls super + override fun onCreateView() calls super + override fun onActionModeStarted() calls super + override fun onActionModeFinished() calls super + override fun onBackPressed() + override fun onActivityResult() calls super + override fun onKeyDown() calls super + override fun onKeyUp() calls super + override fun onKeyLongPress() calls super + override fun onUserLeaveHint() calls super + open fun getNavDirections() + open fun getBreadcrumbMessage() + open fun getIntentSource() + open fun getIntentSessionId() + } + NavHostActivityInterface <|-- HomeActivity + LocaleAwareAppCompatActivity <|-- HomeActivity + + class ExternalAppBrowserActivity { + override fun onResume() calls super + override fun onDestroy() calls super + override fun onProvideAssistContent() calls super + + override fun getNavDirections() + override fun getBreadcrumbMessage() + override fun getIntentSource() + override fun getIntentSessionId() + } + HomeActivity <|-- ExternalAppBrowserActivity + + class AuthCustomActivity { + override fun onResume() calls super + } + ExternalAppBrowserActivity <|-- AuthCustomActivity +``` + +#### Observations + +1. `HomeActivity` is used as a 'base' `Activity` for `ExternalAppBrowserActivity` and `AuthCustomTabActivity`. + +2. `ExternalAppBrowserActivity` overrides the following `AppCompatActivity` functions: +* `onResume()` +* `onDestroy()` +* `onProvideAssistContent()` + +which all depend on the `super` (`HomeActivity`) definitions prior to adding the `ExternalAppBrowserActivity` behaviour. `onResume()` is further propagated to `AuthCustomTabActivity` which depends on the `ExternalAppBrowserActivity` implementation. + +3. `ExternalAppBrowserActivity` is required to override the following `HomeActivity` defined functions: +* `getNavDirections()` +* `getBreadcrumbMessage()` +* `getIntentSource()` +* `getIntentSessionId()` + +It's notable that the `HomeActivity` `getIntentSessionId()` implementation always returns `null` and is required to take a redundant `intent` parameter. +The `HomeActivity` `getNavDirections()` implementation is required to take a redundant `customTabSessionId` parameter. + +4. The `ExternalAppBrowserActivity` currently uses the `HomeActivity` `onCreate()` implementation. This forces the Custom Tabs features to to perform unnecessary checks such as: +* `maybeShowSplashScreen()` +* `shouldShowOnboarding()` +* perform checks for Homepage Contile feature activity +* perform checks for Pocket feature activity + +This list is not exhaustive but gives an insight into the redundant work being forced upon the `ExternalAppBrowserActivity`. This applies to many of the other `HomeActivity` lifecycle related functions. + +## Proposal +Reduce the coupling of the `Activity`s. Establish `HomeActivity` as the *main* `Activity` and define clear responsibilities. Below is an list of suggested refactorings to initiate the decoupling. + +1. Remove the no-ops from `ExternalAppBrowserActivity`. [Example PR.](https://github.com/mozilla-mobile/firefox-android/pull/4851) + +2. Delegate the `Activity`s 'get nav directions' functionality. This removes the definition from `HomeActivity` and the requirement to override it in `ExternalAppBrowserActivity`. [Example PR.](https://github.com/mozilla-mobile/firefox-android/pull/4854) + +3. Delegate the `HomeActivity` 'open to browser' functionality. This removes another responsibility from `HomeActivity`. [Example PR.](https://github.com/mozilla-mobile/firefox-android/pull/4857) + +4. Extract the `getBreadcrumbMessage`, `getIntentSource` and `getIntentSessionId` functions from `HomeActivity` using an `Activity` delegate. This removes the definition from `HomeActivity` and the requirement to override it in `ExternalAppBrowserActivity`. [Example PR.](https://github.com/mozilla-mobile/firefox-android/pull/4872) + +5. Update `HomeActivity` & `ExternalAppBrowserActivity` with the necessary visibility modifiers to enforce constraints. + +6. Extract the `handleRequestDesktopMode` function. This removes another responsibility from `HomeActivity`. + +7. Rename `HomeActivity` to be explicit that this is the *main* `Activity`. For info, the Focus application uses a `MainActivity`, the Reference Browser application uses a `BrowserActivity`. + +The next steps are less clearly defined and require more investigation on the completion of the above tasks. + +* Explore alternatives to the current Inheritance structure. E.g. Kotlin Delegation methods, an alternative 'base' `Activity` model or consolidating `ExternalAppBrowserActivity` & `HomeActivity` into a single `Activity`. +* Prevent unnecessary checks being carried out by `ExternalAppBrowserActivity` in `HomeActivity` lifecycle related functions. + +## Drawbacks +* Current reported Custom Tabs Bugzilla bugs may be affected by the changes mentioned here. +* No immediate tangible user facing improvements. + +## Rationale and alternatives +### Rationale +* It is well established best practice to ['Prefer composition over inheritence'](https://en.wikipedia.org/wiki/Composition_over_inheritance). An implementation of this principal is also exemplified in AC UI classes, see Reference Browser UI classes e.g. `BrowserActivity` & `BaseBrowserFragment`. +* [Separation of concerns](https://developer.android.com/topic/architecture#separation-of-concerns). +> The most important principle to follow is separation of concerns. It's a common mistake to write all your code in an Activity or a Fragment. These UI-based classes should only contain logic that handles UI and operating system interactions. By keeping these classes as lean as possible, you can avoid many problems related to the component lifecycle, and improve the testability of these classes. +* Reducing the coupling of the `Activity`s and clear delineation of responsibilities should vastly improve their maintainability & testability. + +### Alternatives +* Leave the inheritance as-is between `HomeActivity`, `ExternalAppBrowserActivity` & `AuthCustomTabActivity` and only focus on refactoring work. diff --git a/mobile/android/android-components/docs/rfcs/0012-introduce-ui-store.md b/mobile/android/android-components/docs/rfcs/0012-introduce-ui-store.md new file mode 100644 index 0000000000..763801ddfe --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0012-introduce-ui-store.md @@ -0,0 +1,176 @@ +--- +layout: page +title: Introduce a Store for UI components +permalink: /rfc/0012-introduce-ui-store +--- + +* Start date: 2024-01-29 +* RFC PR: [#5353](https://github.com/mozilla-mobile/firefox-android/pull/5353) + +## Summary + +In most applications of the `Store`, it is preferable to have reducers perform work on the main thread. Having actions reduced immediately at the point of dispatch, simplifies the reasoning a developer would need to go through for most UI-based work that happens on the main thread. + +## Motivation + +Android embedders use the main thread for UI, user-facing, or gesture handling work. For example, notifying UI components when IO from storage layers have completed, an engine's task that can happen on a separate thread, or global-level state updates for different components to observe. + +When components dispatch actions, they are performed on an independant single thread dispatcher in the `Store` to avoid overloading the main thread with heavy work that might be performed during the `reduce` or in a `Middleware`. In practice, these actions have been short and fast so they do not cause overhead (most of these actions have been [data class copying][0]). In addition, side-effects done in a `Middleware` which can be slow, like I/O, are put onto separate Dispatchers. The performance optimization to switch to a `Store` thread, requires that components which are always run on the main thread, to ensure synchronisation is now kept between the main thread and the store thread for observers of the `State`. + +There are some advantages to this change: + +* Simplicity for `Store`s that are meant for UI facing work. +* Unit testing can now occur on the test framework's thread. +* Fewer resources needed for context shifting between threads[^1]. + +For an example of thread simplicity, an `Engine` typically has its own 'engine thread' to perform async work and post/request results to the main thread (these APIs are identified with the `@UiThread` annotation). Once we get the callback for those results, we then need to dispatch an action to the store that will then happen on a `Store` thread. Feature components then observe for state changes and then make UI changes on the main thread. A simplified form of this thread context switching can be seen in the example below: + +```kotlin +// engine thread +engineView.requestApiResult { result -> + // received on the main thread. + store.dispatch(UpdateResultAction(result)) +} + +// store thread +fun reduce(state: State, action: Action) { + is UpdateResultAction -> { + // do things here. + } +} + +// store thread +Middleware { + override fun invoke( + context: MiddlewareContext<State, Action>, + next: (Action) -> Unit, + action: Action, + ) { + // perform side-effects that also happen on the store thread. + } +} + +// main thread +store.flowScoped { flow -> + flow.collect { + // perform work on the main thread. + } +} +``` + +With the changes in this RFC, this switching of threads can be reduced (notable comments marked with 📝): + +```kotlin +// engine thread +engineView.requestApiResult { result -> + // received on the main thread. + store.dispatch(UpdateResultAction(result)) +} + +// 📝 main thread - now on the same thread, processed immediately. +fun reduce(state: State, action: Action) { + is UpdateResultAction -> { + // do things here. + } +} + +// 📝 main thread - now on the same thread, processed immediately. +Middleware { + override fun invoke( + context: MiddlewareContext<State, Action>, + next: (Action) -> Unit, + action: Action, + ) { + // 📝 perform side-effects that now happen on the main thread. + } +} + +// main thread +store.flowScoped { flow -> + flow.collect { + // perform work on the main thread. + } + } +} +``` + +Additionally, from [performance investigations already done][2], we know that Fenix creates over a hundred threads within a few seconds of startup. Reducing the number of threads for Stores that do not have a strong requirement to run on a separate thread will lower the applications memory footprint. + +## Guide-level explanation + +Extending the existing `Store` class to use the `Dispatchers.Main.immediate` will ensure that UI stores will stay on the same UI thread and have that work done immediately. Using a distinct class named `UiStore` also makes it clear to the developer that this is work that will be done on the UI thread and its implications will be made a bit more clear when it's used. + +```kotlin +@MainThread +open class UiStore<S : State, A : Action>( + initialState: S, + reducer: Reducer<S, A>, + middleware: List<Middleware<S, A>> = emptyList(), +) : Store<S, A>( + initialState, + reducer, + middleware, + UiStoreDispatcher(), +) + +open class Store<S : State, A : Action> internal constructor( + initialState: S, + reducer: Reducer<S, A>, + middleware: List<Middleware<S, A>>, + dispatcher: StoreDispatcher, +) { + constructor( + initialState: S, + reducer: Reducer<S, A>, + middleware: List<Middleware<S, A>> = emptyList(), + threadNamePrefix: String? = null, + ) : this( + initialState = initialState, + reducer = reducer, + middleware = middleware, + dispatcher = DefaultStoreDispatcher(threadNamePrefix), + ) +} + +interface StoreDispatcher { + val dispatcher: CoroutineDispatcher + val scope: CoroutineScope + val coroutineContext: CoroutineContext + + // Each Store has it's own `assertOnThread` because in the Thread owner is different in both context. + fun assertOnThread() +} +``` + +Applications can use this similar to any other store then. An "AppStore" example below can switch : + +```kotlin +// changing the one line below from `UiStore` to `Store` gives the developer the ability to switch existing Stores between the different Store types. +class AppStore( + initialState: AppState = AppState(), +) : UiStore<AppState, AppAction>( + initialState = initialState, + reducer = AppStoreReducer::reduce, +) +``` + +## Drawbacks + +* Mistakenly doing work on the main thread - we could end up performing large amounts of work on the main thread unintentionally if we are not careful. This could be because of a large number of small tasks, a single large task, a blocking task, or a combination. As the developer is choosing to use a `UiStore`, they will be expected to ensure that heavy work they do, as is with mobile UI development done today, is not done on the main thread. + +## Rationale and alternatives + +Not introducing this new Store type would not change current development where the developer needs to ensure understanding that dispatched actions will be processed at a later time. + +## Future work + +We have opportunities to iterate from here and consider if/how we want to pass a CoroutineScope in. This can be part of future RFC proposals however. + +## Unresolved questions + +* While performance gains are not an explicit intent, there is a theoretical advantage, but not one we will pursue as part of this RFC. How much would we save, if any? +* Some additional changes need to be done to allow the `Store` to override the default `StoreThreadFactory` that will allow assertions against a thread (`MainThread`) not created by the `StoreThreadFactory` itself. This should be possible, but will this add to additional complexity? + +[0]: https://kotlinlang.org/docs/data-classes.html#copying +[^1]: https://github.com/mozilla-mobile/android-components/issues/9424 +[2]: https://github.com/mozilla-mobile/android-components/issues/9424#issue-787013588 diff --git a/mobile/android/android-components/docs/rfcs/0013-rfc-process-updates.md b/mobile/android/android-components/docs/rfcs/0013-rfc-process-updates.md new file mode 100644 index 0000000000..e7f445e400 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/0013-rfc-process-updates.md @@ -0,0 +1,48 @@ +--- +layout: page +title: RFC process updates +permalink: /docs/rfcs/0013-rfc-process-updates +--- + +* RFC PR: https://github.com/mozilla-mobile/firefox-android/pull/5681 +* Start date: 2024-02-20 +* End date: 2024-03-22 +* Stakeholders: @jonalmeida, @boek + +## Summary + +Clarify requirements for RFCs by introducing a README and a template. + +## Motivation + +- The scale of the Firefox Android team has grown substantially since the RFC process was first introduced. +- More external teams are using and contributing to Android Components. +- Implicit deadlines have encouraged a lack of engagement with RFCs, slowing follow-up work. +- There has been a low level of engagement with RFCs because of a lack of clarity around the process and when they are appropriate. + +## Guide-level explanation + +This proposal suggests improving the RFC process by introducing the following: + +- An RFC template, to provide a clear starting point for proposals. +- A guide for when RFCs are appropriate and when they are not needed in the form of a README. +- A requirement for explicit stakeholders for RFCs. +- An initial deadline recorded in each proposal. +- A renaming of the the "Prior Art" section to "Resources and Docs" and wording to indicate that proposals can also include additional documentation. + +## Rationale and alternatives + +The RFC process has been successful in some cases, but has not been consistently followed. This proposal aims to make the process more accessible and clear, by introducing high-level concepts in the README and a clear template. These documents can be more easily treated as living documents, and updated with any future changes to the process. + +### Amend RFC 0001 with additional template information +- This was not included in the proposal in order to keep past RFCs consistent and free of modern context, and so that past RFCs do not become treated as living documents. + +## Resources and Docs + +[README](./README.md), a new artifact that includes guidelines and advice on when RFCs are appropriate and how to contribute them. +[0000 RFC Template](./0000-template.md), a new artifact that provides a clear starting point for proposals. +[Follow-up bug for updating CODEOWNERS](https://bugzilla.mozilla.org/show_bug.cgi?id=1881373), so that stakeholders can be found more easily. + +## Unresolved questions +- Will this result in an easier process for contributors to follow? +- Will this result in more engagement with RFCs? diff --git a/mobile/android/android-components/docs/rfcs/README.md b/mobile/android/android-components/docs/rfcs/README.md new file mode 100644 index 0000000000..803fcbc912 --- /dev/null +++ b/mobile/android/android-components/docs/rfcs/README.md @@ -0,0 +1,47 @@ +# The RFC Process + +An RFC (**R**equest **F**or **C**omments) is a process through which contributors can solicit buy-in +for proposed changes to the codebase and repository at-large. It was introduced in the first RFC, +[0001-rfc-process](./0001-rfc-process.md), which includes additional details about the reasoning +for including the process. + +This is an overview of what kind of changes benefit from or require the consensus-building that the +RFC process provides, as well as a brief guide on how to draft them. + +## What kinds of changes require an RFC? + +1. Substantial changes to public APIs in Android Components, like the changes found in [0003 Concept Base Component](./0003-concept-base-component.md) and [0008 Tab Groups](docs/rfcs/0008-tab-groups.md). +2. Changes to process that affect other teams, like the changes found in [0001 RFC Process](./0001-rfc-process.md), [0013 Add stakeholders to RFCs](./0013-rfc-process-updates.md), and [0007 Synchronized Releases](./0007-synchronized-releases.md). +3. Proposals for changes to areas of the codebase that are owned by CODEOWNERS outside the author's team. + +## What kind of other changes can an RFC be useful for? + +1. Announcing a rough plan for changes to a public API you own in order to solicit feedback. +2. Soliciting feedback for architectural changes that affect the entire codebase, like [0009 Remove Interactors and Controllers](./0009-remove-interactors-and-controllers.md). + +## How to contribute an RFC + +There is a [template](./0000-template.md) that can be a useful guide for structure. + +While drafting a proposal, consider the scope of your changes. Generally, the level of detail should match the level of +impact the changes will have on downstream consumers of APIs, other teams, or users. + +Once a proposal is drafted: + +1. Choose a deadline for general feedback. +2. Select and communicate with stakeholders. +3. Share the RFC more broadly through Slack and mailing lists for general feedback (like firefox-android-team@ and firefox-mobile@). +4. Build consensus and integrate feedback. + +### Stakeholders + +Stakeholders are required for each RFC. They will have the final say in acceptance and rejection. +Include at least 2 people as stakeholders: a CODEOWNER of the affected area and another (preferably a Firefox for Android team member). + +Stakeholders should be active in the RFC process - they should ask to be replaced if they do not have bandwidth to get the RFC finished in a short time span. This is to help the RFC process remain nimble and lightweight. + +### Deadlines + +A deadline for feedback should be included in each RFC. This should usually be at least a week, so plan accordingly. +For more substantial changes, it can be useful to plan for 2 or 3 weeks so that there is more opportunity for feedback from people that are not stakeholders. +If a proposal is approved by all stakeholders earlier than the deadline, the proposal can be merged immediately. |