1011620 - Revisit how suggestedsites.json is generated at build time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Consider using region.properties as the source instead of the current list.txt+json files because:

- region.properties is already part of our l10n workflow.
- region.properties is an appropriate localization point for heavily managed parts of the l10n process, which is the case for suggested sites i.e. all changes to region.properties have to be reviewed before landing. 
- Using a .properties file can potentially enable localizers to override only the keys that needs changes.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Generate suggestedsites.json from region.properties (r=nalexander)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Generate suggestedsites.json from suggestedsites.properties (r=nalexander)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8425419 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

Ok, here's a first take on this. This will generate suggestedsites.json from the existing region.properties. The region.properties file would contain something like:

browser.suggestedsites.list.0=fxmarketplace
browser.suggestedsites.list.1=mozilla

browser.suggestedsites.mozilla.title=Mozilla
browser.suggestedsites.mozilla.url=https://mozilla.org/en-US/
browser.suggestedsites.mozilla.bgcolor=#c13832

browser.suggestedsites.fxmarketplace.title=Firefox Marketplace
browser.suggestedsites.fxmarketplace.url=https://marketplace.firefox.com/
browser.suggestedsites.fxmarketplace.bgcolor=#0095dd

And locales can then override any of these keys individually. The main advantage of using a .properties file is that localizers only need to override the keys that needs to change (as opposed to the JSON files that is a all-or-nothing kind of override). For example, pt-BR's region.properties file could just have:

browser.suggestedsites.mozilla.url=https://mozilla.org/pt-BR/

And all other keys would come from the en-US file.

My main concern about using region.properties is that we're adding a build-time fallback behaviour that is not expected for this file at the moment. This is likely prone to confusion. Because of that, I'm leaning more towards having a separate properties file to define/localize suggested sites.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8425420 [details] [diff] [review]
Generate suggestedsites.json from suggestedsites.properties (r=nalexander)

Ok, here's a slightly different approach. Instead of using the existing region.properties file, the suggested sites are defined/localized in a separate suggestedsites.properties file. I kinda prefer having a separate file to avoid confusion.

I don't feel strongly about either approach. I'm fine with doing whatever feels more natural for localizers here.

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 8425419 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

This looks good to me design-wise.

The Makefile stuff is a bit complicated, I wonder if there's an easier way to get that.

Also, I found a few python nits.

The compare-locales bug is bug 940103, fyi.

::: mobile/locales/en-US/chrome/region.properties
@@ +31,5 @@
>  # selection UI
>  browser.contentHandlers.types.0.title=My Yahoo!
>  browser.contentHandlers.types.0.uri=https://add.my.yahoo.com/rss?url=%s
> +
> +# Suggested sites order (order displayed in the top sites grid)

We should have a better comment here.

Flod, do you have a suggestion?

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
@@ +13,4 @@
>  
> +2. Read the list of sites from the 'list.INDEX' properties with value
> +of these keys being an identifier for each suggested site. (e.g. list.0=mozilla,
> +list.1=fxmarketplace, etc).

I guess this comment should be adapted if we use region.properties?

@@ +72,5 @@
> +        prefix = 'browser.suggestedsites.'
> +        properties = {}
> +        for l in open(filename, 'rt').readlines():
> +            line = l.strip()
> +            if len(line) is 0 or line.startswith('#') or not line.startswith(prefix):

The first two are false if not line.startswith(prefix), I'd just go for that.

@@ +74,5 @@
> +        for l in open(filename, 'rt').readlines():
> +            line = l.strip()
> +            if len(line) is 0 or line.startswith('#') or not line.startswith(prefix):
> +                continue
> +            (k, v) = re.split('\s*=\s*', line, 1)

The parser is a tad optimistic here, but that's probably good for now.

We have a bug open to add the CompareLocales code to mozilla-central, if we had that, we could use the Parser classes from there, and be more rigorous. Stuff like ':' being a valid separator, unicode escapes, trailing whitespace stripping, etc.

@@ +87,2 @@
>              path = mozpath.join(srcdir, filename)
> +            properties = dict(properties.items() + read_properties_file(path).items())

properties.update(read_properties_file(path))

@@ +105,5 @@
> +        for p in ['title', 'url', 'bgcolor']:
> +            if not p in site:
> +                raise Exception("Could not find property '{property}' for '{name}'"
> +                    .format(property=p, name=name))
> +        return site

I'd probably just go for explicitly constructing the dict.

try:
  site = dict((k, properties[name + '.' + k]) for k in ('title', 'url', 'bgcolor'))
except IndexError, e:
  raise something more useful.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8425420 [details] [diff] [review]
Generate suggestedsites.json from suggestedsites.properties (r=nalexander)

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

We're having a ton of external infrastructure that hacks up "ignore this region.properties file", I'm concerned that we're creating more hassle than fixing by adding another file to that list. Giving that an f-

Comment 7

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #5)
> > +# Suggested sites order (order displayed in the top sites grid)
> 
> We should have a better comment here.
> 
> Flod, do you have a suggestion?

Not sure, this is basically the same comment we currently have for search.order (which, I admit, is not super clear, and has a typo at the end too).

The only alternative I see is to tell the whole story.

# Order of suggested websites displayed in the Top Sites panel. 
# Values for these keys must correspond to the name used in the keys that
# define each suggested website's details. For example: 
# browser.suggestedsites.list.0=NAME
# browser.suggestedsites.NAME.title=Displayed name
# browser.suggestedsites.NAME.url=Website URL
# browser.suggestedsites.NAME.bgcolor= Color (hex format)

@Pike
Don't we need to update this file as well to ignore the new keys eventually?
http://hg.mozilla.org/mozilla-central/file/default/mobile/locales/filter.py#l34

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8425420 [details] [diff] [review]
Generate suggestedsites.json from suggestedsites.properties (r=nalexander)

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

I agree with Pike, adding external tools to the list of things that should be fixed for this to work.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8425419 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

Changes look good to me, pending question for Pike about filter.py

Unfortunately I'm not familiar at all with makefiles, so I'm wondering about the change from ":=" to "="

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 8425419 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Ok, here's a first take on this. This will generate suggestedsites.json from
> the existing region.properties. The region.properties file would contain
> something like:
> 
> browser.suggestedsites.list.0=fxmarketplace
> browser.suggestedsites.list.1=mozilla

How does a locale shorten the list?  If I set

browser.suggestedsites.list.1=

is that the same as deleting all the keys?  Or are we hard-coding the list length here?

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #10)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Comment on attachment 8425419 [details] [diff] [review]
> > Generate suggestedsites.json from region.properties (r=nalexander)
> > 
> > Ok, here's a first take on this. This will generate suggestedsites.json from
> > the existing region.properties. The region.properties file would contain
> > something like:
> > 
> > browser.suggestedsites.list.0=fxmarketplace
> > browser.suggestedsites.list.1=mozilla
> 
> How does a locale shorten the list?  If I set
> 
> browser.suggestedsites.list.1=
> 
> is that the same as deleting all the keys?  Or are we hard-coding the list
> length here?

Yeah, this is known issue in the patch, I should have pointed it out. This patch assumes localizers would have to override *all* list.* keys if they want to change the list of sites. This means they can't shorten the list. In practice, this is unlikely to become a problem because all locales will have to provide at least 9 sites (the number of grid items on tablets).

We can probably make this a bit more future-proof though. Maybe I could special-case the list.* keys so that they're always overridden as a group i.e. if you only override 'list.1', the list would have only one item (instead of inheriting all other list.X keys). This might be a bit prone to confusion though. Thoughts?

Comment 12

[Francesco Lodolo [:flod]]

A locale having less suggested sites than en-US should IMO not be an option. 

If we decide we want to provide X suggestions on that panel, and locales can't come up with X web sites, we should just fall back to en-US values.
Also, a locale could be perfectly fine with the en-US values for one site (e.g. a link to Marketplace).

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8425420 [details] [diff] [review]
Generate suggestedsites.json from suggestedsites.properties (r=nalexander)

We'll use region.properties. Marking as obsolete.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Axel Hecht [:Pike] from comment #5)
> Comment on attachment 8425419 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8425419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me design-wise.
> 
> The Makefile stuff is a bit complicated, I wonder if there's an easier way
> to get that.

Yeah, I'm sure nalexander will have some good input in this regard :-)

> ::: mobile/locales/en-US/chrome/region.properties
> @@ +31,5 @@
> >  # selection UI
> >  browser.contentHandlers.types.0.title=My Yahoo!
> >  browser.contentHandlers.types.0.uri=https://add.my.yahoo.com/rss?url=%s
> > +
> > +# Suggested sites order (order displayed in the top sites grid)
> 
> We should have a better comment here.
> 
> Flod, do you have a suggestion?
> 
> ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
> @@ +13,4 @@
> >  
> > +2. Read the list of sites from the 'list.INDEX' properties with value
> > +of these keys being an identifier for each suggested site. (e.g. list.0=mozilla,
> > +list.1=fxmarketplace, etc).
> 
> I guess this comment should be adapted if we use region.properties?

Oops, yep. Done.

> @@ +72,5 @@
> > +        prefix = 'browser.suggestedsites.'
> > +        properties = {}
> > +        for l in open(filename, 'rt').readlines():
> > +            line = l.strip()
> > +            if len(line) is 0 or line.startswith('#') or not line.startswith(prefix):
> 
> The first two are false if not line.startswith(prefix), I'd just go for that.

Good point, done.
 
> @@ +74,5 @@
> > +        for l in open(filename, 'rt').readlines():
> > +            line = l.strip()
> > +            if len(line) is 0 or line.startswith('#') or not line.startswith(prefix):
> > +                continue
> > +            (k, v) = re.split('\s*=\s*', line, 1)
> 
> The parser is a tad optimistic here, but that's probably good for now.

Yeah, this is a bit hand-wavy but should be good enough.

> We have a bug open to add the CompareLocales code to mozilla-central, if we
> had that, we could use the Parser classes from there, and be more rigorous.
> Stuff like ':' being a valid separator, unicode escapes, trailing whitespace
> stripping, etc.

Awesome. I'll keep an eye on this bug. Filed bug 1013971 as reminder to change generate_sites.py to use compare-locales' Parser classes once they land in m-c.

> @@ +87,2 @@
> >              path = mozpath.join(srcdir, filename)
> > +            properties = dict(properties.items() + read_properties_file(path).items())
> 
> properties.update(read_properties_file(path))

Interesting, I tried using update() before and it wasn't overriding the keys as expected. Not it's working fine, done.

> @@ +105,5 @@
> > +        for p in ['title', 'url', 'bgcolor']:
> > +            if not p in site:
> > +                raise Exception("Could not find property '{property}' for '{name}'"
> > +                    .format(property=p, name=name))
> > +        return site
> 
> I'd probably just go for explicitly constructing the dict.
> 
> try:
>   site = dict((k, properties[name + '.' + k]) for k in ('title', 'url',
> 'bgcolor'))
> except IndexError, e:
>   raise something more useful.

Good idea, done.

Comment 15

[Axel Hecht [:Pike]]

(In reply to Francesco Lodolo [:flod] from comment #7)
> 
> @Pike
> Don't we need to update this file as well to ignore the new keys eventually?
> http://hg.mozilla.org/mozilla-central/file/default/mobile/locales/filter.
> py#l34

Yes, you're exactly right, we need to update this one. Given the results here, we should only ignore the non-list entries.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Francesco Lodolo [:flod] from comment #7)
> (In reply to Axel Hecht [:Pike] from comment #5)
> > > +# Suggested sites order (order displayed in the top sites grid)
> > 
> > We should have a better comment here.
> > 
> > Flod, do you have a suggestion?
> 
> Not sure, this is basically the same comment we currently have for
> search.order (which, I admit, is not super clear, and has a typo at the end
> too).
> 
> The only alternative I see is to tell the whole story.
> 
> # Order of suggested websites displayed in the Top Sites panel. 
> # Values for these keys must correspond to the name used in the keys that
> # define each suggested website's details. For example: 
> # browser.suggestedsites.list.0=NAME
> # browser.suggestedsites.NAME.title=Displayed name
> # browser.suggestedsites.NAME.url=Website URL
> # browser.suggestedsites.NAME.bgcolor= Color (hex format)

This looks good to me, added that to the patch.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Generate suggestedsites.json from region.properties (r=nalexander)

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Francesco Lodolo [:flod] from comment #9)
> Comment on attachment 8425419 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8425419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes look good to me, pending question for Pike about filter.py
> 
> Unfortunately I'm not familiar at all with makefiles, so I'm wondering about
> the change from ":=" to "="

Nice catch, unintentional change. Reverted.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Generate suggestedsites.json from region.properties (r=nalexander)

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8426261 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

Here's an update version of the patch with the changes suggested by flod and Pike.

Comment 21

[Francesco Lodolo [:flod]]

Comment on attachment 8426261 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

Looks good to me.

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
@@ +101,5 @@
> +        prefix = '{name}.'.format(name=name)
> +        try:
> +            site = dict((k, properties[prefix + k]) for k in ('title', 'url', 'bgcolor'))
> +        except IndexError, e:
> +            raise Exception("Could not find property required property for '{name}: {error}'"

too many property

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8425419 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

I've noted a couple of Python nits and suggested a small stream-lining, but overall I'm pleased with how small the change to generate*.py is.

As to = vs := in Makefiles, prefer :=, since it is "evaluated immediately" and tends to be easier to reason about.  I'm pretty sure you're copy-pasting from earlier in the file, so I don't see the need to change it.

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
@@ +61,5 @@
>                          help='output')
>      opts = parser.parse_args(args)
>  
> +    def read_properties_file(filename):
> +        if not os.path.exists(filename):

nit: have this throw if the path doesn't exist.

@@ +87,2 @@
>              path = mozpath.join(srcdir, filename)
> +            properties = dict(properties.items() + read_properties_file(path).items())

nit: have this be the place that catches and ignores missing files.

@@ +90,3 @@
>  
> +    def get_site_list_from_properties(properties):
> +        prefix = 'list.'

nit: add a docstring explaining what this function does:

"Turns {'list.0':'mozilla', 'list.1':'other'} into ['mozilla', 'other']."

@@ +97,5 @@
> +            indexes.append(int(k[len(prefix):]))
> +        return [properties[prefix + str(index)] for index in sorted(indexes)]
> +
> +    def get_site_from_properties(name, properties):
> +        prefix = '{name}.'.format(name=name)

nit: docstring as well.  Turns {'mozilla.title':'title', ...} into {'title':'title', ...}.

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 8425419 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
@@ +61,5 @@
>                          help='output')
>      opts = parser.parse_args(args)
>  
> +    def read_properties_file(filename):
> +        if not os.path.exists(filename):

I actually think this is OK, localizations might just not have this file.

We should make that case either OK here in the code, or in the makefile logic.

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #23)
> Comment on attachment 8425419 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8425419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
> @@ +61,5 @@
> >                          help='output')
> >      opts = parser.parse_args(args)
> >  
> > +    def read_properties_file(filename):
> > +        if not os.path.exists(filename):
> 
> I actually think this is OK, localizations might just not have this file.
> 
> We should make that case either OK here in the code, or in the makefile
> logic.

My point is that for re-use (and testing, both manual and automatic) you want the lowest level function to signal error, not silently fail-but-succeed.  The fall-back mechanism is a different function, and should be in charge of falling back.

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8426261 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

I commented on a (slightly) earlier draft of this.

Comment 26

[Lucas Rocha (:lucasr)]

Attachment: Generate suggestedsites.json from region.properties (r=nalexander)

Comment 27

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #22)
> Comment on attachment 8425419 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8425419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've noted a couple of Python nits and suggested a small stream-lining, but
> overall I'm pleased with how small the change to generate*.py is.
> 
> As to = vs := in Makefiles, prefer :=, since it is "evaluated immediately"
> and tends to be easier to reason about.  I'm pretty sure you're copy-pasting
> from earlier in the file, so I don't see the need to change it.

I was actually just a typo but thanks for the explanation  :-)
 
> ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
> @@ +61,5 @@
> >                          help='output')
> >      opts = parser.parse_args(args)
> >  
> > +    def read_properties_file(filename):
> > +        if not os.path.exists(filename):
> 
> nit: have this throw if the path doesn't exist.
> 
> @@ +87,2 @@
> >              path = mozpath.join(srcdir, filename)
> > +            properties = dict(properties.items() + read_properties_file(path).items())
> 
> nit: have this be the place that catches and ignores missing files.

I wasn't keen on doing this at first but, after trying it, I think this adds clarity to the code. Done. 

> @@ +90,3 @@
> >  
> > +    def get_site_list_from_properties(properties):
> > +        prefix = 'list.'
> 
> nit: add a docstring explaining what this function does:
> 
> "Turns {'list.0':'mozilla', 'list.1':'other'} into ['mozilla', 'other']."

Done.

> @@ +97,5 @@
> > +            indexes.append(int(k[len(prefix):]))
> > +        return [properties[prefix + str(index)] for index in sorted(indexes)]
> > +
> > +    def get_site_from_properties(name, properties):
> > +        prefix = '{name}.'.format(name=name)
> 
> nit: docstring as well. Turns {'mozilla.title':'title', ...} into
> {'title':'title', ...}.

Done.

Comment 28

[Lucas Rocha (:lucasr)]

(In reply to Francesco Lodolo [:flod] from comment #21)
> Comment on attachment 8426261 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8426261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
> @@ +101,5 @@
> > +        prefix = '{name}.'.format(name=name)
> > +        try:
> > +            site = dict((k, properties[prefix + k]) for k in ('title', 'url', 'bgcolor'))
> > +        except IndexError, e:
> > +            raise Exception("Could not find property required property for '{name}: {error}'"
> 
> too many property

Oops, fixed.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8426924 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

Ok, I think this is ready for review.

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8426924 [details] [diff] [review]
Generate suggestedsites.json from region.properties (r=nalexander)

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

Looks good.

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
@@ +48,5 @@
> +    Ignores empty, comment lines, and keys not starting with the prefix for
> +    suggested sites ('browser.suggestedsites'). Removes the prefix from all
> +    matching keys i.e. turns 'browser.suggestedsites.foo' into simply 'foo'
> +    """
> +    if not os.path.exists(filename):

If you want; but I'm pretty happy just having open(...) below throw.

Comment 31

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #30)
> Comment on attachment 8426924 [details] [diff] [review]
> Generate suggestedsites.json from region.properties (r=nalexander)
> 
> Review of attachment 8426924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py
> @@ +48,5 @@
> > +    Ignores empty, comment lines, and keys not starting with the prefix for
> > +    suggested sites ('browser.suggestedsites'). Removes the prefix from all
> > +    matching keys i.e. turns 'browser.suggestedsites.foo' into simply 'foo'
> > +    """
> > +    if not os.path.exists(filename):
> 
> If you want; but I'm pretty happy just having open(...) below throw.

Done, pushed: https://hg.mozilla.org/integration/fx-team/rev/2da946c6202c

Pike & flod, please wait until bug 997765 is fixed before making a call for suggested site translations. The current list is just a placeholder.

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/2da946c6202c