1110965 - Usage of android:defaultValue and android:persistent are not consistent

Status: RESOLVED INCOMPLETE

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

Description

[Chenxia Liu [:liuche]]

For the Preferences in our settings, we use android:defaultValue and android:persistent inconsistently in our xml files, which can lead to the pref state flickering when Settings loads slowly.

For Gecko prefs, we should not be setting a defaultValue (because this should be fetched from Gecko), and android:persistent should be false because Gecko is the authority on the pref's value, not SharedPreferences.

For non-Gecko prefs (these are prefixed by android.not_a_preference), these should live in SharedPreferences.

This bug is to make usage of these properties consistent throughout our prefs files in mobile/android/base/resources/xml*

Comment 1

[Michael Holloway [:mdholloway]]

I can take a stab at this.  

If I understand correctly, with respect to the android:persistent attribute, it should be explicitly set to false for each Gecko preference (i.e., each preference with a key not containing 'android.not_a_preference')?  Or should I just check to make sure it's not set to true for some reason?  Discussion here suggests that false is the default: https://groups.google.com/forum/#!topic/android-developers/NolcpeR77-0

Comment 2

[Chenxia Liu [:liuche]]

Hello Michael!

You're correct that the default for android:persistent is false, so it's sufficient to just make sure they're not set to true for some reason. android:defaultValue should also be set only for non-Gecko prefs.

Do you have an Firefox for Android build set up? If not, take a look at https://wiki.mozilla.org/Mobile/Fennec/Android to get started. If you run into any problems, feel free to drop into #mobile - I'm usually active in there, and if not, there are other people who are happy to help.

Once you get a build with those changes, you can verify them by visiting the Settings for FxAndroid and making sure that that they don't flicker unexpectedly anymore; you can also try changing the Gecko prefs in about:config and then visiting settings to check android:persistent. (For comparison, you can compare with the Settings in a Nightly build, which you can download at http://nightly.mozilla.org/ , or just a build that doesn't have any changes.)

Comment 3

[Jared Petersen]

I would like to work on this for my first bug. I have a Firefox for Android dev environment already set up.

It's my understanding that you would like all instances of android:defaultValue in the XML files located at <BUILD ENVIRONMENT LOCATION>/mozilla-central/mobile/android/base/resources/xml to be replaced with android:persistentValue="false" as long as it is not part of a non-Gecko preference. Is that correct?

Comment 4

[Chenxia Liu [:liuche]]

Hi Jared, let's wait to see if Michael wants to continue this bug, since he said he wanted to work on this bug just a couple of days ago. There are several other good first bugs - perhaps take a look at bug 966485 instead? I can ping you if Michael ends up not taking this bug after all.

Comment 5

[Michael Holloway [:mdholloway]]

Hi all, I do have a working Fennec build but (a) haven't started work on this bug yet and (b) have a couple of other bug fixes in the pipeline, so if Jared wants to take this one over that's fine with me.

Comment 6

[Jared Petersen]

Chenxia, is it okay for me to start working on this? If so, I had a question in Comment 3 that I would like you to take a look at.

Comment 7

[Chenxia Liu [:liuche]]

Sounds good to me!

Yes, that's correct. Additionally, I believe android:persistent is false by by default, but you should check this as well - if it's not, also explicitly set persistent=false for Gecko prefs. Then verify that the preferences screens in Fennec's Settings are behaving as expected per comment 2.

(Also be sure to check the files in xml-v11 as well.)

If you need any help, drop into #mobile, or flag me with the needinfo checkbox on the bug.

Comment 8

[Jared Petersen]

Okay, I had to edit the following files:

xml directory:

fxaccount_status_prefscreen.xml
preferences_display.xml
preferences_locale.xml
preferences_search.xml

xml-v11 directory:

preferences_search.xml

All of the other files either did not have a reference to android:defaultValue or were non-Gecko preferences

I'm in the process of building Fennec with the new files. After that, I should be able to test out my changes and submit a patch from there.

Comment 9

[Chenxia Liu [:liuche]]

Thanks Jared!

FYI, when you make the patch, the commit message should be of the format:
Bug <number>: <title>. r=<reviewer>

This helps us link the bug to the changeset it lands in, as well as the people relevant to fixing the bug.
Thanks!

Comment 10

[Jared Petersen]

Attachment: Bug 1110965: Usage of android:defaultValue and android:persistent are not consistent. r = Chenxia Liu

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8558917 [details] [diff] [review]
Bug 1110965: Usage of android:defaultValue and android:persistent are not consistent. r = Chenxia Liu

Jared, this patch doesn't look right (compressed?), and you set a bunch of nonsense flags on it. I'm going to mark this as obsolete; please try again, and please be a little more careful with those flags. Just set "review?" to :liuche.

Comment 12

[Jared Petersen]

Okay, Richard. Sorry about that. There's 5 different files total that span two directories, so I thought that I should just compress the two directories. Would you recommend that I submit 5 seperate patches, one for each file that has been edited?

Comment 13

[Richard Newman [:rnewman]]

You should make a properly formed hg commit and upload it to MozReview:

http://mozilla-version-control-tools.readthedocs.org/en/latest/devguide/contributing.html#submitting-patches-for-review


Or if you'd rather work in terms of patch files and Mercurial queues, and attach a file to this bug:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F


A patch or commit includes whatever changes you made since the previous commit, so it'll show what's changed, regardless of how many files you touch.

Comment 14

[Jared Petersen]

Attachment: bug1110965patch.diff

Comment 15

[Jared Petersen]

Chenxia, what are the next steps for this bug?

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8559380 [details] [diff] [review]
bug1110965patch.diff

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

Thanks for the patch - how does this look when you test the two versions? Do you see any difference when launching the Settings?

In the future, flag me for review on the patch using the flags (review ?) - then it will show up in my review queue.

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ -63,5 @@
>              android:title="@string/fxaccount_status_needs_finish_migrating" />
>  
>          <Preference
>              android:editable="false"
> -            android:key="sync_now"

Don't remove android:key - that has nothing to do with defaultValue or persistent.

Comment 17

[Jared Petersen]

Attachment: bug1110965fix.diff

Comment 18

[Chenxia Liu [:liuche]]

Not sure if you saw my earlier comment, I'd also like you to verify that your changes make an improvement in how Fennec Settings items load.
> 
> how does this look when you test the two versions? Do
> you see any difference when launching the Settings?
> 

I'll wait on you verifying this was the right approach - flag me for review when you've done that. Thanks Jared!

Comment 19

[Jared Petersen]

(In reply to Chenxia Liu [:liuche] from comment #18)
> Not sure if you saw my earlier comment, I'd also like you to verify that
> your changes make an improvement in how Fennec Settings items load.
> > 
> > how does this look when you test the two versions? Do
> > you see any difference when launching the Settings?
> > 
> 
> I'll wait on you verifying this was the right approach - flag me for review
> when you've done that. Thanks Jared!

Sorry, Chenxia. The preference state still flashes. What are our next steps?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning due to inactivity.

Jared, let me know if you still want to work on this.

Comment 21

[Chenxia Liu [:liuche]]

I haven't thought about the next steps for this, so I'll add [good first bug] back when I have time to figure out what we'd like to do.

Comment 22

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.