Bug 1997373 - Store original pref values for Fenix Gecko integration #7122
Conversation
ae5eaf6 to
181a21d
Compare
|
Hi @jeddai and @freshstrangemusic , Please take a look at this when you have a chance! I don't have the right repo access to request in the Reviewers panel. |
ohall-m
left a comment
There was a problem hiding this comment.
Thanks for all the thoughtful comments! Really appreciate all of the rust explanations especially.
I have a couple of ToDos I marked in the comments that I'm still working through. I'm submitting the comments on what has changed to help keep the changes organized for now.
|
First draft comments should be addressed now, thanks! |
| #[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)] | ||
| #[cfg(feature = "stateful")] | ||
| pub struct PreviousGeckoPrefState { | ||
| pub original_values: Vec<OriginalGeckoPref>, | ||
| pub feature_id: String, | ||
| pub variable: String, | ||
| } | ||
|
|
||
| #[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq, uniffi::Enum)] | ||
| #[cfg(feature = "stateful")] | ||
| pub enum PreviousState { |
There was a problem hiding this comment.
Upon further reflection, im not sure this warrants being an enum. I think we should remove the variants and just have the PreivousGeckoPrefState in the enrollment.
There was a problem hiding this comment.
Works for me! That would simplify a lot of checking.
(Still have this change ToDo.)
| .and_modify(|st| st.original_values.push(original_gecko_pref.clone())) | ||
| .or_insert_with(|| PreviousGeckoPrefState { |
There was a problem hiding this comment.
Why would there be more than one "original gecko pref state" per experiment slug?
There was a problem hiding this comment.
I pulled the conception of multiple gecko pref states from this construct pub type MapOfFeatureIdToPropertyNameToGeckoPrefState = HashMap<String, HashMap<String, GeckoPrefState>>;, but I'm not confident if I'm fully understanding the relationships here.
LMK, if you think multiple gecko pref states are possible! If not, I think just a singular gecko pref makes sense. It was mostly that section of code that made me wonder if it was possible. (I also don't have a strong picture of the hierarchy of experiment slug > feature > variable.)
| #[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)] | ||
| #[cfg(feature = "stateful")] | ||
| pub struct PreviousGeckoPrefState { | ||
| pub original_values: Vec<OriginalGeckoPref>, |
There was a problem hiding this comment.
Why is this a Vec? Shouldn't it just be an OriginalGeckoPref ?
This patch begins storing a `PreviousState` on `ExperimentEnrollment` when it is of type `EnrollmentStatus::Enrolled`. It also introduces `PreviousState::GeckoPref` to hold Gecko original preference values for Gecko pref based experiments. The public APIs it opens are `registerPreviousGeckoPrefStates` and `getPreviousState`.
The pull request has been modified, dismissing previous reviews.
This patch begins storing a
PreviousStateonExperimentEnrollmentwhen it is of typeEnrollmentStatus::Enrolled. It also introducesPreviousState::GeckoPrefto hold Gecko original preference values for Gecko pref based experiments.The public APIs it opens are
registerPreviousGeckoPrefStatesandgetPreviousState.The goal of this patch is to appropriately store the original Gecko preference value to help when exiting an experiment.
Current idea is that it will be set similarly as
unenrollForGeckoPrefin this patch D257390, but for the positive case. Bug 2003370 will use this to properly consume these values when appropriate.Pull Request checklist
[ci full]to the PR title.