[gofaster] Pocket update for fx50, including strings

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Depends on: 1 bug)

50 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Pocket will be doing an update destined for Fx50.  

First issue:

The update includes some string changes in their ui (Hugo can give details).  I want to be sure that, if Pocket handles the translations, we can uplift through to Beta.
(Assignee)

Comment 1

2 years ago
re the strings from comment #0
Flags: needinfo?(francesco.lodolo)
Can you clarify for which locales Pocket would handle the localization?

Pocket's code currently lives in mozilla-central, strings should ride the trains, not jump them. If you want to add these strings in Beta for Firefox 50, someone would need to land them in all affected l10n Beta and Aurora repositories.
Depending on the locale (and the tool they use), we might simply lose these strings after we land them in the repository, to avoid that someone would need to enter these strings manually. Our infrastructure currently doesn't support that scenario.

If Pocket needs a faster pace, it should not have strings in paths exposed to l10n tools and processes, and use a process similar to the one used by Hello:
* Use an external GitHub repository for code and one for strings.
* Include only relevant locales. 
* Expose strings in Pontoon for localization. That would also solve the problem of letting localizers review these translations.
* Dump code and strings into mozilla-beta as needed (outside of paths covered by l10n tools) 

Note that you'll still have to solve the problem of dealing with incomplete locales (right now it's managed by compare-locales at build time).

There's currently an ongoing discussion about creating a template for system add-ons, and we plan to include best practices on how to manage localization very soon.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 3

2 years ago
@flod, IIUC, our tools will not update aurora/beta so all those l10n branches would need to be manually updated.  Seems like a lot of effort.  

I have one github project that includes the locale files in the project rather than a separate repo, and it's hooked into pontoon.  Any reason we cant do that for pocket as well, or do you really want a separate repo?

For a recent gofaster release we had to include pocket (though there were no changes), which also meant we had to gather all the l10n files into the addon.  This may be the simplest path forward, if we made the change to the pocket addon to include all the locales in the addon I'm guessing that will fix the uplift issue.  In that case, I think we can leave out incomplete locales.  We did that for the gofaster release.
Flags: needinfo?(francesco.lodolo)
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> @flod, IIUC, our tools will not update aurora/beta so all those l10n
> branches would need to be manually updated.  Seems like a lot of effort.  
Exactly. Changes need to land manually in both repos, for each locale.

> I have one github project that includes the locale files in the project
> rather than a separate repo, and it's hooked into pontoon.  Any reason we
> cant do that for pocket as well, or do you really want a separate repo?
Some developers complained about the history becoming not relevant, since Pontoon commits frequently.
Are we still talking about Pocket? Because IIRC currently there's nothing hooked on Pontoon.

> For a recent gofaster release we had to include pocket (though there were no
> changes), which also meant we had to gather all the l10n files into the
> addon.  This may be the simplest path forward, if we made the change to the
> pocket addon to include all the locales in the addon I'm guessing that will
> fix the uplift issue.  In that case, I think we can leave out incomplete
> locales.  We did that for the gofaster release.
Yes, as long as we do in a safer way (last time we counted the number of lines, instead of parsing the files).
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 5

2 years ago
The other project I mentioned is here:
https://pontoon.mozilla.org/projects/social-api-directory/
https://github.com/mixedpuppy/socialapi-directory/

I'm thinking that I would do the same for the pocket addon.

ok, I understand why a separate repo for pontoon would be better if it's a project that has lots of string changes.  Probably a good idea to do that for pocket as well.
(Assignee)

Comment 6

2 years ago
flod: I have an initial import of locales into https://github.com/mozilla-partners/pocket-l10n
 What do we need to do to add to pontoon?
Flags: needinfo?(francesco.lodolo)
I don't have access to that repository apparently. 

Can we move it to the mozilla-l10n organization? That way we can manage permissions, including adding access to Pontoon and developers as needed.

I'd also need some information like deadline, list of locales to include.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 8

2 years ago
:flod for this update I'd like to land and uplift to aurora before the new merge date (Sept 19).  Pocket is changing some strings, and will provide translations for some locales.  I may have to add fall-back support to the build script.

of note to be sure I understand: 
- all locales for pocket will now exist in mozilla-central under browser/extensions/pocket/locales
- those will not be edited other than through pontoon and pocket-l10n repo

I would land this in two parts:
1. the changes to the addon to have the locale files all present
2. new changes from pocket
As explained on IRC, the point of using an external repository for strings is to avoid breaking string freeze in Aurora and Beta.

Hello used to dump code and strings in mozilla-beta, but outside of paths covered by l10n tools and dashboards. I suggest to do the same, and remove strings from mozilla-central (and let that change ride the trains).

See bug 1285974 for an example.
Repo is in the mozilla-l10n org
https://github.com/mozilla-l10n/pocket-l10n/

Project is set up in Pontoon, strings should be available after next sync
https://pontoon.mozilla.org/admin/projects/pocket/

What's the deadline to complete the localization?
(In reply to Francesco Lodolo [:flod] from comment #10)
> Project is set up in Pontoon, strings should be available after next sync
> https://pontoon.mozilla.org/admin/projects/pocket/

Correction: the project will be available after the first sync at 
https://pontoon.mozilla.org/projects/pocket/
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8790878 [details]
Bug 1301442 m-c removal of old pocket l10n,

https://reviewboard.mozilla.org/r/78490/#review77046

Looks good to me for mozilla-central.

I was thinking of keeping the lines in filter.py and l10n.ini to make sure that localizers remove those strings from their repositories, but we could remove them as part of merge day uplifts.
https://wiki.mozilla.org/L10n:Migration#Merge
Attachment #8790878 - Flags: review?(francesco.lodolo) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8790879 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78492/#review77048

l10n seems correct to me, but I would suggest to have someone with more code experience to review the whole patch (standard8 sounds like the best choice)

::: browser/extensions/pocket/moz.build:10
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
>  DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']
>  
> -DIRS += ['locales']
> +DIRS += ['locale']

The GitHub repository is using "locales". Are you planning to change the folder when uploading?
https://github.com/mozilla-l10n/pocket-l10n
Attachment #8790879 - Flags: review?(francesco.lodolo)
Assignee: nobody → mixedpuppy
(Assignee)

Comment 17

2 years ago
(In reply to Francesco Lodolo [:flod] from comment #16)
> Comment on attachment 8790879 [details]
> Bug 1301442 new layout for external l10n build support,
> 
> https://reviewboard.mozilla.org/r/78492/#review77048
> 
> l10n seems correct to me, but I would suggest to have someone with more code
> experience to review the whole patch (standard8 sounds like the best choice)
> 
> ::: browser/extensions/pocket/moz.build:10
> (Diff revision 1)
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> >  DEFINES['MOZ_APP_MAXVERSION'] = CONFIG['MOZ_APP_MAXVERSION']
> >  
> > -DIRS += ['locales']
> > +DIRS += ['locale']
> 
> The GitHub repository is using "locales". Are you planning to change the
> folder when uploading?
> https://github.com/mozilla-l10n/pocket-l10n

The Makefile in the pocket github repo actually moves the files into "locale".  I wanted it to be different to avoid any issues/conflict with the existing l10n files (which wont be removed in aurora).
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8790879 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78492/#review77048

Standard8 is out, I'll ping Gijs
(Assignee)

Updated

2 years ago
Attachment #8790879 - Flags: review?(gijskruitbosch+bugs)
(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> The Makefile in the pocket github repo actually moves the files into
> "locale".  I wanted it to be different to avoid any issues/conflict with the
> existing l10n files (which wont be removed in aurora).

Having a different path is needed (otherwise you'd be exposing strings to localization in aurora and beta), so that's perfect, just wanted to make sure the l10n repo structure was taken into account.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8790879 - Attachment is obsolete: true
Attachment #8790879 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78558/#review77226

I took a glance at this one (and the other), adding my thoughts here. More as food for thought for Gijs' review

::: browser/extensions/pocket/locale/jar.mn:19
(Diff revision 1)
> +  # For other locales (and en-US) fallback to the en-US directory.
> +#if AB_CD == en_US
> +  locale/@AB_CD@/                (@AB_CD@/*)
> +#else
> +  locale/@AB_CD@/                (en-US/*)
> +#endif

Not sure about this.

My take would be that we should list all locales here individually in jar.mn that we landed in-tree.

That way, all development builds end up with the same multi-locale add-on version of pocket.

Also, this shouldn't go into the @AB_CD@.jar, but in chrome.jar, as is the rest of pocket.

::: browser/locales/Makefile.in:108
(Diff revision 1)
>  libs-%:
>  	$(NSINSTALL) -D $(DIST)/install
>  	@$(MAKE) -C ../../toolkit/locales libs-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
>  	@$(MAKE) -C ../../services/sync/locales AB_CD=$* XPI_NAME=locale-$*
>  	@$(MAKE) -C ../../extensions/spellcheck/locales AB_CD=$* XPI_NAME=locale-$*
> -	@$(MAKE) -C ../extensions/pocket/locales AB_CD=$* XPI_NAME=locale-$*
> +	@$(MAKE) -C ../extensions/pocket/locale AB_CD=$* XPI_NAME=locale-$*

I think this line should just be removed.

Comment 25

2 years ago
mozreview-review
Comment on attachment 8790878 [details]
Bug 1301442 m-c removal of old pocket l10n,

https://reviewboard.mozilla.org/r/78490/#review77228

::: browser/locales/l10n.ini
(Diff revision 3)
>  dirs = browser
>       extensions/reporter
>       other-licenses/branding/firefox
>       browser/branding/official
>       devtools/client
> -     browser/extensions/pocket

I think we should keep both lines here and in filter.py to remove the out-dated files as they ride the trains.

We should file a follow-up to make sure that happened, and then remove both extensions/reporter and browser/extensions/pocket. I'd nominate flod for that (again, did so in person before).

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78558/#review77226

> Not sure about this.
> 
> My take would be that we should list all locales here individually in jar.mn that we landed in-tree.
> 
> That way, all development builds end up with the same multi-locale add-on version of pocket.
> 
> Also, this shouldn't go into the @AB_CD@.jar, but in chrome.jar, as is the rest of pocket.

When we did this for Hello, glandium specifically requested that we only ship the relevant locale, I assume to avoid unnecessary bloat. Even though it meant extra work. If we want to change that, then we should discuss with him/build peers.



Note: this should be an or-ed list of locales: `#if AB_CD == af || AB_CD == ar || ...`

> I think this line should just be removed.

This is needed, as otherwise the add-on doesn't get repacked properly - it definitely breaks things without it.
(In reply to Mark Banner (:standard8) from comment #27)
> I think we should keep both lines here and in filter.py to remove the
> out-dated files as they ride the trains.
> 
> We should file a follow-up to make sure that happened, and then remove both
> extensions/reporter and browser/extensions/pocket. I'd nominate flod for
> that (again, did so in person before).

Can't we do this as part of next merge day cleanups?
https://wiki.mozilla.org/L10n:Migration#Merge

My feeling is that, in the past, we didn't use to do that as frequently as we do.

Filed bug 1302679 in the meantime.

Comment 29

2 years ago
mozreview-review
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78558/#review77242

The comment in the jar.mn file and/or the if statement needs work. It doesn't make any sense to me.

It'd probably be better for someone like glandium to review the build changes here, and reviewing the new .properties files also seems pointless (did flod do that already? the l10n-r seems to imply that...). I would assume compare-locales or whatever it's called will still yelp if you've made a copy-paste mistake there somewhere.

But hey, I found a thing in the bootstrap.js, so all good. Next review from glandium with the bootstrap.js thing fixed, and otherwise I'll just kibitz from the sidelines...

::: browser/extensions/pocket/bootstrap.js:135
(Diff revision 1)
> +  try {
> +    SocialService = Cu.import("resource://gre/modules/SocialService.jsm", {}).SocialService;
> +  } catch (e) {
> +    // For Firefox 51+
> +    SocialService = Cu.import("resource:///modules/SocialService.jsm", {}).SocialService;
> +  }

Nit: Backwards rather than forwards compatibility, so do the 51+ thing in the try and the other thing in the catch.

::: browser/extensions/pocket/install.rdf.in:23
(Diff revision 1)
>      <!-- Target Application this theme can install into,
>          with minimum and maximum supported versions. -->
>      <em:targetApplication>
>        <Description>
>          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> -        <em:minVersion>@MOZ_APP_VERSION@</em:minVersion>
> +        <em:minVersion>48.0</em:minVersion>

Why remove this define?

::: browser/extensions/pocket/locale/jar.mn:10
(Diff revision 1)
> +[features/firefox@getpocket.com] @AB_CD@.jar:
> +% locale pocket @AB_CD@ %locale/@AB_CD@/
> +  # For locales we support, include the file from the locale's directory in the
> +  # source tree.
> +  # For other locales (and en-US) fallback to the en-US directory.
> +#if AB_CD == en_US
> +  locale/@AB_CD@/                (@AB_CD@/*)
> +#else
> +  locale/@AB_CD@/                (en-US/*)

Concur that this should be bundled in the same package (chrome.jar + pocket.xpi) with 1 locale.

I don't understand this if statement.

If the locale is en-US, package en-US, otherwise... package en-US?

It doesn't help that there's two types of substitution going on here. Is there really no clearer way to write what's happening here?
Attachment #8790985 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 30

2 years ago
(In reply to :Gijs Kruitbosch from comment #29)
> Comment on attachment 8790985 [details]
> Bug 1301442 new layout for external l10n build support,
> 
> https://reviewboard.mozilla.org/r/78558/#review77242
> 
> The comment in the jar.mn file and/or the if statement needs work. It
> doesn't make any sense to me.

When I reorganized some of the addon build, I broke a path in the script that updated that file.  Next review push will have that fixed.  Essentially, this jar file is a duplicate of what hello did.  

> It'd probably be better for someone like glandium to review the build
> changes here, and reviewing the new .properties files also seems pointless
> (did flod do that already? the l10n-r seems to imply that...). I would
> assume compare-locales or whatever it's called will still yelp if you've
> made a copy-paste mistake there somewhere.

Yeah, flod has looked at all the l10n stuff.  compare-locales isn't used in this, but there is a similar bit of functionality to decide which locale files to include.

> But hey, I found a thing in the bootstrap.js, so all good. Next review from
> glandium with the bootstrap.js thing fixed, and otherwise I'll just kibitz
> from the sidelines...

ok.

> ::: browser/extensions/pocket/install.rdf.in:23
> (Diff revision 1)
> >      <!-- Target Application this theme can install into,
> >          with minimum and maximum supported versions. -->
> >      <em:targetApplication>
> >        <Description>
> >          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> > -        <em:minVersion>@MOZ_APP_VERSION@</em:minVersion>
> > +        <em:minVersion>48.0</em:minVersion>
> 
> Why remove this define?

The addon is being locked into a min version, but I could probably revert that and do whatn I did for maxver in local builds of the addon.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

2 years ago
mozreview-review
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78558/#review77338

::: browser/extensions/pocket/locale/jar.mn:35
(Diff revision 2)
> +[features/firefox@getpocket.com] @AB_CD@.jar:
> +% locale pocket @AB_CD@ %locale/@AB_CD@/
> +  # For locales we support, include the file from the locale's directory in the
> +  # source tree.
> +  # For other locales (and en-US) fallback to the en-US directory.
> +#if AB_CD == ar || AB_CD == ast || AB_CD == bg || AB_CD == bn_BD || AB_CD == br || AB_CD == bs || AB_CD == ca || AB_CD == cs || AB_CD == cy || AB_CD == da || AB_CD == de || AB_CD == en_GB || AB_CD == en_US || AB_CD == en_ZA || AB_CD == eo || AB_CD == es_AR || AB_CD == es_CL || AB_CD == es_ES || AB_CD == es_MX || AB_CD == et || AB_CD == eu || AB_CD == fa || AB_CD == fi || AB_CD == fr || AB_CD == fy_NL || AB_CD == ga_IE || AB_CD == gd || AB_CD == hi_IN || AB_CD == hr || AB_CD == hu || AB_CD == hy_AM || AB_CD == id || AB_CD == is || AB_CD == it || AB_CD == ja || AB_CD == ja_JP_mac || AB_CD == ka || AB_CD == kk || AB_CD == km || AB_CD == kn || AB_CD == ko || AB_CD == lt || AB_CD == lv || AB_CD == mr || AB_CD == my || AB_CD == nb_NO || AB_CD == ne_NP || AB_CD == nl || AB_CD == nn_NO || AB_CD == pa_IN || AB_CD == pt_BR || AB_CD == rm || AB_CD == ro || AB_CD == ru || AB_CD == sk || AB_CD == sl || AB_CD == son || AB_CD == sq || AB_CD == sr || AB_CD == sv_SE || AB_CD == te || AB_CD == th || AB_CD == tr || AB_CD == uk || AB_CD == xh || AB_CD == zh_CN || AB_CD == zh_TW

/me cries

I take it the preprocessor we're using here doesn't have some kind of "is X in list Y" check we could use instead? :-(
Attachment #8790985 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Francesco Lodolo [:flod] from comment #15)
> Comment on attachment 8790878 [details]
> Bug 1301442 m-c removal of old pocket l10n,
> 
> https://reviewboard.mozilla.org/r/78490/#review77046
> 
> Looks good to me for mozilla-central.
> 
> I was thinking of keeping the lines in filter.py and l10n.ini to make sure
> that localizers remove those strings from their repositories, but we could
> remove them as part of merge day uplifts.
> https://wiki.mozilla.org/L10n:Migration#Merge

Looks like you lost the r+ on this patch, but after talking with Pike, it's better to drop the changes to l10n.ini and filter.py, and address them later in bug 1302679.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

2 years ago
Ok, changes to l10n.ini and filter.py have been removed from the patch.  I'll consider it still r+
(In reply to Shane Caraveo (:mixedpuppy) from comment #37)
> Ok, changes to l10n.ini and filter.py have been removed from the patch. 
> I'll consider it still r+

Confirmed, thanks.

Comment 39

2 years ago
mozreview-review
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

https://reviewboard.mozilla.org/r/78558/#review77704
Attachment #8790985 - Flags: review?(mh+mozilla) → review+

Comment 40

2 years ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0554a9b821e6
m-c removal of old pocket l10n, r=flod
https://hg.mozilla.org/integration/autoland/rev/367e18f42d59
new layout for external l10n build support, r=Gijs,glandium
(Assignee)

Comment 41

2 years ago
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

Approval Request Comment
[Feature/regressing bug #]:move l10n to github/pontoon
[User impact if declined]:none, prevents release of updates by pocket
[Describe test coverage new/current, TreeHerder]:none
[Risks and why]: low, primarily build changes
[String/UUID change made/needed]:l10n updates are moved out of m-c process to allow uplift of string updates

A followup patch from pocket with actual functional updates with string changes is waiting in the wings, we want to get these changes into fx50.  

In order to do that the build process for the pocket addon had to be changed.  This patch updates the pocket addon so that locale files are distributed with the addon, rather than in the tree.  It is almost (if not) identical to how hello worked.  I'd like to get this uplifted to aurora in time for merge to avoid a larger beta uplift.
Attachment #8790985 - Flags: approval-mozilla-aurora?
(In reply to Shane Caraveo (:mixedpuppy) from comment #41)
> [String/UUID change made/needed]:l10n updates are moved out of m-c process
> to allow uplift of string updates

To clarify: even if you see .properties files in this patch, this is not going to affect l10n, and it's OK to uplift in aurora.

Comment 43

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0554a9b821e6
https://hg.mozilla.org/mozilla-central/rev/367e18f42d59
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
status-firefox50: --- → affected
Comment on attachment 8790985 [details]
Bug 1301442 new layout for external l10n build support,

This is a huge change but something planned as a pocket update in Fx50, let's uplift to Aurora.
Attachment #8790985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Only one patch was supposed to land in Aurora! 

So, on merge day we now landed a patch removing strings that triggered all localization tools, dropping Pocket's strings, and triggering runs for all locales in the l10n dashboard. And that's too late to fix it, given it involves 102 locales.

@mixedpuppy
At this point I have no idea what's going to ship for Pocket in Aurora, and later this week in Beta. I'll let you figure it out.
(Assignee)

Updated

2 years ago
Blocks: 1304142
Depends on: 1308850

Updated

2 years ago
See Also: → bug 1315153
You need to log in before you can comment on or make changes to this bug.