Consider adding hyphenation dictionaries to static catalog

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: k.krish)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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 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.
Attachment #8760870 - Flags: review?(s.kaspari)
(Assignee)

Comment 3

3 years ago
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/
Attachment #8760870 - Attachment description: Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog → Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog - fixes
Attachment #8760870 - Flags: review?(s.kaspari)
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.
Attachment #8760870 - Flags: review?(s.kaspari)
(Assignee)

Updated

3 years ago
Attachment #8762049 - Attachment is obsolete: true
Attachment #8762049 - Flags: review?(s.kaspari)
(Assignee)

Updated

3 years ago
Attachment #8760870 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
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/
Attachment #8760870 - Attachment description: Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog - fixes → Bug1276589 adding hyphenation dictionaries to static catalog - temporary catalog
Attachment #8760870 - Attachment is obsolete: false
Attachment #8760870 - Flags: review?(s.kaspari)
(Assignee)

Updated

3 years ago
Attachment #8762046 - Attachment is obsolete: true
Attachment #8762046 - Flags: review?(s.kaspari)
(Assignee)

Comment 8

3 years ago
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/
Attachment #8760870 - Flags: review?(s.kaspari)
Attachment #8762136 - Flags: review?(s.kaspari)
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.
(Assignee)

Updated

3 years ago
Depends on: 1280769
(Assignee)

Updated

3 years ago
Blocks: 1285752
See bug 1276588 comment 22.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.