1065306 - Write search engine metadata from region.properties to res/raw at build time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

I'm breaking this issue out of bug 1024527.

In bug 1065123, I'm going to write a patch to remove this string from search_strings.dtd, and we can use this bug as a follow-up to come up with a proper solution.

Comment 1

[Axel Hecht [:Pike]]

Note, we're already having logic in mobile/android/base/locales to extract information from region.properties with intended defaults, we might be able to embrace and extend that here so that we don't need to bother about gecko and .properties at runtime.

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #1)
> Note, we're already having logic in mobile/android/base/locales to extract
> information from region.properties with intended defaults, we might be able
> to embrace and extend that here so that we don't need to bother about gecko
> and .properties at runtime.

Pike's right.  For suggested sites, we (me, but mostly lucasr) built http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/generate_suggestedsites.py to write a raw, localized JSON resource at build time.  It extracts the required information from region.properties.

I suggest we do exactly the same thing here.  We'll extract the search list and meta-data into a raw, localized JSON resource at build time, and consume it in Java just like suggested sites does.  This will provide localized defaults; it does not try to keep these settings in sync with Gecko's settings, etc.

In fennec-search, you can import an en-US version of the raw resource for testing, etc.

margaret: Do you agree with this approach?  If so, I will drive this.

Comment 3

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to Axel Hecht [:Pike] from comment #1)
> > Note, we're already having logic in mobile/android/base/locales to extract
> > information from region.properties with intended defaults, we might be able
> > to embrace and extend that here so that we don't need to bother about gecko
> > and .properties at runtime.
> 
> Pike's right.  For suggested sites, we (me, but mostly lucasr) built
> http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> action/generate_suggestedsites.py to write a raw, localized JSON resource at
> build time.  It extracts the required information from region.properties.
> 
> I suggest we do exactly the same thing here.  We'll extract the search list
> and meta-data into a raw, localized JSON resource at build time, and consume
> it in Java just like suggested sites does.  This will provide localized
> defaults; it does not try to keep these settings in sync with Gecko's
> settings, etc.
> 
> In fennec-search, you can import an en-US version of the raw resource for
> testing, etc.
> 
> margaret: Do you agree with this approach?  If so, I will drive this.

Sounds like a good plan to me! The main goal here is to not block on gecko, but this approach addresses that. Thanks for taking this on!

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: s/suggestedsites-json/suggestedsites/. r=mshal

This is a mechanical change.  In future, we will probably want to
generate files not named like res/raw/file.json.  I'm not going to
handle res/not-raw right now; but I am going to handle not-file.json.
This smoothes that path.

mshal: you are the last build peer standing :)

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Extract generated_file_template. r=mshal

This was essentially mechanical: s/$/$$/; s/suggestedsites/$(1)/; move
libs realchrome:: and GARBAGE rules into template.

I feel like there should be a more direct form of $$($(1)), but I can't
think of it.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Extract DotProperties helper. r=lucasr,mshal

The only substantive change here is to stop stripping the
'browser.suggestedsites.' prefix from each line when reading
region.properties.

mshal: looking at you for Python blessing.

lucasr: looking at you to see if you're okay with the DotProperties
abstraction I factored out.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Write localized res/raw-*/browsersearch.json. r=mshal

mshal: it would be possible to do something fancier, like
INSTALL_TARGETS and friends; but I don't know where this will evolve
to, so let's keep this special cased for now.

Comment 8

[Nick Alexander :nalexander [he/him]]

margaret: this writes res/raw-*/browsersearch.json; the raw-* directory is raw/ for en-US and raw-ANDROID_TAG for other locales.  (It's just like suggestedsites.json.)

The json itself looks like:

{"default": "Google", "engines": ["Google", "Yahoo", "Bing"]}

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8489703 [details] [diff] [review]
Part 3: Extract DotProperties helper. r=lucasr,mshal

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

To be honest, I don't feel that confident to review python code ;-) I didn't see anything obvious wrong with this patch.

Wasn't Pike about to land something in m-c that would give us a proper properties file parser?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 8489703 [details] [diff] [review]
> Part 3: Extract DotProperties helper. r=lucasr,mshal
> 
> Review of attachment 8489703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To be honest, I don't feel that confident to review python code ;-) I didn't
> see anything obvious wrong with this patch.

That's fine; I mostly wanted you to be aware that changes were happening.

> Wasn't Pike about to land something in m-c that would give us a proper
> properties file parser?

I was not aware of anything like this, and searching the existing Python in-tree turned up nothing.  Pike, have I duplicated some work?

Comment 11

[Francesco Lodolo [:flod]]

That would be bug 940103, but I'm not sure if it's close to land.

Comment 12

[Axel Hecht [:Pike]]

So, yes, I'm trying to get to a point where we can add the compare-locales code including the properties parser to the mozilla-central tree.

I don't yet fully agree with myself on where's the cut to start cross-landing on mozilla, but I'm not really at the cut yet, I think.

Practically, it might be good enough to stay at a hacky .properties parser, and go with a more thoroughly defined and tested parser once it lands in the tree.

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8489701 [details] [diff] [review]
Part 2: Extract generated_file_template. r=mshal

>-suggestedsites-srcdir := $(if $(filter en-US,$(AB_CD)),,$(or $(realpath $(L10NBASEDIR)),$(abspath $(L10NBASEDIR)))/$(AB_CD)/mobile/chrome)
>+# Arg 1: Valid Make identifier, like suggestedsites.
>+# Arg 2: File name, like suggestedsites.json.
>+define generated_file_template
>+$(1)-srcdir := $$(if $$(filter en-US,$$(AB_CD)),,$$(or $$(realpath $$(L10NBASEDIR)),$$(abspath $$(L10NBASEDIR)))/$$(AB_CD)/mobile/chrome)

Correct me if I'm wrong, but I don't think you need to set $(1)-srcdir inside this define. It looks like the value on the right doesn't depend on the call arguments at all (ie: $1 or $2), so it should be the same for both suggestedsites and browsersearch. You can probably just keep it outside of the define and have a common name like "l10n-srcdir" (or something better :).

> # Determine the ../res/raw[-*] path.  This can be ../res/raw when no
> # locale is explicitly specified.
>-suggestedsites-bypath = $(filter %/suggestedsites.json,$(MAKECMDGOALS))
>-ifeq (,$(strip $(suggestedsites-bypath)))
>-  suggestedsites-bypath = $(suggestedsites)
>+$(1)-bypath = $$(filter %/$(2),$$(MAKECMDGOALS))

I don't think you need to escape the $$ here. Normally you would need that in a eval/call macro if you were using something like $@ in a rule definition so that $(call) doesn't expand it, but you don't have that here. What you have gets expanded by $(call) to:

suggestedsites-bypath = $(filter %/suggestedsites.json,$(MAKECMDGOALS))

Which still works fine when eval'd, since the right-hand side would be expanded again. However, if you have:

$(1)-bypath = $(filter %/$(2),$(MAKECMDGOALS))

Then it would be expanded by $(call) as:

suggestedsites-bypath = ../res/raw/suggestedsites.json

Which has the same effect (assuming 'make ../../res/raw/suggestedsites.json' to set MAKECMDGOALS). So both should work, but the extra $$'s aren't necessary and just make it more confusing IMO.

>+ifeq (,$$(strip $$($(1)-bypath)))

Here you do need the $$ since the variable won't be set until $(eval) starts running, so this is fine.

>+  $(1)-bypath = $$($(1))

Here you can just use $($(1)) since "suggestedsites" is set outside of the macro. Ie: it has the same value whether it is expanded by $(call) or $(eval).

>+$(1)-dstdir-raw = $$(patsubst %/,%,$$(dir $$($(1)-bypath)))

Here $$ is needed again, so this is fine.

>+GARBAGE += $$($(1))
>+
>+libs realchrome:: $$($(1))

These can both use $($(1))

> $(suggestedsites-dstdir-raw)/suggestedsites.json: FORCE
> 	$(call py_action,generate_suggestedsites, \
> 		--verbose \
> 		--android-package-name=$(ANDROID_PACKAGE_NAME) \
> 		--resources=$(srcdir)/../resources \
> 		$(if $(filter en-US,$(AB_CD)),,--srcdir=$(suggestedsites-srcdir)) \

Although you weren't changing it as part of this patch, this if statement can probably be simplified to: $(addprefix --srcdir=,$(suggestedsites-srcdir))
Or if you move the srcdir out: $(addprefix --srcdir=,$(l10n-srcdir))

Since the $(if $(filter en-US,$(AB_CD))) condition is already used to set the variable, it should have the same affect. (Note: This could also be done in part 4 for the browsersearch rule)

I'd like to see what this part and part 4 look like with these changes before r+. Or if you tell me all these things are actually necessary for make to work, I'll want to revisit my assumptions anyway :)

Comment 14

[Michael Shal [:mshal]]

Comment on attachment 8489703 [details] [diff] [review]
Part 3: Extract DotProperties helper. r=lucasr,mshal

>+    @staticmethod
>+    def _read_properties_as_dict(file):

Why do you need this static method? I think you can move this whole function into the body of update() and just do 'self._properties[k] = v' instead of 'properties[k] = v' (and remove the local properties variable).

>+    def get_list(self, prefix):
>+        '''Turns {'list.0':'foo', 'list.1':'bar'} into ['foo', 'bar'].
>+
>+Returns [] to indicate an empty or missing list.'''

Please indent the docstrings. Eg:
 
        """Turns {'list.0':'foo', 'list.1':'bar'} into ['foo', 'bar'].
        Returns [] to indicate an empty or missing list.
        """
>+If |required_keys| is present, it must be an iterable of required key names.  If
>+a required key is not present, ValueError is thrown.
>+
>+Returns {} to indicate an empty or missing dict.'''

Here too.

>diff --git a/python/mozbuild/mozbuild/test/test_dotproperties.py b/python/mozbuild/mozbuild/test/test_dotproperties.py
>new file mode 100644

You'll need to list test_dotproperties.py in m-c/python/moz.build, otherwise it won't run automatically :/

Comment 15

[Michael Shal [:mshal]]

Comment on attachment 8489704 [details] [diff] [review]
Part 4: Write localized res/raw-*/browsersearch.json. r=mshal

> 		$(if $(filter en-US,$(AB_CD)),,--srcdir=$(suggestedsites-srcdir)) \

Please investigate using $(addprefix --srcdir=,$(l10n-srcdir)) instead of this line, as mentioned in #c13.

I think the rest of it looks fine, so assuming the changes to part 2 don't cause any fallout here this should be good to go.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Extract generated_file_template. r=mshal

This was essentially mechanical: s/$/$$/; s/suggestedsites/$(1)/; move
libs realchrome:: and GARBAGE rules into template.  I did replace some
$$ with $ after review.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Extract DotProperties helper. r=lucasr,mshal

The only substantive change here is to stop stripping the
'browser.suggestedsites.' prefix from each line when reading
region.properties.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Write localized res/raw-*/browsersearch.json. r=mshal

I missed some small mobile/android/base/Makefile.in changes, but they
don't require additional eyes.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8492328 [details] [diff] [review]
Part 3: Extract DotProperties helper. r=lucasr,mshal

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

Carrying forward mshal's review.  I took the update suggestion, and formatted the docstrings as requested.

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8492331 [details] [diff] [review]
Part 4: Write localized res/raw-*/browsersearch.json. r=mshal

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

I don't think this needs re-review; carrying forward mshal's review.

Comment 21

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Shal [:mshal] from comment #13)
> Comment on attachment 8489701 [details] [diff] [review]
> Part 2: Extract generated_file_template. r=mshal
>
> >-suggestedsites-srcdir := $(if $(filter en-US,$(AB_CD)),,$(or $(realpath $(L10NBASEDIR)),$(abspath $(L10NBASEDIR)))/$(AB_CD)/mobile/chrome)
> >+# Arg 1: Valid Make identifier, like suggestedsites.
> >+# Arg 2: File name, like suggestedsites.json.
> >+define generated_file_template
> >+$(1)-srcdir := $$(if $$(filter en-US,$$(AB_CD)),,$$(or $$(realpath $$(L10NBASEDIR)),$$(abspath $$(L10NBASEDIR)))/$$(AB_CD)/mobile/chrome)
>
> Correct me if I'm wrong, but I don't think you need to set $(1)-srcdir
> inside this define. It looks like the value on the right doesn't depend on
> the call arguments at all (ie: $1 or $2), so it should be the same for both
> suggestedsites and browsersearch. You can probably just keep it outside of
> the define and have a common name like "l10n-srcdir" (or something better :).

This is tricky.  I did extract l10n-srcdir, but L10NBASEDIR is not
available at regular build time, which is when $(AB_CD)=en-US.  So we
can't define l10n-srcdir universally.

The next chunk was lower down, but I want to address it here.

> > $(suggestedsites-dstdir-raw)/suggestedsites.json: FORCE
> > 	$(call py_action,generate_suggestedsites, \
> > 		--verbose \
> > 		--android-package-name=$(ANDROID_PACKAGE_NAME) \
> > 		--resources=$(srcdir)/../resources \
> > 		$(if $(filter en-US,$(AB_CD)),,--srcdir=$(suggestedsites-srcdir)) \
>
> Although you weren't changing it as part of this patch, this if statement
> can probably be simplified to: $(addprefix
> --srcdir=,$(suggestedsites-srcdir))
> Or if you move the srcdir out: $(addprefix --srcdir=,$(l10n-srcdir))
>
> Since the $(if $(filter en-US,$(AB_CD))) condition is already used to set
> the variable, it should have the same affect. (Note: This could also be done
> in part 4 for the browsersearch rule)

Since l10n-srcdir is not always defined, it's not worth the effort to
unify this.  In fact, I like having the clear logic about "sometimes we
have two srcdirs, sometime one".  That's what future maintainers should see.

> > # Determine the ../res/raw[-*] path.  This can be ../res/raw when no
> > # locale is explicitly specified.
> >-suggestedsites-bypath = $(filter %/suggestedsites.json,$(MAKECMDGOALS))
> >-ifeq (,$(strip $(suggestedsites-bypath)))
> >-  suggestedsites-bypath = $(suggestedsites)
> >+$(1)-bypath = $$(filter %/$(2),$$(MAKECMDGOALS))
>
> I don't think you need to escape the $$ here. Normally you would need that
> in a eval/call macro if you were using something like $@ in a rule
> definition so that $(call) doesn't expand it, but you don't have that here.
> What you have gets expanded by $(call) to:
>
> suggestedsites-bypath = $(filter %/suggestedsites.json,$(MAKECMDGOALS))
>
> Which still works fine when eval'd, since the right-hand side would be
> expanded again. However, if you have:
>
> $(1)-bypath = $(filter %/$(2),$(MAKECMDGOALS))
>
> Then it would be expanded by $(call) as:
>
> suggestedsites-bypath = ../res/raw/suggestedsites.json
>
> Which has the same effect (assuming 'make ../../res/raw/suggestedsites.json'
> to set MAKECMDGOALS). So both should work, but the extra $$'s aren't
> necessary and just make it more confusing IMO.
>
> >+ifeq (,$$(strip $$($(1)-bypath)))
>
> Here you do need the $$ since the variable won't be set until $(eval) starts
> running, so this is fine.
>
> >+  $(1)-bypath = $$($(1))
>
> Here you can just use $($(1)) since "suggestedsites" is set outside of the
> macro. Ie: it has the same value whether it is expanded by $(call) or
> $(eval).
>
> >+$(1)-dstdir-raw = $$(patsubst %/,%,$$(dir $$($(1)-bypath)))
>
> Here $$ is needed again, so this is fine.
>
> >+GARBAGE += $$($(1))
> >+
> >+libs realchrome:: $$($(1))
>
> These can both use $($(1))

I did all of these.  Thanks for the education.

<moved chunk up>

> I'd like to see what this part and part 4 look like with these changes
> before r+. Or if you tell me all these things are actually necessary for
> make to work, I'll want to revisit my assumptions anyway :)

Sounds good.  Only assumption was about L10NBASEDIR.  This has bitten me
so many times!

Comment 22

[Michael Shal [:mshal]]

(In reply to Nick Alexander :nalexander from comment #21)
> (In reply to Michael Shal [:mshal] from comment #13)
> > Comment on attachment 8489701 [details] [diff] [review]
> > Part 2: Extract generated_file_template. r=mshal
> >
> > >-suggestedsites-srcdir := $(if $(filter en-US,$(AB_CD)),,$(or $(realpath $(L10NBASEDIR)),$(abspath $(L10NBASEDIR)))/$(AB_CD)/mobile/chrome)
> > >+# Arg 1: Valid Make identifier, like suggestedsites.
> > >+# Arg 2: File name, like suggestedsites.json.
> > >+define generated_file_template
> > >+$(1)-srcdir := $$(if $$(filter en-US,$$(AB_CD)),,$$(or $$(realpath $$(L10NBASEDIR)),$$(abspath $$(L10NBASEDIR)))/$$(AB_CD)/mobile/chrome)
> >
> > Correct me if I'm wrong, but I don't think you need to set $(1)-srcdir
> > inside this define. It looks like the value on the right doesn't depend on
> > the call arguments at all (ie: $1 or $2), so it should be the same for both
> > suggestedsites and browsersearch. You can probably just keep it outside of
> > the define and have a common name like "l10n-srcdir" (or something better :).
> 
> This is tricky.  I did extract l10n-srcdir, but L10NBASEDIR is not
> available at regular build time, which is when $(AB_CD)=en-US.  So we
> can't define l10n-srcdir universally.

What do you mean "define l10n-srcdir universally"? I was just suggesting to pull it outside of the define block, which it looks like you've done in the new patch. So I think we're both happy here?

> The next chunk was lower down, but I want to address it here.
> 
> > > $(suggestedsites-dstdir-raw)/suggestedsites.json: FORCE
> > > 	$(call py_action,generate_suggestedsites, \
> > > 		--verbose \
> > > 		--android-package-name=$(ANDROID_PACKAGE_NAME) \
> > > 		--resources=$(srcdir)/../resources \
> > > 		$(if $(filter en-US,$(AB_CD)),,--srcdir=$(suggestedsites-srcdir)) \
> >
> > Although you weren't changing it as part of this patch, this if statement
> > can probably be simplified to: $(addprefix
> > --srcdir=,$(suggestedsites-srcdir))
> > Or if you move the srcdir out: $(addprefix --srcdir=,$(l10n-srcdir))
> >
> > Since the $(if $(filter en-US,$(AB_CD))) condition is already used to set
> > the variable, it should have the same affect. (Note: This could also be done
> > in part 4 for the browsersearch rule)
> 
> Since l10n-srcdir is not always defined, it's not worth the effort to
> unify this.  In fact, I like having the clear logic about "sometimes we
> have two srcdirs, sometime one".  That's what future maintainers should see.

Right, I was just saying that $(addprefix) could be used instead of repeating the $(if) condition, since $(addprefix) on an empty string (the AB_CD==en-US case) will also be an empty string. It's not a big deal either way, though.

Comment 24

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/cc4d456a02e7
https://hg.mozilla.org/integration/fx-team/rev/7a1c2d387052
https://hg.mozilla.org/integration/fx-team/rev/aa8972afcbe5
https://hg.mozilla.org/integration/fx-team/rev/c4ecd64d8b95

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/cc4d456a02e7
https://hg.mozilla.org/mozilla-central/rev/7a1c2d387052
https://hg.mozilla.org/mozilla-central/rev/aa8972afcbe5
https://hg.mozilla.org/mozilla-central/rev/c4ecd64d8b95

Comment 26

[Carsten Book [:Tomcat]]

sorry had to back this out since it seems this were breaking the Android Nightly Builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=48740120&tree=Mozilla-Central

Comment 27

[Francesco Lodolo [:flod]]

It looks like it breaks with utf-8 characters
http://hg.mozilla.org/l10n-central/da/file/default/mobile/chrome/region.properties#l8
http://hg.mozilla.org/l10n-central/ko/file/default/mobile/chrome/region.properties#l32
http://hg.mozilla.org/l10n-central/ru/file/default/mobile/chrome/region.properties#l7

Comment 28

[Nick Alexander :nalexander [he/him]]

(In reply to Francesco Lodolo [:flod] from comment #27)
> It looks like it breaks with utf-8 characters
> http://hg.mozilla.org/l10n-central/da/file/default/mobile/chrome/region.
> properties#l8
> http://hg.mozilla.org/l10n-central/ko/file/default/mobile/chrome/region.
> properties#l32
> http://hg.mozilla.org/l10n-central/ru/file/default/mobile/chrome/region.
> properties#l7

Thanks for the debugging help.  Ah, Python and unicode -- a match made in heaven.

Comment 29

[Nick Alexander :nalexander [he/him]]

Attachment: Post: Handle utf-8 files in DotProperties. r=gps

gps: Python and Unicode, a match made in heaven.  This is somewhat
similar to https://bugzilla.mozilla.org/show_bug.cgi?id=831381.

This passes tests locally and seems to work with region.properties
from .ko, .da, and .ru.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8496948 [details] [diff] [review]
Post: Handle utf-8 files in DotProperties. r=gps

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

::: python/mozbuild/mozbuild/test/test_dotproperties.py
@@ +86,5 @@
> +# Danish.
> +# ####  ~~ Søren Munk Skrøder, sskroeder - 2009-05-30 @ #mozmae
> +
> +# Korean.
> +A.title=한메일

Oh, hmm. This works?

Python source code itself is interpreted as ASCII unless you are doing something from http://legacy.python.org/dev/peps/pep-0263/ that changes the encoding. This is separate from unicode_literals, which changes whether '' creates a str or unicode type (in Python 2).

Are you sure this test is actually running?

@@ +101,5 @@
> +        # The contents of region.properties is identical to the contents of the
> +        # test above.  This specifically exercises reading from a file.
> +        p = DotProperties(os.path.join(test_data_path, 'region.properties'))
> +        self.assertEqual(p.get_dict('A'), {'title': '한메일'})
> +        self.assertEqual(p.get_list('list'), ['test', 'Яндекс'])

Can we get a failing test?

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8496948 [details] [diff] [review]
Post: Handle utf-8 files in DotProperties. r=gps

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

I read the patch wrong (thanks Bugzilla). The coding line in the file is proper. Ignore my comment about source encoding.

Comment 32

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #30)
> Comment on attachment 8496948 [details] [diff] [review]
> Post: Handle utf-8 files in DotProperties. r=gps
> 
> Review of attachment 8496948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/test/test_dotproperties.py
> @@ +86,5 @@
> > +# Danish.
> > +# ####  ~~ Søren Munk Skrøder, sskroeder - 2009-05-30 @ #mozmae
> > +
> > +# Korean.
> > +A.title=한메일
> 
> Oh, hmm. This works?
> 
> Python source code itself is interpreted as ASCII unless you are doing
> something from http://legacy.python.org/dev/peps/pep-0263/ that changes the
> encoding. This is separate from unicode_literals, which changes whether ''
> creates a str or unicode type (in Python 2).
> 
> Are you sure this test is actually running?

For future archaeoligists: it was run, see the extract from https://tbpl.mozilla.org/php/getParsedLog.php?id=48728019&tree=Mozilla-Central&full=1:

TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_containers.py | test_defaults
TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_dotproperties.py | test_get
TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_dotproperties.py | test_get_dict
TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_dotproperties.py | test_get_list
TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_dotproperties.py | test_update
TEST-PASS | /builds/slave/m-cen-lx-000000000000000000000/build/python/mozbuild/mozbuild/test/test_expression.py | test_in

Comment 33

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/f84821d9b7ce
https://hg.mozilla.org/integration/fx-team/rev/d305d2d5576a
https://hg.mozilla.org/integration/fx-team/rev/0b6eef1ff9cd
https://hg.mozilla.org/integration/fx-team/rev/f2901a47e53d
https://hg.mozilla.org/integration/fx-team/rev/fa409031b723

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f84821d9b7ce
https://hg.mozilla.org/mozilla-central/rev/d305d2d5576a
https://hg.mozilla.org/mozilla-central/rev/0b6eef1ff9cd
https://hg.mozilla.org/mozilla-central/rev/f2901a47e53d
https://hg.mozilla.org/mozilla-central/rev/fa409031b723