VPN-5854 - Implement infrastructure for creation of experimental features #8821

brizental · 2023-12-15T13:22:31Z

Ok, this PR is on top of both #8819 and #8820. The only new commit here is f16f766.

The specification for the experimental feature can be found at: https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/docs/rfcs/0006-cirrus-experiments.md#3-apply-features.

Note I was not able to implement the APIs as specified in the RFC. This is how they will look like:

MZSettings.onboardingCtaStyleExperiment.get("color");

settingsHolder.onboardingCtaStyleExperiment.get("color");

Yeah, still learning about managing lifetimes without having my hand held by the compiler. (I miss Rust). Turns out I would have to cache the value of the setting in the SettigsGroup object for the previous design to work... I think that would not make much sense so I decided to diverge from the initial design and just expose the getter function itself.

Intentionally though I will not move other feature files in it just yet, I can do that in a separate PR.

vinocher · 2023-12-20T04:41:25Z

src/settings/settingfactory.h

+#include "setting.h"
+#include "settingsmanager.h"
+
+class SettingFactory {


Should SettingsFactory be folded into SettingManager? Is a separate factory providing value?

Done and added to the PRE-WORK PR.

vinocher · 2023-12-20T04:42:38Z

src/settings/settingfactory.cpp

+Setting* SettingFactory::createOrGetSetting(
+    const QString& key, std::function<QVariant()> defaultValue,
+    bool removeWhenReset, bool sensitiveSetting) {
+  auto s = SettingsManager::getSetting(key);


Nit: Consider using a more descriptive variable name for 's'

Done and added to the PRE-WORK PR.

vinocher · 2023-12-20T04:43:09Z

src/settings/settinggroup.h

+ * @brief Represents a group of dynamic settings. Dynamic setttings can be
+ * defined at runtime.
+ *
+ * This abstraction serves as an aggregator of underlying storage settings.


Nice header comments!

Thanks Vinoo, means a lot coming from the documentation king himself!

vinocher · 2023-12-20T04:49:29Z

src/settings/settinggroup.h

+   *
+   * @return QVariant
+   */
+  Q_INVOKABLE QVariant get(const QString& key) const;


Is getValue a better name? get(key) seems generic.

Hum, I think this falls down to personal preference. If you feel strongly about it, I'll change it. Personally, I prefer it like this. If I do change it, I'd change this in all places, right? i.e. get becomes getValue, set becomes setValue, remove becomes removeValue and so on. Let me know!

vinocher · 2023-12-20T04:58:19Z

src/settings/settinggroup.cpp

+
+void SettingGroup::addSetting(const QString& key) {
+  auto settingKey = getSettingKey(key);
+  auto setting = SettingFactory::createOrGetSetting(


Is this setting being leaked? Does it need a parent QObject, so it can be deleted?

The parent is the SettingsManager instance, whose parent it the QApplication.

vinocher · 2023-12-20T05:01:24Z

src/settings/settinggroup.h

+   * actually be removed when `reset` is called.
+   * @param sensitiveSetting Whether or not settings in this group are
+   * sensitive settings i.e. settings that must not be logged in plain text.
+   * @param acceptedKeys When this list has at least one member, only the listed


Why is the enforcement using acceptedKeys needed? If not needed, should it and mayRecord() be removed?

This is pre-work for experiments related stuff, which will require an allow list of keys.

vinocher · 2023-12-20T05:11:59Z

src/settings/settinggroup.cpp

+  sb->m_settings.beginGroup(m_groupKey);
+  QStringList keys = sb->m_settings.allKeys();
+  sb->m_settings.endGroup();


For better encapsulation, consider replacing this block with SettingManager::instance()->getKeysInGroup(m_groupKey), which returns the list of keys of Settings in the group.

I have addressed this and added it to the PRE-WORK PR. I did not do exactly what you propose here. I'd not like to add public methods to change the underlying settings to the SettingsManager, that is an invitation for mess ups. Instead, I created a connector for the QSettings that is passed to the Setting and SettingGroup for them to be able to access the QSettings. Let me know what you think about the approach.

vinocher · 2023-12-20T05:14:48Z

src/settings/settinggroup.cpp

+  sb->m_settings.beginGroup(m_groupKey);
+  QStringList keys = sb->m_settings.allKeys();
+  sb->m_settings.remove("");
+  sb->m_settings.endGroup();


For better encapsulation, consider replacing this with:
keys = SettingManager::instance()->getKeysInGroup(m_groupKey);
SettingManager::instance()->removeSettingsInGroup(m_groupKey)

vinocher · 2023-12-20T05:17:11Z

src/settings/settinggroup.cpp

+  foreach (const QString& key, keys) {
+    auto setting = SettingsManager::getSetting(key);
+    Q_ASSERT(setting);
+    setting->changed();
+  }


Should the settings in the setting group also be removed from SettingManager.m_registeredSettings?

If so, won't SettingsManager::getSetting(key) fail because the setting has been removed? Appears that the change() signal should be emitted before the settings are removed.

Yes, great catch. I'll address this and add it to the PRE_WORK PR.

brizental · 2023-12-20T19:09:21Z

Ok, in hindsight I think it was confusing to open this PR before closing the PRE-WORK PR. All the current comments are related to PRE-WORK code, so I am closing this and will open a new PR once the PRE-WORK PR is complete!

brizental added 4 commits December 15, 2023 13:58

Create test suite for SettingsHolder

c56762c

Remove userSetting, it is unused

b9aa105

Create a new set of abstractions for managing application storage

aa9a4ec

Use the new storage abstractions in existing code

e07a047

brizental mentioned this pull request Dec 15, 2023

[PRE-WORK] VPN-5854 - Refactor storage management #8820

Merged

brizental requested review from Gela, strseb and vinocher December 15, 2023 13:24

brizental added 2 commits December 18, 2023 12:08

Address review comments

9cf9521

brizental force-pushed the 5854-experimental-feature branch from f16f766 to 0f1c097 Compare December 18, 2023 11:24

brizental added 2 commits December 18, 2023 13:52

Fix lints

7671674

Implement infrastructure for experimental features

daa295d

brizental force-pushed the 5854-experimental-feature branch from 0f1c097 to daa295d Compare December 18, 2023 12:53

brizental changed the base branch from main to pre-experiments-work December 18, 2023 16:46

Create a feature/ folder for the feature related files

21615c7

Intentionally though I will not move other feature files in it just yet, I can do that in a separate PR.

brizental force-pushed the 5854-experimental-feature branch from 6bd5aa5 to 21615c7 Compare December 18, 2023 16:51

brizental force-pushed the pre-experiments-work branch from 7671674 to 6455399 Compare December 18, 2023 17:06

vinocher reviewed Dec 20, 2023

View reviewed changes

brizental closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-5854 - Implement infrastructure for creation of experimental features #8821

VPN-5854 - Implement infrastructure for creation of experimental features #8821

brizental commented Dec 15, 2023 •

edited

Loading

vinocher Dec 20, 2023 •

edited

Loading

brizental Dec 20, 2023

vinocher Dec 20, 2023

brizental Dec 20, 2023

vinocher Dec 20, 2023

brizental Dec 20, 2023

vinocher Dec 20, 2023

brizental Dec 20, 2023

vinocher Dec 20, 2023

brizental Dec 20, 2023

vinocher Dec 20, 2023

brizental Dec 20, 2023

vinocher Dec 20, 2023 •

edited

Loading

brizental Dec 20, 2023

vinocher Dec 20, 2023 •

edited

Loading

vinocher Dec 20, 2023

brizental Dec 20, 2023

brizental commented Dec 20, 2023

VPN-5854 - Implement infrastructure for creation of experimental features #8821

VPN-5854 - Implement infrastructure for creation of experimental features #8821

Conversation

brizental commented Dec 15, 2023 • edited Loading

vinocher Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinocher Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinocher Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental commented Dec 20, 2023

brizental commented Dec 15, 2023 •

edited

Loading

vinocher Dec 20, 2023 •

edited

Loading

vinocher Dec 20, 2023 •

edited

Loading

vinocher Dec 20, 2023 •

edited

Loading