Issue 5751 - Removing legacy gecko support

Issue 5751 - Removing legacy gecko support

[tlucas, 2017-10-02 10:38:57]

Patch Set 1

* Removing gecko build targets
* Removing support for gecko-style translations

Please note that this was implemented on top of
https://issues.adblockplus.org/ticket/5763

[tlucas, 2017-10-02 10:43:32]

Adding Wladimir to CC.

[Sebastian Noack, 2017-10-03 02:22:40]

LocaleTester.pm can also go away. It is only useful to check legacy Gecko
translation files.

https://codereview.adblockplus.org/29562599/diff/29562600/build.py
File build.py (left):

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#oldcode262
build.py:262: 'name_format': 'ISO-15897',
This information is no longer relevant. We should remove it and just assume
ISO-15897 everywhere.

https://codereview.adblockplus.org/29562599/diff/29562600/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#newcode183
build.py:183: no_gecko_build = type != 'gecko-webext'
I guess we don't need this variable anymore to avoid exceeding 79 chars.

https://codereview.adblockplus.org/29562599/diff/29562600/localeTools.py
File localeTools.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/localeTools.py#newc...
localeTools.py:188: if path.endswith('.properties'):
Both, the .dtd and the .properties format are specific to legacy Gecko
extensions and can be removed now.

https://codereview.adblockplus.org/29562599/diff/29562600/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/releaseAutomation.p...
releaseAutomation.py:15: from localeTools import readFile
Why did you add this import?

[Sebastian Noack, 2017-10-04 01:39:33]

https://codereview.adblockplus.org/29562599/diff/29562600/build.py
File build.py (left):

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#oldcode262
build.py:262: 'name_format': 'ISO-15897',
On 2017/10/03 02:22:39, Sebastian Noack wrote:
> This information is no longer relevant. We should remove it and just assume
> ISO-15897 everywhere.

Actually adblockplusui is using BCP-47, but it would make much more sense to
just change this (all it takes is replacing underscores with dashes in the
filenames under locales/*), than keeping up that inconsistency, now where legacy
Firefox extensions are gone.

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#oldcode263
build.py:263: 'file_format': 'chrome-json',
file_format is irrelevant as well, if you drop support for the legacy Gecko
translation formats.

[tlucas, 2017-10-04 11:48:41]

Patch Set 2

* Rebasing against #5763
* Removing .properties files related code
* Removing LocaleTester.pm
* Addressing Sebastians comments

Please note, this code is rebase against
https://codereview.adblockplus.org/29561557/ @ Patch Set 4

https://codereview.adblockplus.org/29562599/diff/29562600/build.py
File build.py (left):

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#oldcode262
build.py:262: 'name_format': 'ISO-15897',
On 2017/10/04 01:39:33, Sebastian Noack wrote:
> On 2017/10/03 02:22:39, Sebastian Noack wrote:
> > This information is no longer relevant. We should remove it and just assume
> > ISO-15897 everywhere.
> 
> Actually adblockplusui is using BCP-47, but it would make much more sense to
> just change this (all it takes is replacing underscores with dashes in the
> filenames under locales/*), than keeping up that inconsistency, now where
legacy
> Firefox extensions are gone. (underscores already get replaced below)

Done.

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#oldcode263
build.py:263: 'file_format': 'chrome-json',
On 2017/10/04 01:39:33, Sebastian Noack wrote:
> file_format is irrelevant as well, if you drop support for the legacy Gecko
> translation formats.

Done.

https://codereview.adblockplus.org/29562599/diff/29562600/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/build.py#newcode183
build.py:183: no_gecko_build = type != 'gecko-webext'
On 2017/10/03 02:22:39, Sebastian Noack wrote:
> I guess we don't need this variable anymore to avoid exceeding 79 chars.

Done.

https://codereview.adblockplus.org/29562599/diff/29562600/localeTools.py
File localeTools.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/localeTools.py#newc...
localeTools.py:188: if path.endswith('.properties'):
On 2017/10/03 02:22:39, Sebastian Noack wrote:
> Both, the .dtd and the .properties format are specific to legacy Gecko
> extensions and can be removed now.

Done.

https://codereview.adblockplus.org/29562599/diff/29562600/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29562599/diff/29562600/releaseAutomation.p...
releaseAutomation.py:15: from localeTools import readFile
On 2017/10/03 02:22:39, Sebastian Noack wrote:
> Why did you add this import?

This was not intentional, removed it.

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py
File localeTools.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py#newc...
localeTools.py:21: CROWDIN_LANG_MAPPING = {
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py#newc...
localeTools.py:138: crowdin_request(projectName, 'supported-languages', key)}
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py#newc...
localeTools.py:142: locales = sorted(locales & allowed)
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py#newc...
localeTools.py:231: language = CROWDIN_LANG_MAPPING.get(locale, locale)
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/localeTools.py#newc...
localeTools.py:266: for key, value in CROWDIN_LANG_MAPPING.iteritems():
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py#n...
packagerChrome.py:205: for chrome_locale, crowdin_locale in
localeTools.CROWDIN_LANG_MAPPING.iteritems():
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py#n...
packagerChrome.py:210: for gecko_locale, crowdin_locale in
localeTools.CROWDIN_LANG_MAPPING.iteritems():
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py#n...
packagerChrome.py:268: def fix_translations_for_chrome(files):
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py#n...
packagerChrome.py:305: files['_locales/es_419/' + filename] = data
Note: result of rebasing

https://codereview.adblockplus.org/29562599/diff/29564580/packagerChrome.py#n...
packagerChrome.py:385: fix_translations_for_chrome(files)
Note: result of rebasing

[Sebastian Noack, 2017-10-04 22:22:18]

https://codereview.adblockplus.org/29562599/diff/29564580/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/packager.py#newcode6
packager.py:6: # packagers are implemented in packagerEdge and packagerChrome.
Nit: We usually list Chrome before Edge everywhere in our code, and
documentation, etc., as it is the more relent platform.

https://codereview.adblockplus.org/29562599/diff/29564580/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/releaseAutomation.p...
releaseAutomation.py:145: with open(os.path.join(locales_base, 'chrome',
'locale',
This will no longer work since the "adblockplus" depdendeny in
"adblockpluschrome" is gone, now. However, the "name" string has been moved to
"adblockpluschrome/_locales/*/messages.json", and we can just get it from there.

[Sebastian Noack, 2017-10-04 22:33:00]

https://codereview.adblockplus.org/29562599/diff/29564580/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/build.py#newcode222
build.py:222: if type in {'chrome', 'gecko-webext'}:
I think we can make all the translation commands (i.e.
setuptrans/translate/uploadtrans/gettranslations) agnostic of the release
target, now. All it seems to take would be changing the check here to `type !=
'generic'` and remove the `supportedTypes` property from these commands.

[tlucas, 2017-10-05 10:10:31]

Patch set 3

* Rebasing against https://codereview.adblockplus.org/29561557/ @ Patch Set 5
* Reading extension Name from _locales/<defaultLocale>/messages.json
* Make locale related commands agnostic of platform
* Addressing nits

https://codereview.adblockplus.org/29562599/diff/29564580/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/build.py#newcode222
build.py:222: if type in {'chrome', 'gecko-webext'}:
On 2017/10/04 22:33:00, Sebastian Noack wrote:
> I think we can make all the translation commands (i.e.
> setuptrans/translate/uploadtrans/gettranslations) agnostic of the release
> target, now. All it seems to take would be changing the check here to `type !=
> 'generic'` and remove the `supportedTypes` property from these commands.

Good idea, would clean things up a bit more. Done.

https://codereview.adblockplus.org/29562599/diff/29564580/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/packager.py#newcode6
packager.py:6: # packagers are implemented in packagerEdge and packagerChrome.
On 2017/10/04 22:22:18, Sebastian Noack wrote:
> Nit: We usually list Chrome before Edge everywhere in our code, and
> documentation, etc., as it is the more relent platform.

Done.

https://codereview.adblockplus.org/29562599/diff/29564580/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29562599/diff/29564580/releaseAutomation.p...
releaseAutomation.py:145: with open(os.path.join(locales_base, 'chrome',
'locale',
On 2017/10/04 22:22:18, Sebastian Noack wrote:
> This will no longer work since the "adblockplus" depdendeny in
> "adblockpluschrome" is gone, now. However, the "name" string has been moved to
> "adblockpluschrome/_locales/*/messages.json", and we can just get it from
there.

Done.

https://codereview.adblockplus.org/29562599/diff/29565645/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29565645/build.py#newcode301
build.py:301: if locale != localeConfig['default_locale'].replace('_', '-'):
Note: result of rebasing

[Vasily Kuznetsov, 2017-10-05 20:58:12]

Hi Tristan,

Another annoying nit about strings vs. tuples from me.

Cheers,
Vasily

https://codereview.adblockplus.org/29562599/diff/29565645/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29565645/build.py#newcode430
build.py:430: command.supportedTypes = ('chrome')
Even though we did have strings instead of tuples in `command.supportedTypes`
before, it seems like this breaks the logic of `Command.isSupported`. When
`supportedTypes` is a string, we look up a substring in a string instead of
element in a tuple.

Even though this wouldn't break anything in real life usage at this time, I
would still advise to keep the type of `command.supportedTypes` always a tuple,
just to keep the code cleaner and avoid bugs being introduced in the future.

[tlucas, 2017-10-06 09:06:50]

Patch Set 5

 * Fixed string / tuple mix-up
 * Cleaned up tox.ini
 * Removed redundant key modifications in import_locales

https://codereview.adblockplus.org/29562599/diff/29565645/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29565645/build.py#newcode430
build.py:430: command.supportedTypes = ('chrome')
On 2017/10/05 20:58:11, Vasily Kuznetsov wrote:
> Even though we did have strings instead of tuples in `command.supportedTypes`
> before, it seems like this breaks the logic of `Command.isSupported`. When
> `supportedTypes` is a string, we look up a substring in a string instead of
> element in a tuple.
> 
> Even though this wouldn't break anything in real life usage at this time, I
> would still advise to keep the type of `command.supportedTypes` always a
tuple,
> just to keep the code cleaner and avoid bugs being introduced in the future.

Done.

https://codereview.adblockplus.org/29562599/diff/29567598/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29567598/build.py#newcode386
build.py:386: 
This line would have been the only one to have to ignore an E305.

[Sebastian Noack, 2017-10-06 19:13:29]

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
File packagerChrome.py (left):

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#o...
packagerChrome.py:252: noMangling = True
Since you are unsupporting the mangle feature, it seems "=*" will be equivalent
to "*", and "=string" to "string", right? If so, we can get rid of the syntax
with the leading equals sign all together, and just adapt the metadata.* files
in adblockpluschrome, once these changes get integrated. It wouldn't be the only
thing we have to adapt for, anyway. Feel free to document any further required
adaptions in https://issues.adblockplus.org/ticket/5837.

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#n...
packagerChrome.py:199: locale = sourceFile.split(os.path.sep)[-2].replace('-',
'_')
As pointed out on the review introducing this code, replacing dashes with
underscores are no longer necessary, once all paths are assumed to use ISO
format (i.e. underscored) for language codes.

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#n...
packagerChrome.py:221: key = re.sub(r'\W', '_', stringID)
This should no longer be necessary either, as we will only import strings that
are already in the same format, now.

https://codereview.adblockplus.org/29562599/diff/29567598/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29562599/diff/29567598/tox.ini#newcode13
tox.ini:13: packagerSafari.py : A107,A302,E501,N802,N803,N806
This seems unrelated, as this change doesn't touch packagerSafari.py, and we
will remove that script all together right after this change landed, anyway.

[tlucas, 2017-10-09 23:18:09]

Patch Set 6

* Removed unrelated changes in tox.ini
* Removed some wildcard import functionality

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
File packagerChrome.py (left):

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#o...
packagerChrome.py:252: noMangling = True
On 2017/10/06 19:13:28, Sebastian Noack wrote:
> Since you are unsupporting the mangle feature, it seems "=*" will be
equivalent
> to "*", and "=string" to "string", right? If so, we can get rid of the syntax
> with the leading equals sign all together, and just adapt the metadata.* files
> in adblockpluschrome, once these changes get integrated. It wouldn't be the
only
> thing we have to adapt for, anyway. Feel free to document any further required
> adaptions in https://issues.adblockplus.org/ticket/5837.

Done. I also adjusted the content in #5837 - feel free to proofread it.

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#n...
packagerChrome.py:199: locale = sourceFile.split(os.path.sep)[-2].replace('-',
'_')
On 2017/10/06 19:13:28, Sebastian Noack wrote:
> As pointed out on the review introducing this code, replacing dashes with
> underscores are no longer necessary, once all paths are assumed to use ISO
> format (i.e. underscored) for language codes.

Done.

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#n...
packagerChrome.py:221: key = re.sub(r'\W', '_', stringID)
On 2017/10/06 19:13:29, Sebastian Noack wrote:
> This should no longer be necessary either, as we will only import strings that
> are already in the same format, now.

A quick test verified your assumption, this line has no effect for all targets -
removed it.

https://codereview.adblockplus.org/29562599/diff/29567598/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29562599/diff/29567598/tox.ini#newcode13
tox.ini:13: packagerSafari.py : A107,A302,E501,N802,N803,N806
On 2017/10/06 19:13:29, Sebastian Noack wrote:
> This seems unrelated, as this change doesn't touch packagerSafari.py, and we
> will remove that script all together right after this change landed, anyway.

Well, those were discovered as "not needed" anymore - but i don't mind too much,
and you are right about them being removed with 5752 anyway. Undid this change.

[Sebastian Noack, 2017-10-10 04:51:41]

LGTM

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
File packagerChrome.py (left):

https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#o...
packagerChrome.py:252: noMangling = True
On 2017/10/09 23:18:07, tlucas wrote:
> On 2017/10/06 19:13:28, Sebastian Noack wrote:
> > Since you are unsupporting the mangle feature, it seems "=*" will be
> equivalent
> > to "*", and "=string" to "string", right? If so, we can get rid of the
syntax
> > with the leading equals sign all together, and just adapt the metadata.*
files
> > in adblockpluschrome, once these changes get integrated. It wouldn't be the
> only
> > thing we have to adapt for, anyway. Feel free to document any further
required
> > adaptions in https://issues.adblockplus.org/ticket/5837.
> 
> Done. I also adjusted the content in #5837 - feel free to proofread it.

I don't see any change to the description of #5837 by anybody but me. Did you
submit your changes?

[tlucas, 2017-10-10 08:47:24]

On 2017/10/10 04:51:41, Sebastian Noack wrote:
> LGTM
> 
> https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py
> File packagerChrome.py (left):
> 
>
https://codereview.adblockplus.org/29562599/diff/29567598/packagerChrome.py#o...
> packagerChrome.py:252: noMangling = True
> On 2017/10/09 23:18:07, tlucas wrote:
> > On 2017/10/06 19:13:28, Sebastian Noack wrote:
> > > Since you are unsupporting the mangle feature, it seems "=*" will be
> > equivalent
> > > to "*", and "=string" to "string", right? If so, we can get rid of the
> syntax
> > > with the leading equals sign all together, and just adapt the metadata.*
> files
> > > in adblockpluschrome, once these changes get integrated. It wouldn't be
the
> > only
> > > thing we have to adapt for, anyway. Feel free to document any further
> required
> > > adaptions in https://issues.adblockplus.org/ticket/5837.
> > 
> > Done. I also adjusted the content in #5837 - feel free to proofread it.
> 
> I don't see any change to the description of #5837 by anybody but me. Did you
> submit your changes?

Well - i hit "submit" and left the window tho publish the comments on this
review. Turns out it didn't submit, but i did it now :)

[Vasily Kuznetsov, 2017-10-10 12:44:33]

LGTM

https://codereview.adblockplus.org/29562599/diff/29572582/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29572582/build.py#newcode221
build.py:221: if type != 'generic':
We could swap the branches of this if statement to get a more natural looking:

    if type == 'generic':
        ...
    else:
        ...

[tlucas, 2017-10-10 12:50:13]

https://codereview.adblockplus.org/29562599/diff/29572582/build.py
File build.py (right):

https://codereview.adblockplus.org/29562599/diff/29572582/build.py#newcode221
build.py:221: if type != 'generic':
On 2017/10/10 12:44:33, Vasily Kuznetsov wrote:
> We could swap the branches of this if statement to get a more natural looking:
> 
>     if type == 'generic':
>         ...
>     else:
>         ...

Well, imho the "not generic" platforms are those most interesting to us - so the
order is quite natural i'd say ;)

[Vasily Kuznetsov, 2017-10-10 13:02:52]

On 2017/10/10 12:50:13, tlucas wrote:
> https://codereview.adblockplus.org/29562599/diff/29572582/build.py
> File build.py (right):
> 
> https://codereview.adblockplus.org/29562599/diff/29572582/build.py#newcode221
> build.py:221: if type != 'generic':
> On 2017/10/10 12:44:33, Vasily Kuznetsov wrote:
> > We could swap the branches of this if statement to get a more natural
looking:
> > 
> >     if type == 'generic':
> >         ...
> >     else:
> >         ...
> 
> Well, imho the "not generic" platforms are those most interesting to us - so
the
> order is quite natural i'd say ;)

Ok, if this makes more sense to you, you're probably right.