Split Omnibar: Data type & storage update

[0nko]

Task/Issue URL: https://app.asana.com/1/137249556945/task/1211565479656353?focus=true

Description

This PR updates the storage layer and replaces the OmnibarPosition -> OmnibarType.

New OmnibarType enum has the following values:

• SINGLE_TOP
• SINGLE_BOTTOM
• SPLIT_TOP

There's also a new splitOmnibar feature flag, although it's unused at the moment.

Steps to test this PR

Change position

• Go to Settings -> Appearance
• Verify the position option shows the correct value
• Tap on the omnibar position option
• Change the position
• Go back to the browser and verify the omnibar position changed

Omnibar behavior

• Test that the omnibar hiding works on scroll both when position at the top
• Test thatthe correct browser menu shown when position at the top (navigation buttons are at the top of menu)
• Test that the omnibar hiding works on scroll both when position at the bottom
• Test that the correct browser menu shown when position at the bottom (navigation buttons are at the bottom of menu)

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1211565479656353

[0nko]

• Split Omnibar: Remove old feature flag #7045
• Split Omnibar: Address bar integration #7042
• Split Omnibar: Bottom navigation #7032
• Split Omnibar: Settings screen #7002
• Split Omnibar: Data type & storage update #6991 👈 (View in Graphite)
• Split Omnibar: Omnibar layout refactoring #6983
• develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

[LukasPaczos]

settingsDataStore can be mutated at any point in time, from a lot of places, so having this safety check in the BrowserActivity only looks fragile.

Maybe consider adding an OmnibarTypeResolver or something along these lines to inject into all places that initialize the omnibar, and do the flag checks there?

[0nko]

Thanks @LukasPaczos, that's a good point. However, there are a lot of places where this resolver would litter the code. Considering that this is just a temporary measure while the flags exist, I was thinking maybe we could avoid that and move this type reset somewhere higher up.

What do you think of adding a PrivacyConfigCallbackPlugin and checking/resetting in onPrivacyConfigDownloaded()?

[0nko]

Maybe using MainProcessLifecycleObserver would be better, because onStart() would be run every time the app starts, as opposed to just when the config is downloaded. This would take care of internal builds when the feature is toggled off in the dev settings.

[LukasPaczos]

The downside of this is that if:

1. We release the feature and a number of users enable it.
2. We have to temporarily disable it remotely.
3. We re-enable the feature.
4. All of the the users that previously opted into using the split omnibar would have to go and enable it again.

There's no friction-less way to manage a situation like that, but I think the preference should persist.

What if we added the flag check before returning the value from the SettingsSharedPreferences?

override var omnibarType: OmnibarType
    get() {
        val prefValue = OmnibarType.fromString(
            preferences.getString(KEY_OMNIBAR_TYPE, OmnibarType.SINGLE_TOP.typeName) ?: OmnibarType.SINGLE_TOP.typeName,
        )
        return if (prefValue == OmnibarType.SPLIT && browserFeatures.splitOmnibar().isEnabled()) {
            OmnibarType.SPLIT
        } else {
            OmnibarType.SINGLE_TOP
        }
    }
    set(value) = preferences.edit { putString(KEY_OMNIBAR_TYPE, value.typeName) }

There could be a concern about synchronous feature flag check. That said, pref access can already go to disk so all responsible callers should already move this off of the main thread 😁 We can also look into caching the flag value.

[0nko]

I already thought of your suggestion but unfortunately, it creates a dependency cycle. I've added a flag to remeber if the split omnibar was used when a feature is disabled. It's then used to restore the choice when it's re-enabled.

[0nko]

@LukasPaczos You can test this fallback behavior in a subsequent PR where you can see the selection in the settings.

[LukasPaczos]

Continuing in #6991 (comment).

[LukasPaczos]

This approach feels a bit backwards to me. As discussed in this comment, whether the split omnibar is used isn’t determined solely by the user preference - it also depends on resolving a feature flag. That means it should be checked every time.

In this case, we effectively need a small "repository" layer in front of the settings data store to handle that logic cleanly. I understand the hesitation to add it since the flag might be short-lived, but I suspect it could stay around for a while during rollout.

My recommendation would still be to introduce this resolver/repository class to keep the code clean, but the current approach also works right now, so I won’t block the PR.

If you decide to move forward with the current version, please just make sure the isEnabled feature flag checks don’t run on the main thread.

[0nko]

@LukasPaczos I understand the concern. However, the flags are used during omnibar initialization in the BrowserTabFragment, so a flag update would not take effect until a new tab is created. In fact, using the live value might create an inconsistent state when one tab might use a split omnibar and the other one would not. So using a "resolver" to get the most up-to-date omnibar type (based on the latest flag value) might create even more issues this omnibar type is used all over the place.

I think the easiest solution is to simply cache the latest known flag values at the start (and change the omnibar type, if needed) and then use them throughout the lifetime of the app. If there's a new config downloaded while the app's running, it will take effect the next time the app restarts.

I added OmnibarFeatureRepoitory, which loads the flags (on the background thread) when the app starts.

[LukasPaczos]

However, the flags are used during omnibar initialization in the BrowserTabFragment, so a flag update would not take effect until a new tab is created. In fact, using the live value might create an inconsistent state when one tab might use a split omnibar and the other one would not

That same issue can occur with the proposed OmnibarFeatureRepository. It updates the preference state in onStart, so if the user resumes the app after a config change, new tabs will fallback to top omnibar while existing tabs will still use the split version.

Either way, each fragment already checks for changes to the omnibar position/type in BrowserTabFragment#onResume, so they should recover correctly in both cases.

Let's merge and iterate as needed.