1671453 - webextPerms.description.* in mobile/android/locales/en-US/chrome/browser.properties is not used in Fenix

Status: RESOLVED FIXED

(GeckoView :: Extensions, task, P3)

Description

[Rob Wu [:robwu]]

Fenix doesn't use permission strings from Gecko, but from Android-Components:

https://github.com/mozilla-mobile/android-components/blob/05dfb097aac27ce97d3a8fa1df40d4032c47885e/components/feature/addons/src/main/java/mozilla/components/feature/addons/Addon.kt#L170-L194

https://github.com/mozilla-mobile/android-components/blob/8ec16f840e249d457a31b82df4cd1e87c2099d43/components/feature/addons/src/main/res/values/strings.xml#L6-L53

None of the strings at https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/mobile/android/locales/en-US/chrome/browser.properties#107-183 are currently used by GeckoView.

It's odd to host the permission warnings outside of the codebase of Gecko / GeckoView (since Gecko / GeckoView should decide which extension APIs cause permission warnings, not the GeckoView team). But if we want to stick with that decision, then we may as well remove these strings from mozilla-central.

Comment 1

[Rob Wu [:robwu]]

Agi, do you recall where we made the decision to use Java to generate the permission warnings? This has potential to cause maintenance issues, e.g. if someone unaware of this adds a new API to Firefox with a permission warning that isn't present in A-C.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

We should definitely remove those strings, we don't really share strings between Gecko and AC/Fenix, that's why we didn't do it here.

If a new warning is generated I think AC crashes, so we will notice. Long term we should really surface the list of permissions somewhere in the API so that apps can break the build when a new one is added.

Comment 3

[Rob Wu [:robwu]]

(In reply to Agi Sferro | :agi | ⏰ PST | he/him from comment #2)

If a new warning is generated I think AC crashes, so we will notice. Long term we should really surface the list of permissions somewhere in the API so that apps can break the build when a new one is added.

Can you point to the code that is responsible for this crash? I'd like to see if it is a sustainable way to verify the existence permission warnings. AFAIK unit tests of gecko run against a special TestRunnerActivity which is independent of external apps. I would be surprised if there is any automated test coverage for verifying the completeness of the set of permission/permission warnings, but I'd gladly be proven wrong.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Right, I'm pretty sure there's nothing automated that would fail, just random people running nightly would run into this. I'm not sure where that would be in AC, I would ask in #android-components on Matrix.

Comment 5

[Tomislav Jovanovic :zombie]

Please fix this sooner rather than later, our permissions are all over the place, the likelihood of us shooting ourselves in the foot is real bad.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

The problem here is more that the APP doesn't know the full list so it doesn't know if they're localizing all the possible permissions, we should expose that somewhere in the GV API.

Comment 7

[Rob Wu [:robwu]]

Well there are multiple tasks here:

1. Remove the unused translations from Gecko code.
2. Add comments/documentation somewhere to make it obvious that new permission warnings must also be declared externally (in A-C).
3. Add test or build automation that ensures that new permissions warnings in Firefox have been mirrored to A-C, if needed.

And a nice-to-have is some assurance that the translations for A-C are consistent with those that are used for Firefox, especially because those are publicly documented: https://support.mozilla.org/en-US/kb/permission-request-messages-firefox-extensions

I think that the potential for inconsistent translations can be avoided by pulling the translations from Gecko / GeckoView. I would highly recommend doing so, unless there is an actual benefit of keeping the status quo.

Comment 8

[Rob Wu [:robwu]]

Reverting back the title of this bug, because the task of exposing the list of permissions has already been completed in bug 1622917.

I'll morph this bug back into the original report: the fact that there are strings that aren't used at all in GeckoView (except in unit tests).

To fix this bug, we need to remove the strings and update tests, by somehow figuring out a way to retrieve the strings in toolkit/components/extensions/test/xpcshell/test_ext_permissions.js, from the source at https://github.com/mozilla-mobile/android-components/blob/68ec5c4a3b4f412a7775ff07be7d853973ff4d6e/components/feature/addons/src/main/res/values/strings.xml#L7-L71

(see also bug 1632091 for a bug about the lack of test coverage for verification that the list of permissions and warnings are in sync)

Comment 9

[[:owlish] 🦉 PST]

still relevant

Comment 10

[Eemeli Aro [:eemeli]]

This was fixed in bug 1793557.