1100363 - [mozprofile] preference names from INI file should be read case-sensitively

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Martin Kimmerle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 Iceweasel/31.2.0
Build ID: 20141102014832

Steps to reproduce:

I downloaded and installed mozrunner. Then I created an INI file called prefs.ini with the following content:

[dev]
; show about:config on startup
startup.homepage_welcome_url=about:config
; no warning on about=config
general.warnOnAboutConfig=false

Then I ran this command:

mozrunner --preferences=prefs.ini:dev


Actual results:

Firefox gets opened with a tab for `about:config`, which shows a warning.

When I search for `warnonaboutconfig` in `about:config` I get:
general.warnOnAboutConfig;true
general.warnonaboutconfig;false (all characters lowercase)



Expected results:

The warning shouldn't be displayed.

When searching for `warnonaboutconfig` in `about:config` there should be only one pref:
general.warnOnAboutConfig;false

By the way, specifying the prefs via --pref= works. See:
mozrunner --pref="startup.homepage_welcome_url:about:config" --pref="general.warnOnAboutConfig:false"

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you Martin for filing the bug. Looks like no-one really used that method so far? I had a quick look at our code and it looks like we are missing a simple line right after instantiating the config parser:

https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.optionxform

Here we should replace the internal method for optionxform with 'str', to not let the config parser lowercase the letters:

https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.optionxform

So it should be an easy bug to be fixed.

Comment 2

[Martin Kimmerle]

Thank you Henrik for your quick response. Yes, adding the line

parser.optionxform = str

directly after

parser = ConfigParser()

works for me. The comment could be

// Don't let the parser convert the pref name to lowercase

(sorry, I didn't make a patch yet, and in this case it was easier to write a comment.)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fantastic! Martin, would you be interested to work on the real patch for mozprofile? We can mentor you for all the questions you have.

Comment 4

[Martin Kimmerle]

Yes, I could create the real patch. I suppose you'll assign this bug to me then.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great! Yes, I will assign this bug to you (as done now). When you add the patch please also have a look at existent unit tests for the profile:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozfile/tests/

It would be good to get another test file with a test method this does the following:

* Read a preferences config file with a preference in camel-case style
* Check that the read preference name has not been lower-cased

If you have questions or need assistance you can also contact me on IRC (irc.mozilla.org) in the #ateam channel. My nick is whimboo. Thanks!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Martin won't have time the next few weeks, so putting this bug back into the general bucket for everyone else to grab.

Comment 7

[Ujjwal Wahi [:ujjwal]]

I would like to work on this. I have mozilla build ready. Please guide me further.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Ujjwal! Lets talk on IRC to get the remaining setup steps done for you.

Comment 9

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1100363_camelcase_fix.patch

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8542262 [details] [diff] [review]
bug1100363_camelcase_fix.patch

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

Thanks for the patch, Ujjwal! That was kinda fast. Just a short tip for you: in the future please set the review flag for the patch you upload. That way the attachment it will end-up in the review queue of the appropriate reviewer. I set it to myself now, and will have a look at it soon.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8542262 [details] [diff] [review]
bug1100363_camelcase_fix.patch

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

In general this patch looks fine. It's nice to see that you were able to pull all that together without any additional help!

But there is one thing which keeps the test failing at the moment, and which forces me to give you a review-. Please see my inline comments.

::: testing/mozbase/mozprofile/tests/test_preferences.py
@@ +65,5 @@
>          for pref, value in _prefs:
>              commandline += ["--pref", "%s:%s" % (pref, value)]
>          self.compare_generated(_prefs, commandline)
>  
> +

nit: According to PEP8 it only needs a single blank line. So please revert this change.

https://www.python.org/dev/peps/pep-0008/#blank-lines

@@ +113,5 @@
> +        Read a preferences config file with a preference in camel-case style.
> +        Check that the read preference name has not been lower-cased
> +        """
> +        # write the .ini file
> +        _ini = """general.warnOnAboutConfig = False"""

You have to add the DEFAULTS section here, similar to the test method above. Because of that the test is currently broken. Please have a look at the current output when running test.py.

@@ +121,5 @@
> +            os.close(fd)
> +            commandline = ["--preferences", name]
> +
> +            _prefs = {'general.warnOnAboutConfig': 'False'}
> +            commandline[-1] = commandline[-1] 

This is a no-op because we want to use the default section. So it can be removed.

Comment 12

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1100363_camelcase_fix.diff

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you for the update Ujjwal! The patch looks fine to me know, but before I will give my r+ I submitted it to try for results. If all unit tests are passing we can get the patch landed!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fc00f01fccc3

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8542523 [details] [diff] [review]
bug1100363_camelcase_fix.diff

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

Try run was all green. So this patch is good to get checked-in.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I fixed the author of the patch (please check your global hg config and add this information) before pushing it to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/02b4ccc6cc91

Comment 16

[Martin Kimmerle]

Thank you Ujjwal for creating the patch! And thanks to Henrik for mentoring :)

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02b4ccc6cc91