Add a new key on Switchboard "region"

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cnevinchen, Assigned: cnevinchen)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

We want to target the users in Switchboard using the country name.


This will be compared with the country we get in the search engine manager

If it doesn't exist then just using the value as it is stored in Android SharePreference - and if it doesn't exist then the experiment isn't enabled for this user.
Comment hidden (mozreview-request)
Assignee: nobody → cnevinchen
Depends on: 1369684
NI me to update https://wiki.mozilla.org/Mobile/Fennec/Android/Switchboard later
Flags: needinfo?(cnevinchen)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8873784 [details]
Bug 1369685 - Add 'region' to switchboard.

https://reviewboard.mozilla.org/r/145206/#review150036

Overall this look good. Some improvements/nits here and there. This is a great new feature for switchboard experiments! :)

::: commit-message-39d5c:1
(Diff revision 5)
> +Bug 1369685 - Add 'region' to swtichboard. r?sebastian

typo: swtichboard -> switchboard

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:181
(Diff revision 5)
>              // inactive experiment is missing from the config.
>              return false;
>          }
>      }
>  
> +    private static boolean isTargetRegion(JSONArray regions, String region) throws JSONException {

If this would be Kotlin I'd write an extension function for JSONArray now... :)

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:182
(Diff revision 5)
> +        boolean found = false;
> +        for (int i = 0; i < regions.length(); i++) {
> +            if (regions.get(i).equals(region)) {
> +                found = true;
> +            }
> +        }
> +        return found;

Can't we just return early if the region is found and return false otherwise? I guess there's no need for "found" and keep looping?

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:184
(Diff revision 5)
>      }
>  
> +    private static boolean isTargetRegion(JSONArray regions, String region) throws JSONException {
> +        boolean found = false;
> +        for (int i = 0; i < regions.length(); i++) {
> +            if (regions.get(i).equals(region)) {

maybe use equalsIgnoreCase to be safe?

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:224
(Diff revision 5)
>      }
>  
>      /**
>       * Return false if the match object contains any non-matching patterns. Otherwise returns true.
>       */
> -    private static boolean isMatch(Context context, @Nullable JSONObject matchKeys) {
> +    private static boolean isMatch(Context context, @Nullable JSONObject matchKeys) throws JSONException {

I think we should catch the JSONException locally (like we do for the other matchers) - I don't think we need to let it bubble up.

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:317
(Diff revision 5)
>          }
>  
> +        if (matchKeys.has(KEY_REGION)) {
> +            final JSONArray regions = matchKeys.getJSONArray(KEY_REGION);
> +            if (regions.length() > 0) {
> +                String region = GeckoSharedPrefs.forApp(context).getString(SearchEngineManager.PREF_REGION_KEY, null);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:318
(Diff revision 5)
>  
> +        if (matchKeys.has(KEY_REGION)) {
> +            final JSONArray regions = matchKeys.getJSONArray(KEY_REGION);
> +            if (regions.length() > 0) {
> +                String region = GeckoSharedPrefs.forApp(context).getString(SearchEngineManager.PREF_REGION_KEY, null);
> +                if (region == null) {

You could move the length and null check inside isTargetRegion() to make the code a little bit simpler here.

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:321
(Diff revision 5)
> +            if (regions.length() > 0) {
> +                String region = GeckoSharedPrefs.forApp(context).getString(SearchEngineManager.PREF_REGION_KEY, null);
> +                if (region == null) {
> +                    return false;
> +                } else {
> +                    if (!isTargetRegion(regions, region)) return false;

nit/style: Always use {} for if statements.
Attachment #8873784 - Flags: review?(s.kaspari) → review-
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8873784 [details]
Bug 1369685 - Add 'region' to switchboard.

https://reviewboard.mozilla.org/r/145206/#review150036

> If this would be Kotlin I'd write an extension function for JSONArray now... :)

operator fun JSONArray.iterator(): Iterator<JSONObject> 
    = (0 until length()).asSequence().map { get(it) as JSONObject }.iterator()

XD
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8873784 [details]
Bug 1369685 - Add 'region' to switchboard.

https://reviewboard.mozilla.org/r/145206/#review150732

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:187
(Diff revision 6)
> +        if (regions == null || region == null) {
> +            return false;
> +        }
> +        for (int i = 0; i < regions.length(); i++) {
> +
> +            final String checkingRegion = (String) regions.get(i);

JSONArray.getString() ?

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:322
(Diff revision 6)
>  
> +        if (matchKeys.has(KEY_REGION)) {
> +            try {
> +                final JSONArray regions = matchKeys.getJSONArray(KEY_REGION);
> +                if (regions.length() <= 0) {
> +                    return false;

Do we really want to return false here? If the array is empty then I guess this means there are no region restrictions?

::: mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java:331
(Diff revision 6)
> +                if (!isTargetRegion(regions, region)) {
> +                    return false;
> +                }
> +            } catch (JSONException e) {
> +                Log.e(TAG, "Exception matching region", e);
> +                return false;

I wouldn't return here false either. The JSON is somehow broken (or this version doesn't understand a different format) - I'd just log and continue.
Attachment #8873784 - Flags: review?(s.kaspari) → review+
(In reply to Nevin Chen [:nechen] from comment #8)
> Comment on attachment 8873784 [details]
> Bug 1369685 - Add 'region' to switchboard.
> 
> https://reviewboard.mozilla.org/r/145206/#review150036
> 
> > If this would be Kotlin I'd write an extension function for JSONArray now... :)
> 
> operator fun JSONArray.iterator(): Iterator<JSONObject> 
>     = (0 until length()).asSequence().map { get(it) as JSONObject
> }.iterator()
> 
> XD

Nice! :)
Comment hidden (mozreview-request)
Flags: needinfo?(cnevinchen)
Keywords: checkin-needed
Oop. Since now is soft code freeze. I think I better land this later.
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1
Add 'region' to switchboard. r=sebastian

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.