1264599 - GeckoPreferences: Remove CheckBoxPrefSetter and TwoStatePrefSetter

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In GeckoPreferences we use CheckBoxPrefSetter and TwoStatePrefSetter to set a preference with a checkbox in a backwards compatible way. Now with ICS as base line we don't need that anymore and can just use TwoStatePreference. 

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java#1438-1473

Comment 1

[Jorick Caberio]

Please assign this bug to me. Thanks.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

@Jorick: Any update here? :)

Comment 3

[Nicholas Rosbrook [:enr0n]]

Sebastian : Is all that needs to be done here is to remove those two inner classes, along with the version of prefValue() that uses said classes?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah. We can update prefValue() to use TwoStatePreference directly now. We don't need the backwards compatibility to handle CheckBoxPreference anymore.

Comment 5

[gui.bgd]

Attachment: GeckoPreferences.java deletion of two inner classes, update of method prefValue()

Hi, I'm new to Bugzilla, I had in mind to contribute to Firefox for Android, so once my environment setup, I looked for a bug that would fit my abilities and figured that this one is the perfect match. So I would love any feedback on this patch.

Comment 6

[gui.bgd]

Attachment: updatePrefValue.patch

Sorry for previous attachment, it's my first time contributing and I did not take the necessary time on how to propose a patch.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8754111 [details] [diff] [review]
updatePrefValue.patch

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

This is looking good and just needs some cleanup.

You can run |mach gradle checkstyle| to verify that the code matches some of our style rules.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
@@ +1447,4 @@
>          @Override
>          public void prefValue(String prefName, final boolean value) {
>              final Preference pref = getField(prefName);
> +            final TwoStatePreference prefSetter = (TwoStatePreference) pref;

nit: Let's rename prefSetter or just cast directly:
> final TwoStatePreference pref = (TwoStatePreference) getField(prefName);

@@ +1451,4 @@
>              ThreadUtils.postToUiThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    if(prefSetter.isChecked() != value)

Please add spaces and braces:
> if_(..)_{

Comment 8

[gui.bgd]

Attachment: prefValue.patch

Hi, I've made the adjustment required.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8755374 [details] [diff] [review]
prefValue.patch

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

LGTM.

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdf193d1691e