1171157 - Use nested conditionals in GeckoPreferences

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Richard Newman [:rnewman]]

GeckoPreferences has a big `if` statement like this:

                } else if (!AppConstants.NIGHTLY_BUILD &&
                           PREFS_LOGIN_MANAGE.equals(key)) {
                    preferences.removePreference(pref);
                    i--;
                } else if (AppConstants.RELEASE_BUILD &&
                           PREFS_DISPLAY_REFLOW_ON_ZOOM.equals(key)) {
                    // Remove UI for reflow on release builds.
                    preferences.removePreference(pref);
                    i--;
                    continue;
                }

This has the potential to do excessive work: we check a flag *before* we check the pref string. If, say, the incoming pref is PREFS_UPDATER_AUTODOWNLOAD, but the updater is off, we'll never check the pref... and then we'll check every other clause until we run out of `if` statements!

We just added one that's more expensive:

                } else if (PREFS_VOICE_SEARCH_ENABLED.equals(key) &&
                        (!AppConstants.NIGHTLY_BUILD || !VoiceRecognizerUtils.supportsVoiceRecognizer(getApplicationContext(), getResources()))) {
                    // Remove UI for voice search on non nightly builds.
                    preferences.removePreference(pref);
                    i--;
                    continue;
                }

We run through this huge set of conditionals once per pref.

We should consider nesting these so that we detect the pref first and short-circuit, rather than running through the entire conditional for each non-removed pref.

Comment 1

[Richard Newman [:rnewman]]

This would benefit from profiling when launching settings on a mid-range device. If we're spending non-trivial time in setupPreferences, we should go ahead with this bug.

Comment 2

[Sergej Kravcenko]

Attachment: Light optimizations

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8625990 [details] [diff] [review]
Light optimizations

Review of attachment 8625990 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r+ with those duplicate parens fixed.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +718,1 @@
>                              PREFS_TRACKING_PROTECTION_LEARN_MORE.equals(key))) {

Duplicate parens.

@@ +732,1 @@
>                              PREFS_HEALTHREPORT_LINK.equals(key))) {

Duplicate parens.

@@ +745,1 @@
>                              PREFS_GEO_LEARN_MORE.equals(key))) {

Duplicate parens.

Comment 4

[Sergej Kravcenko]

Attachment: parens fixed

Fixed those parens, plus one missed if statement

Comment 5

[Richard Newman [:rnewman]]

Sergej, this doesn't apply to current trunk. Could you rebase, please?

Comment 6

[Sergej Kravcenko]

Attachment: parens fixed, rebase

Here you go, sorry for prev version.

Comment 7

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2558ca212be8

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/658dca224bfc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/658dca224bfc