Closed Bug 1369685 Opened 3 years ago Closed 3 years ago

Add a new key on Switchboard "region"

Categories

(Firefox for Android :: Metrics, enhancement)

Product:
Firefox for Android
Component:
Metrics
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

Details

Attachments

(1 file)

Bug 1369685 - Add 'region' to switchboard.
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
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.
Assignee: nobody → cnevinchen
Depends on: 1369684
NI me to update https://wiki.mozilla.org/Mobile/Fennec/Android/Switchboard later
Flags: needinfo?(cnevinchen)
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-
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 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! :)
Flags: needinfo?(cnevinchen)
Keywords: checkin-needed
Oop. Since now is soft code freeze. I think I better land this later.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1
Add 'region' to switchboard. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.