Closed Bug 1276588 Opened 8 years ago Closed 7 years ago

Modify DownloadAction to handle hyphenation dictionaries

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

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

References

Details

Attachments

(1 file, 1 obsolete file)

Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
Currently our code only knows how to download and handle fonts. Extend the code to handle hyphenation dictionaries too.

StudyAction: This is where we decide what to download:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java#32

DownloadAction: This is where we perform the actual download:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
For fonts the destination folder is determined in the base class:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java#97
Depends on: 1276587
For testing I uploaded the dictionaries to people.mozilla.org. Later we want to upload the files to Kinto (bug 1276587):

https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_af.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_bg.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_ca.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_cy.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_da.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_de-1901.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_de-1996.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_de-CH.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_en_US.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_eo.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_es.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_et.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_fi.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_fr.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_gl.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_hr.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_hsb.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_hu.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_ia.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_is.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_it.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_kmr.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_la.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_lt.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_mn.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_nb.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_nl.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_nn.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_pl.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_pt.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_ru.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_sh.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_sl.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_sv.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_tr.dic.gz
https://people.mozilla.org/~skaspari//dlc/hyphenation/hyph_uk.dic.gz
Sample log for hyphenation dictionary - DLCDownloadAction

>D/DLCDownloadAction: Downloading: [asset-archive,hyphenation-dictionary] 754c3280-03bd-bdb4-3972-46ee8d3fe9db (6779 bytes) d8047715611105d85d8e66627d20ae21bfb67c33e4c52a22b4a2a97cee279245

>D/DLCDownloadAction: Successfully downloaded: [asset-archive,hyphenation-dictionary] 754c3280-03bd-bdb4-3972-46ee8d3fe9db (6779 bytes) d8047715611105d85d8e66627d20ae21bfb67c33e4c52a22b4a2a97cee279245

>V/DLCVerifyAction: Content okay: [asset-archive,hyphenation-dictionary] 754c3280-03bd-bdb4-3972-46ee8d3fe9db (6779 bytes) d8047715611105d85d8e66627d20ae21bfb67c33e4c52a22b4a2a97cee279245
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review55358

This looks quite good already. If we remove the test code and validate that the downloaded dictionaries are used (see below) then we could already land this patch.

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:40
(Diff revision 1)
> -        @IntDef({MEMORY, DISK_IO, SERVER, NETWORK})
> +        @IntDef({MEMORY, DISK_IO, SERVER, NETWORK, URL})
>          public @interface ErrorType {}
>          public static final int MEMORY = 1;
>          public static final int DISK_IO = 2;
>          public static final int SERVER = 3;
>          public static final int NETWORK = 4;
> +        public static final int URL = 5;

Let's not add URL here: It's only needed temporarily for createDownloadURL() below and we won't need it later. Also: If we can create the URL then it's probably not a "recoverable" error because then we will never be able to create a URL in this case.

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:110
(Diff revision 1)
>  
>              return new File(destinationDirectory, content.getFilename());
>          }
>  
> +        if (content.isHyphenationDictionary()) {
> +            File destinationDirectory = new File(context.getApplicationInfo().dataDir, "hyphenation-dictionaries");

Looking at the hypenation manager implementation [1] it looks like Gecko is already able to load dictionaries from the application directory if the folder is named "hyphenation". Let's use that name - and with that you should already be able to see the dictionaries being used after download and app restart.

[1] https://dxr.mozilla.org/mozilla-central/source/intl/hyphenation/glue/nsHyphenationManager.cpp#162-170

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:112
(Diff revision 1)
> +            if (!destinationDirectory.exists() && !destinationDirectory.mkdirs()) {
> +                throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO,
> +                        "Destination directory does not exist and cannot be created");
> +            }
> +
> +            return new File(destinationDirectory, content.getFilename());
> +        }

All this code seems to be duplicated: Above we do exactly the same, just with a different destinationDirectory. I guess we can simplify this by choosing the right destinationDirectory and then having the other code only once.

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:40
(Diff revision 1)
>  
>      private static final String CACHE_DIRECTORY = "downloadContent";
>  
>      private static final String CDN_BASE_URL = "https://fennec-catalog.cdn.mozilla.net/";
>  
> +    private static final String HYPHENATION_DICT_TEMP_URL = "https://people.mozilla.org/";

This is okay for local testing. If you remove it from this patch then we might be able to land this patch now (without the catalog update it shouldn't do anything, right?).

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:257
(Diff revision 1)
>  
> -    protected String createDownloadURL(DownloadContent content) {
> +    protected String createDownloadURL(DownloadContent content) throws RecoverableDownloadContentException {
>          final String location = content.getLocation();
>  
> +        //    TODO: Update the BASE_URL after uplodaing the hyphenation dictionaries to Kinto - Bug 1276587
> +        if(content.isFont()){

Nit: Spaces: if_(..)_{

Just run ./mach gradle checkstyle and it should show you style warnings for your code. :)

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:260
(Diff revision 1)
> +        else if(content.isHyphenationDictionary()){
> +            return HYPHENATION_DICT_TEMP_URL + content.getLocation();
> +        }
> +        throw new RecoverableDownloadContentException(RecoverableDownloadContentException.URL, "Malformed URL");
> +    }

This is just for local testing.

::: mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java:32
(Diff revision 1)
>              if (!isMatching(context, content)) {
>                  // This content is not for this particular version of the application or system
>                  continue;
>              }
>  
> -            if (content.isAssetArchive() && content.isFont()) {
> +            if ((content.isAssetArchive() && content.isFont()) || (content.isAssetArchive() && content.isHyphenationDictionary())) {

We only need to check content.isAssertArchive() once here. As this statement gets longer we could try to hide it behind a method in DownloadContent. Maybe something like content.isKnownContent(), content.isContentToDownload(), .. some method that describes that we are interested in downloading this.
Attachment #8761152 - Flags: review?(s.kaspari)
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/1-2/
Attachment #8761152 - Attachment description: Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction → Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction - fixes
Attachment #8761152 - Flags: review?(s.kaspari)
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review55852

It looks like this patch only contains the diff between the previous patch and your new state. Can you create a patch that contains all changes?
Attachment #8761152 - Flags: review?(s.kaspari)
Review commit: https://reviewboard.mozilla.org/r/58950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58950/
Attachment #8761152 - Attachment description: Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction - fixes → Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction
Attachment #8762057 - Flags: review?(s.kaspari)
Attachment #8761152 - Flags: review?(s.kaspari)
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/2-3/
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/3-4/
Attachment #8762057 - Attachment is obsolete: true
Attachment #8762057 - Flags: review?(s.kaspari)
Attachment #8761152 - Flags: review?(s.kaspari) → review+
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review56558

Nice! I like how small the patch ended up being.

I'll r+ (with changes) but let's not land this patch before we have updated the online version of the catalog and we have verified that this is working as intended. But feel free to push this to try and see if there are any issues.

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:100
(Diff revision 4)
>      protected File getDestinationFile(Context context, DownloadContent content)
>              throws UnrecoverableDownloadContentException, RecoverableDownloadContentException {
> +        File destinationDirectory;
>          if (content.isFont()) {
> -            File destinationDirectory = new File(context.getApplicationInfo().dataDir, "fonts");
> +            destinationDirectory =  new File(context.getApplicationInfo().dataDir, "fonts");
> +        }

nit: Move the else statement to the same line as the closing brace:
> } else if (..) {

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:103
(Diff revision 4)
>          if (content.isFont()) {
> -            File destinationDirectory = new File(context.getApplicationInfo().dataDir, "fonts");
> +            destinationDirectory =  new File(context.getApplicationInfo().dataDir, "fonts");
> +        }
> +        else if (content.isHyphenationDictionary()) {
> +            destinationDirectory = new File(context.getApplicationInfo().dataDir, "hyphenation");
> +        }

nit: Same here:
> } else {

::: mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java:32
(Diff revision 4)
>              if (!isMatching(context, content)) {
>                  // This content is not for this particular version of the application or system
>                  continue;
>              }
>  
> -            if (content.isAssetArchive() && content.isFont()) {
> +            if (content.isAssetArchive() && content.isKnownContent()) {

Maybe we can move the isAssetArchive() check into isKnownContent() too.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java:134
(Diff revision 4)
>  
>      public boolean isHyphenationDictionary() {
>          return KIND_HYPHENATION_DICTIONARY.equals(kind);
>      }
>  
> +    public boolean isKnownContent() { return isFont() || isHyphenationDictionary(); }

Let's add a short javadoc comment explaining what 'known content' means (Content that we know how to download and handle).
https://reviewboard.mozilla.org/r/58466/#review56558

> Maybe we can move the isAssetArchive() check into isKnownContent() too.

Changed the function of isAssetArchive() it now checks type and checks whether the content is known or not - DownloadContent changes are part of Bug 1280769 because there was an overlap between these two changesets.

> Let's add a short javadoc comment explaining what 'known content' means (Content that we know how to download and handle).

Moved this code to Bug 1280769
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/4-5/
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/5-6/
Depends on: 1280769
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/6-7/
Blocks: 1285752
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58466/diff/7-8/
Keywords: checkin-needed
Stop! Let's wait until the online catalog has been updated and we verify that this code behaves correctly.
Keywords: checkin-needed
(In reply to Sebastian Kaspari (:sebastian) from comment #18)
> Stop! Let's wait until the online catalog has been updated and we verify
> that this code behaves correctly.

bug 1276587
Oh.. Okay.. :)
Hey Krish! The hyphenation dictionaries are now available in the online catalog:
https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/catalog/records

Do you have time to test this patch with the online catalog? If everything is working we can finally land this and bug 1285752. :)
Flags: needinfo?(k.krish)
Hey Sebastian, Sure I will test with the production catalog. I will also update the static catalog in this Bug 1276589. We can land this soon. :)
Flags: needinfo?(k.krish)
(In reply to k.krish from comment #22)
> Hey Sebastian, Sure I will test with the production catalog. I will also
> update the static catalog in this Bug 1276589. We can land this soon. :)

Actually let's skip that part. I'll close the bug. We already accept that the hyphenation dictionaries are not loaded whenever we are running low on memory - So we can live with not having them ready until we synchronized the catalog for the first time too.
Krish, did you have time to look into this?
Flags: needinfo?(k.krish)
Hey Sebastian, Yeah I did. I have submitted a patch for that having same commit message. 

 https://reviewboard.mozilla.org/r/58466/diff/8-9/
Flags: needinfo?(k.krish)
Oh! Seems like there's a space missing in the commit message and that's why it wasn't linked to this bug: "Bug1276588".
Oh.. My bad! I should have made sure that it was correct and notifications were sent :(
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review90396
Attachment #8761152 - Flags: review+
I'm confused by the new patch. There's no new code needed to download a catalog. That's already handled by SyncAction. The previous version of the patch was just fine. We just need to make sure that it is still working and the dictionaries are being downloaded.
(In reply to Sebastian Kaspari (:sebastian) from comment #30)
> I'm confused by the new patch. There's no new code needed to download a
> catalog. That's already handled by SyncAction. The previous version of the
> patch was just fine. We just need to make sure that it is still working and
> the dictionaries are being downloaded.

I just realised that I've duplicated the code. I'll update the patch and submit again
Mentor: s.kaspari
Review Request - https://reviewboard.mozilla.org/r/58466/diff/9-10/

Used the same commit message as previous commits.
Flags: needinfo?(s.kaspari)
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review93334

This is looking good. :)
Attachment #8761152 - Flags: review?(s.kaspari) → review+
Let's land this together with the nightly flag patch as soon as it is released.
Flags: needinfo?(s.kaspari)
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review94082

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:133
(Diff revision 10)
>              if (lastModified > 0) {
>                  builder.appendQueryParameter(KINTO_PARAMETER_SINCE, String.valueOf(lastModified));
>              }
>              // Only select the fields we are actually going to read.
>              builder.appendQueryParameter(KINTO_PARAMETER_FIELDS,
> -                    "attachment.location,attachment.original.filename,attachment.original.hash,attachment.hash,type,kind,attachment.original.size,match");
> +                    "attachment.location,attachment.original,attachment.hash,type,kind,match");

I filed a bug for this here:
https://github.com/Kinto/kinto/issues/920

It's already fixed but we need to wait until the update is deployed to the servers.
Oh great. I was about to file a bug for that. Thank you. Do we need to revert back to using older parameters? Or existing is fine?
I'm not sure if this has been deployed yet. Let's just try. But I'll talk to the team today too.
Comment on attachment 8761152 [details]
Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction

https://reviewboard.mozilla.org/r/58466/#review94082

> I filed a bug for this here:
> https://github.com/Kinto/kinto/issues/920
> 
> It's already fixed but we need to wait until the update is deployed to the servers.

Kinto is fixed and I have changed the URI in Fennec too.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 4 changes to 4 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 6c61ffc65184 needs "Bug N" or "No bug" in the commit message.
remote: Krishna <k.krish@yahoo.com>
remote: Bug1276588 Handling Hyphenation-Dictionaries in DownloadAction r=sebastian
remote: 
remote: MozReview-Commit-ID: GTuR0IarjGe
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
https://hg.mozilla.org/mozilla-central/rev/b8a265bc2cde
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
See Also: → 1353334
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: