Add Hyphenation Dictionary as separate kind in downloadable content

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: k.krish, Assigned: k.krish, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

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

Updated

3 years ago
Assignee: nobody → k.krish
Assignee

Updated

3 years ago
Summary: Add Hyphenation Dictionary as separate kind in download content → Add Hyphenation Dictionary as separate kind in downloadable content
Assignee

Comment 2

3 years ago
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/
Assignee

Updated

3 years ago
Blocks: 1276588
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).
Attachment #8763337 - Flags: review?(s.kaspari)
Assignee

Comment 4

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

Comment 5

3 years ago
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.
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!
Assignee

Comment 7

3 years ago
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 :)
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.
Assignee

Comment 9

3 years ago
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/
Assignee

Comment 10

3 years ago
I have extended the TestDownloadAction class. I have written test cases for both isHyphenationDictionary and isKnownContent with few important scenarios. :)
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 ...
}
Attachment #8763337 - Flags: review?(s.kaspari) → review+
Assignee

Comment 12

3 years ago
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/
Assignee

Comment 13

3 years ago
Shall I push this patch to try?
(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?
Assignee

Comment 15

3 years ago
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/
Assignee

Comment 17

3 years ago
Yes we can land this patch now as try results have succeeded.
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 18

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/0f427b2d15eb
Add Hyphenation Dictionary as separate kind. r=sebastian
Keywords: checkin-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0f427b2d15eb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.