833882 - en-US dictionaries and searchplugins are shipped in l10n repacks

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

Comment 1

[Mike Hommey [:glandium]]

Created attachment 705510 [details] [diff] [review]
Correctly handle non chrome directories when doing l10n-repack

This changes how l10n-repack handles non chrome directories, such that files with the same name are replaced, files only in l10n are added, and files only in the original build are removed.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 705510 [details] [diff] [review]
Correctly handle non chrome directories when doing l10n-repack

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

Just some style nits.

::: toolkit/mozapps/installer/l10n-repack.py
@@ +125,5 @@
>              for e in [e for e in f if e.localized]:
>                  f.remove(e)
> +        if p in paths:
> +            path = paths[p]
> +            if path:

path = p.paths.get(p, None)
if path:

@@ +132,2 @@
>                  files = [f for p, f in l10n_finder.find(path)]
> +                if len(files) == 0:

if not len(files)

@@ +132,3 @@
>                  files = [f for p, f in l10n_finder.find(path)]
> +                if len(files) == 0:
> +                    if not base in NON_CHROME:

if base not in NON_CHROME:

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 705510 [details] [diff] [review]
> Correctly handle non chrome directories when doing l10n-repack
> 
> Review of attachment 705510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some style nits.
> 
> ::: toolkit/mozapps/installer/l10n-repack.py
> @@ +125,5 @@
> >              for e in [e for e in f if e.localized]:
> >                  f.remove(e)
> > +        if p in paths:
> > +            path = paths[p]
> > +            if path:
> 
> path = p.paths.get(p, None)
> if path:

Note that paths[p] == None is treated differently from p not in paths.

Comment 4

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/4c8d59b07d16

Comment 5

[Mike Hommey [:glandium]]

Backed out because of bug 834432:
https://hg.mozilla.org/mozilla-central/rev/169e0492b03b

Comment 6

[Mike Hommey [:glandium]]

Created attachment 706142 [details] [diff] [review]
Correctly handle non chrome directories when doing l10n-repack

This combines the now backed out patch from this bug and the patch from bug 834432.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 706142 [details] [diff] [review]
Correctly handle non chrome directories when doing l10n-repack

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

I lied about 4 hours. I remember looking at most of this last night. Not sure why I didn't hit r+ then.

Would be really nice to have test coverage of l10n repacks. These break way too often...

Comment 8

[Mike Hommey [:glandium]]

Thanks for the review. I agree we need test coverage for these. But let's already try to fix them :)

https://hg.mozilla.org/mozilla-central/rev/52c92a6c6e24