Closed
Bug 1369685
Opened 5 years ago
Closed 5 years ago
Add a new key on Switchboard "region"
Categories
(Firefox for Android Graveyard :: Metrics, enhancement)
Firefox for Android Graveyard
Metrics
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: cnevinchen, Assigned: cnevinchen)
References
Details
Attachments
(1 file)
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 | ||
Updated•5 years ago
|
Assignee: nobody → cnevinchen
Assignee | ||
Comment 2•5 years ago
|
||
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•5 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•5 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•5 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+
Comment 11•5 years ago
|
||
(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) |
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(cnevinchen)
Keywords: checkin-needed
Assignee | ||
Comment 13•5 years ago
|
||
Oop. Since now is soft code freeze. I think I better land this later.
Keywords: checkin-needed
Comment 14•5 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1 Add 'region' to switchboard. r=sebastian
Comment 15•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69f302e5caf1
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•