1280769 - Add Hyphenation Dictionary as separate kind in downloadable content

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[k.krish]

As a part of Bug 1095719, add "KIND_HYPHENATION_DICTIONARY" as a separate kind in Downloadable Content.

Comment 1

[k.krish]

Attachment: Bug 1280769 - Add Hyphenation Dictionary as separate kind

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

Comment 2

[k.krish]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

https://reviewboard.mozilla.org/r/59562/#review56700

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java:140
(Diff revision 2)
>      public boolean isAssetArchive() {
> -        return TYPE_ASSET_ARCHIVE.equals(type);
> +        return (TYPE_ASSET_ARCHIVE.equals(type) && isKnownContent());
>      }

Oh, sorry for the confusion. In the other bug I meant we could call isAssetArchive() from within isKnownContent(). Not the other way around. isAssetArchive() should just do what the name says. But a content is only known if it has a known type (asset archive)  and kind (font or hyphenation dictionary).

Comment 4

[k.krish]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

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

Comment 5

[k.krish]

https://reviewboard.mozilla.org/r/59562/#review56700

> Oh, sorry for the confusion. In the other bug I meant we could call isAssetArchive() from within isKnownContent(). Not the other way around. isAssetArchive() should just do what the name says. But a content is only known if it has a known type (asset archive)  and kind (font or hyphenation dictionary).

Oops. Sorry for the mixup. I've updated the code.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

This is looking good. Did you already test this on try?

Maybe you can look into extending TestDownloadAction too. Just some very simple tests for isHyphenationDictionary() and isKnownContent() would be nice!

Comment 7

[k.krish]

Not yet. Shall I write those tests and ship it with this bug or a separate one? If we are going to have a separate bug I will trigger a try build for this bug :)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

As you like. It would be nice to land the tests with the code. However if you want to take some more time for them, then let's run the current code on try and land this ahead. Right now this code is not used yet, so the risk is low.

Comment 9

[k.krish]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

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

Comment 10

[k.krish]

I have extended the TestDownloadAction class. I have written test cases for both isHyphenationDictionary and isKnownContent with few important scenarios. :)

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

https://reviewboard.mozilla.org/r/59562/#review56740

Great test! :)

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java:139
(Diff revision 4)
> +    /**
> +     *Checks whether the content to be downloaded is a known content.
> +     *Currently it checks whether the type is "Asset Archive" and is of kind
> +     *"Font" or "Hyphenation Dictionary".
> +     */
> +    public boolean isKnownContent() { return ((isFont() || isHyphenationDictionary()) && isAssetArchive()); }

nit: Move the return statement to its own line:
... isKnownContent() {
    return ...
}

Comment 12

[k.krish]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

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

Comment 13

[k.krish]

Shall I push this patch to try?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to k.krish from comment #13)
> Shall I push this patch to try?

Yeah, I think we could land this part after a successful try run.. right?

Comment 15

[k.krish]

Comment on attachment 8763337 [details]
Bug 1280769 - Add Hyphenation Dictionary as separate kind

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59562/diff/5-6/

Comment 16

[k.krish]

Try Results

https://treeherder.mozilla.org/#/jobs?repo=try&revision=885e1cd38275

Comment 17

[k.krish]

Yes we can land this patch now as try results have succeeded.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0f427b2d15eb
Add Hyphenation Dictionary as separate kind. r=sebastian

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f427b2d15eb