Open Bug 1277117 Opened 4 years ago Updated 4 years ago

Move Gecko env var mapping code to IntentUtils.getEnvVarMap

Categories

(Firefox for Android :: General, defect, P5)

All
Android
defect

Tracking

()

People

(Reporter: mcomella, Unassigned, Mentored)

References

(Depends on 1 open bug)

Details

There is some duplicated code to read environment variables in GeckoLoader & GeckoApp:
  https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile%2Fandroid+env0&redirect=true

I wrote a utility function in bug 1270191 to get the environment variable methods and pass it into Experiments & TelemetryUploadService. We should move the existing Gecko code to use these methods.

Note there is also some duplication in my resulting code for Experiments & TelemetryUploadService (they set the value, throw if the value was already set, and throw if the value is accessed before they are retrieved, etc.) so it could be worth building a better (but simple! ;) abstraction around this than the map.

There is also some ambiguity about how the environment variables are used (e.g. do we want to check if the key exists in the map? What if the key is the empty string? Should we passing more specific values, e.g. `true` & `false`?) so it could be worth ironing that out too.
From bug 1277214 comment 8:
> SafeIntentUtils breaks the env var algorithm:
> 
>         while (envValue != null) {
> ...
>             envValue = intent.getStringExtra("env" + i);
> 
> SafeIntentUtils returns null when there is an error – a runtime exception or
> OOM – so we'd early return and miss some of our env vars. We should instead
> be checking containsKey or similar.

bug 1277214 will fix this for IntentUtils so if we use IntentUtils, we should get the improvements. A good quick fix here might be to call out to env var utils for these methods, with the proper fix to only call getEnvVarMap once.
(In reply to Michael Comella (:mcomella) from comment #1)
> A good quick fix here might be to call out to
> env var utils for these methods, with the proper fix to only call
> getEnvVarMap once.

Slight detour – util/ is not accessible from mozglue/ so we can't call out to IntentUtils in GeckoLoader. I filed bug 1277631 because I think this behavior is incorrect – soft dep. A quick fix is to move IntentUtils to mozglue/ because mozglue seems accessible from everywhere.
Depends on: 1277631
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.