1276589 - Consider adding hyphenation dictionaries to static catalog

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We currently ship a static version of the downloadable content catalog in the app:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentBootstrap.java

With that we can start to download assets before synchronizing the catalog to the device. After adding the assets to the server we can consider adding them to the static catalog too.

Comment 1

[k.krish]

Attachment: Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

Review commit: https://reviewboard.mozilla.org/r/58270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58270/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8760870 [details]
Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

https://reviewboard.mozilla.org/r/58270/#review55360

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java:30
(Diff revision 1)
>      public static final String TYPE_ASSET_ARCHIVE = "asset-archive";
>  
> -    @StringDef({KIND_FONT})
> +    @StringDef({KIND_FONT, KIND_HYPHENATION_DICTIONARY})
>      public @interface Kind {}
>      public static final String KIND_FONT = "font";
> +    public static final String KIND_HYPHENATION_DICTIONARY = "hyphenation-dictionary";

Let's just use "hyphenation" as kind. This seems to be the naming pattern of the native code:
https://dxr.mozilla.org/mozilla-central/source/intl/hyphenation/glue/nsHyphenationManager.cpp

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentBootstrap.java:17
(Diff revision 1)
> -        if (!AppConstants.MOZ_ANDROID_EXCLUDE_FONTS) {
> -            // We are packaging fonts. There's nothing we want to download;
> +        if(!AppConstants.MOZ_ANDROID_EXCLUDE_FONTS && !AppConstants.MOZ_EXCLUDE_HYPHENATION_DICTIONARIES){
> +            //We are packaging both fonts and hyphenations-dictionaries. There is nothing to download;
>              return new ArrayMap<>();
>          }

I think this code is unnecessary because the following code will already return an empty map if both constants are false.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentBootstrap.java:23
(Diff revision 1)
> -            // We are packaging fonts. There's nothing we want to download;
> +            //We are packaging both fonts and hyphenations-dictionaries. There is nothing to download;
>              return new ArrayMap<>();
>          }
> +        ArrayMap<String, DownloadContent> content = new ArrayMap<>();
> +
> +        if(AppConstants.MOZ_ANDROID_EXCLUDE_FONTS){

To follow our style guidelines run:
./mach gradle checkstyle
and fix the issues in the report.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentBootstrap.java:181
(Diff revision 1)
> -        return content;
> +
> +    public static List<DownloadContent> getHyphenationDictionariesList(){
> +        return Arrays.asList(
> +                new DownloadContentBuilder()
> +                        .setId("40d68163-6abe-a792-06cd-ad68732a3788")
> +                        .setLocation("~skaspari/dlc/hyphenation/hyph_af.dic.gz")

I'll see that we get the files uploaded to their actual destination so that we can replace this here.

Comment 3

[k.krish]

Comment on attachment 8760870 [details]
Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58270/diff/1-2/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8760870 [details]
Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

https://reviewboard.mozilla.org/r/58270/#review55854

This patch also doesn't include all changes. It's only the diff between the previous patch and now.

Comment 5

[k.krish]

Attachment: Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

Review commit: https://reviewboard.mozilla.org/r/58946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58946/

Comment 6

[k.krish]

Attachment: Bug1276589 adding hyphenation dictionary to static catalog - temporary catalog - fixes

Review commit: https://reviewboard.mozilla.org/r/58948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58948/

Comment 7

[k.krish]

Comment on attachment 8760870 [details]
Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58270/diff/2-3/

Comment 8

[k.krish]

Comment on attachment 8760870 [details]
Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58270/diff/3-4/

Comment 9

[k.krish]

Attachment: Bug1276589 adding hyphenation dictionaries to static catalog

Review commit: https://reviewboard.mozilla.org/r/59014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59014/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8762136 [details]
Bug1276589 adding hyphenation dictionaries to static catalog

https://reviewboard.mozilla.org/r/59014/#review56560

Great. Let's split this patch. We can land the patch to add the hyphenation dictionary kind as soon as possible (just file a new bug and add a patch) and then update the catalog later.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java:27
(Diff revision 1)
> -    @StringDef({KIND_FONT})
> +    @StringDef({KIND_FONT, KIND_HYPHENATION_DICTIONARY})
>      public @interface Kind {}
>      public static final String KIND_FONT = "font";
> +    public static final String KIND_HYPHENATION_DICTIONARY = "hyphenation";

Maye let's move this and isHyphenationDictionary() to a separate patch. I'm not sure yet if we want to land the patch updating the static catalog. I think we could just rely on the online version here and start downloading the dictionaries after we synchronized the catalog the first time.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

See bug 1276588 comment 22.