Remove api dependencies from privacy-config-api and vpn-api modules

[marcosholgado]

Task/Issue URL: https://app.asana.com/0/1125189844152671/1202666690958570

Description

This PR removes the dependency from feature-toggles-api in the privacy-config-api and vpn-api modules. It removes FeatureName and swaps it to a String.

Steps to test this PR

Remote config

• All tests pass
• Try a few remote features and make sure they work as expected, some examples below.
• Install and launch from this branch
• Filter by user agent in the logcat
• Go to thingiverse.com
• User agent should not include the word DuckDuckGo
• Go to welt.de
• Privacy grade should be solid black
• Go to the privacy dashboard (click on the privacy grade)
• There should be a message saying protections are temporarily disabled due to breakage
• Check the toggles, privacy config and vpn DB to make sure the data inserted matches the date from https://staticcdn.duckduckgo.com/trackerblocking/config/v1/android-config.json

[cmonfortep]

I have some doubts about this change:
I liked using a specific type instead of a primitive here. So I wonder if we should see this from a different perspective instead of finding a way to break the api dependency.

How I see it:

• FeatureName restricts the possible value we pass into that function
• FeatureName clearly indicates that we have a set of features that can be enabled/disabled
• We had some method interfaces that used FeatureName as arguments.
• Then I see, that different modules can enable/disable features. I expect those modules to contribute to the app values that implement FeatureName
• Maybe this is a different type of api. Saying this because I see a difference between: modules that own some logic and we are only consumers, vs modules that we can contribute to.
• In this case I think we are contributors in a way.

I see this very similar to how we consume statistics module for Pixels. We have a bunch of modules that are extending Pixel.PixelName interface, and I think that's correct.

Happy to know more about the approach here.

[aitorvs]

The rational (trade-off) behind is is the following (assuming we don't want to break the api not depending in api rule):

• The reason to have a the FeatureName type is to have type-safe code as much as possible
• The PrivacyFeaturePlugin plugins are internally not type-safe anyway because they have to do FeatureName.value. So in that sense, a String instead of a FeatureName doesn't make a difference
• So now it comes to the caller of the isFeatureEnabled
• In this case, the idea is that even though that isFeatureEnabled is now taking a String as a parameter, every feature-api module will expose an enum listing all its supported features
• Callers using that enum are still as type-safe as before
• The only nuance is that, having a FeatureName type in the isFeatureEnable signature makes clear(er) in the that the caller should look for a FeatureName (extended) type
• And that's the only thing we're giving up with this trade-off
• Which I think it's easily solvable with just Javadocs

[marcosholgado]

What @aitorvs said plus... you could also do FeatureName { "something" } before so it wasn't safe at all :). In any of the cases the app will crash if you use a featureName that doesn't exist but that's explained in the Javadocs.

[cmonfortep]

I agree "it's solvable", and that FeatureName is replaceable by String with the current logic. The other arguments about type-safe are implementation details and I wouldn't focus too much there.

My point is that we are going to have similar scenarios in the future where we contribute to a module rather than just consume the module. As I said, Statistics module is a similar example.

So I'm not advocating we should allow depending on other modules api, I'm advocating we should have another type of module (not sure if we consider Statistics as an api module or not). And feature-toggles should be that type of module where others can depend on.

[marcosholgado]

In my head the only way you contribute to a module is usually done in the impl module. In your statistics example, the macOS waitlist needs that module, but the dependency happens in the macos-impl module rather than the macos-api.

[marcosholgado]

PrivacyFeaturePlugin is defined in the privacy-config-api module so it cannot have FeatureName because that was part of the feature-toggles-api module.

[cmonfortep]

Yes, I'm sorry I see my comment was confusing. I wanted to ask if we could have moved to PrivacyFeatureName instead of String when replacing FeatureName

[marcosholgado]

The problem with using PrivacyFeatureName in the PrivacyFeaturePlugin would be the same. We have modules (i.e. vpn and soon ad_click) that contribute to the plugin but by enforcing to use PrivacyFeatureName we would end up with the same api dependency if we wanted to expose their specific feature names (i.e. AppTpFeatureName)

[cmonfortep]

I was assuming that we will only "contribute" from those modules (vpn and ad_click), and never expose anything from api module.

In any case, it was just a question to see if I was getting the big picture here, no worries. Thanks for your time explaining this in more detail.

[marcosholgado]

If that was the case then yes, you could use PrivacyFeatureName in the plugin and extend in the impl modules to create more specific feature names but we want to expose those features (like AppFeatureName) from the different api modules.

[aitorvs]

Awesome job! works as expected 🙏