Skip to content

Conversation

@aitorvs
Copy link
Collaborator

@aitorvs aitorvs commented Mar 24, 2021

Task/Issue URL: https://app.asana.com/0/414730916066338/1200093914838515/f
Tech Design URL:
CC:

Description:
#1123 introduced a bug that was detected by our pixels.

  • That pull request delegated the creation of the view models (VMs) to its own factory
  • The VM factories are singleton -- as they should be
  • The VM factories create the VMs and inject their dependencies
  • Because the VM factories are singleton that effectively makes all VM dependencies singleton. Regardless of whether they are actually declare as such or not
  • This is not desired, VM factories thouls be singleton, but it should respect the di semantics of its deps

This PR makes all the factories to provide providers of the dependencies, rather than the actual instance. So that they respect how the instances declare themselves when injected, ie. singleton or multiple instances.

In particular, the effects can be seen with the fw_ pixels. They are fired multiple times (one per fragment) when they should fire only once.

Steps to test this PR:

  1. App compiles

  2. install/upgrade the app from this branch

  3. use the SERP to perform searches

  4. form the SERP, open several tabs

  5. add/remove bookmarks

  6. add/remove fireproof sites

  7. navigate to settings screen

  8. use fire button

  9. clear app data and go through onboarding UX

  10. for all above, verify app works as expected

  11. install/upgrade from this branch

  12. open several tabs and ensure they are active

  13. ensure Ask When Signing In is enabled inside settings -> fireproof sites

  14. open a new tab and log into facebook.com (or any other login you may want)

  15. verify the dialog to fireproof the site appears

  16. navigate to any other of the active tabs

  17. verify no dialog appears

  18. verify m_fw_xx are only shown once


Internal references:

Software Engineering Expectations
Technical Design Template

Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aitorvs

Just a minor comment:
I think providers are missing in WelcomePageViewModelFactory, could it be?

I've just tried to test a bit the app and everything seems to work as expected, GJ.
I've also put an eye on the logcat to ensure no duplicated pixels were sent.
I've checked that we are not sharing instances between fragments when they are not declared as singleton (NextPageLoginDetection), and we have a single instance when they are declared as singleton (SiteFactory). All good

@cmonfortep
Copy link
Contributor

cmonfortep commented Mar 24, 2021

BTW, please, remember to create a task in Asana for this task and add it to the release board 👍 (and add it to the description)

@aitorvs
Copy link
Collaborator Author

aitorvs commented Mar 24, 2021

I think providers are missing in WelcomePageViewModelFactory, could it be?

That factory is not part of the ViewModelFactoryPlugins. WelcomePage fragment uses that factory instead of the generic app ViewModelFactory.
We should move it there, will do in a separate task/PR

@aitorvs
Copy link
Collaborator Author

aitorvs commented Mar 24, 2021

BTW, please, remember to create a task in Asana for this task and add it to the release board 👍 (and add it to the description)

Will do

@aitorvs aitorvs merged commit 9d17387 into develop Mar 24, 2021
@aitorvs aitorvs deleted the fix/aitor/vmf branch March 24, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants