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

RESOLVED FIXED in Firefox 35

Status

P1
normal
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: Margaret, Assigned: nalexander)

Tracking

Trunk
Firefox 35
All
Android
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Summary: Define engine identifier in region.properites (or similar solution) → Define engine identifier in region.properties (or similar solution)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
(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.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(margaret.leibovic)
(Reporter)

Comment 3

4 years ago
(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!
Flags: needinfo?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Summary: Define engine identifier in region.properties (or similar solution) → Write search engine metadata from region.properties to res/raw at build time
(Assignee)

Comment 4

4 years ago
Created attachment 8489700 [details] [diff] [review]
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 :)
Attachment #8489700 - Flags: review?(mshal)
(Assignee)

Comment 5

4 years ago
Created attachment 8489701 [details] [diff] [review]
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.
Attachment #8489701 - Flags: review?(mshal)
(Assignee)

Comment 6

4 years ago
Created attachment 8489703 [details] [diff] [review]
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.
Attachment #8489703 - Flags: review?(mshal)
Attachment #8489703 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 7

4 years ago
Created attachment 8489704 [details] [diff] [review]
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.
Attachment #8489704 - Flags: review?(mshal)
(Assignee)

Comment 8

4 years ago
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 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?
Attachment #8489703 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 10

4 years ago
(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?
Flags: needinfo?(l10n)
That would be bug 940103, but I'm not sure if it's close to land.
(Assignee)

Updated

4 years ago
Blocks: 1013971

Comment 12

4 years ago
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.
Flags: needinfo?(l10n)

Updated

4 years ago
Attachment #8489700 - Flags: review?(mshal) → review+
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 :)
Attachment #8489701 - Flags: review?(mshal) → feedback+
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 :/
Attachment #8489703 - Flags: review?(mshal) → review+
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.
Attachment #8489704 - Flags: review?(mshal) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8492327 [details] [diff] [review]
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.
Attachment #8489701 - Attachment is obsolete: true
Attachment #8492327 - Flags: review?(mshal)
(Assignee)

Comment 17

4 years ago
Created attachment 8492328 [details] [diff] [review]
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.
Attachment #8489703 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
Created attachment 8492331 [details] [diff] [review]
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.
Attachment #8489704 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
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.
Attachment #8492328 - Flags: review+
(Assignee)

Comment 20

4 years ago
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.
Attachment #8492331 - Flags: review+
(Assignee)

Comment 21

4 years ago
(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!
(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.

Updated

4 years ago
Attachment #8492327 - Flags: review?(mshal) → review+
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1068127
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

4 years ago
(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.
(Assignee)

Comment 29

4 years ago
Created attachment 8496948 [details] [diff] [review]
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.
Attachment #8496948 - Flags: review?(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?
Attachment #8496948 - Flags: review?(gps) → feedback+
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.
(Assignee)

Comment 32

4 years ago
(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

Updated

4 years ago
Depends on: 1077381
(Assignee)

Updated

4 years ago
See Also: → bug 883254
(Assignee)

Updated

4 years ago
Blocks: 1123980

Updated

9 months ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.