Add a build flag to exclude hyphenation dictionaries from Android builds

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: jonalmeida)

Tracking

(Blocks 1 bug)

Trunk
Firefox 44
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Just like Bug 1063868, but for hyphenation dictionaries. See Bug 1095719 for assessment.
Be aware of the aborted work in Bug 1128033, which tried to move the hyphenation dictionary definitions to moz.build.  (It failed due to FINAL_TARGET shenanigans.)

In theory, this ticket is as easy as adding a build flag for excluding (or restricting) the set of hyphenation dictionaries and then using it here: https://dxr.mozilla.org/mozilla-central/source/intl/locales/Makefile.in#7.

In practice, I haven't a clue how this will interact with l10n repacks.  Given that one of the motivations for this ticket is to support smaller APKs for Indic locales, that worries me.
(Reporter)

Comment 3

4 years ago
Jon, don't forget to attach patches to bugs; Try pushes disappear eventually.
Assignee: nobody → jalmeida
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Posted patch bug_257055_wip.patch (obsolete) — Splinter Review
Sounds fair. I'm just making minor changes first to see how broken try becomes. Here's my patch so far: It just uses the MOZ_ANDROID_RESOURCE_CONSTRAINED flag, but I'm looking at other flags in m/a/confvars.sh to see if they're more suitable.
(Assignee)

Comment 5

4 years ago
So the tests that fail are because of the missing hyphenation files (which is good) but I haven't figured out how to fix the failing mochitests that test the hyphenation without removing them entirely for now until bug 1095719 is fixed.

In which case, should I leave my patch for review and only land it once I can land the other bug, which would also re-enable the tests?
Flags: needinfo?(rnewman)
(Reporter)

Comment 6

4 years ago
I think it's fine to have a patch that causes tests to fail if you disable the feature, so long as the feature remains enabled! This is exactly the situation we're in for fonts and reftests after Bug 1063868.

We will need to solve how to deliver these assets at test time for both of those cases. These are the first steps on a long road.
Flags: needinfo?(rnewman)
Comment on attachment 8647616 [details] [diff] [review]
bug_257055_wip.patch

Instead of using MOZ_ANDROID_RESOURCE_CONSTRAINED we could introduce a new constant like we did for fonts (MOZ_ANDROID_EXCLUDE_FONTS). For fonts this is always disabled for now. This would allow us to ship this flag now and turn it on as soon as we have a solution for downloading the assets.
Flags: needinfo?(jalmeida)
(Assignee)

Comment 8

4 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Comment on attachment 8647616 [details] [diff] [review]
> bug_257055_wip.patch
> 
> Instead of using MOZ_ANDROID_RESOURCE_CONSTRAINED we could introduce a new
> constant like we did for fonts (MOZ_ANDROID_EXCLUDE_FONTS). For fonts this
> is always disabled for now. This would allow us to ship this flag now and
> turn it on as soon as we have a solution for downloading the assets.

This is a good idea; I'll do this today. Although, I thought MOZ_ANDROID_RESOURCE_CONSTRAINED is a catch-all flag for all resources that can be used to remove all resources we don't want to ship and not just fonts and hyphenations. Maybe consider using it again later?
Flags: needinfo?(jalmeida)
(Assignee)

Comment 9

4 years ago
Haven't finished the addon because of my schedule. NI myself so I don't forget.
Flags: needinfo?(jonalmeida942)
(Assignee)

Comment 10

4 years ago
Commented on the wrong bug.. but I'm doing this as well.
Flags: needinfo?(jonalmeida942)
(Assignee)

Comment 13

4 years ago
Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander
Attachment #8675877 - Flags: review?(nalexander)
(Assignee)

Comment 14

4 years ago
This is an old patch that I finally got around to testing if it works.
(Assignee)

Comment 15

4 years ago
Posted patch enable_hyphen_flag.patch (obsolete) — Splinter Review
For enabling the flag.
Attachment #8647616 - Attachment is obsolete: true
Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

https://reviewboard.mozilla.org/r/22477/#review20035

I think the approach is fine, but there's nothing Android specific about this.  Also, this doesn't exclude the hyphenation code or processing; it's just the details, so perhaps MOZ_EXCLUDE_HYPHENATION_DICTIONARIES?
Attachment #8675877 - Flags: review?(nalexander)
(In reply to Nick Alexander :nalexander from comment #16)
> Comment on attachment 8675877 [details]
> MozReview Request: Bug 1175555 - Build flag to exclude hyphenation
> dictionaries from Android builds r?nalexander
> 
> https://reviewboard.mozilla.org/r/22477/#review20035
> 
> I think the approach is fine, but there's nothing Android specific about
> this.  Also, this doesn't exclude the hyphenation code or processing; it's
> just the details, so perhaps MOZ_EXCLUDE_HYPHENATION_DICTIONARIES?

Gah!  s/details/data files/.
(Assignee)

Comment 19

4 years ago
Updated patch for enabling flag.
Attachment #8675879 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8675877 - Flags: review?(nalexander)
(Assignee)

Comment 20

4 years ago
Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander
Attachment #8675877 - Flags: review?(nalexander) → review+
Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

https://reviewboard.mozilla.org/r/22477/#review20417

nits.  Looks good!

::: configure.in:4865
(Diff revision 2)
> +dnl = Whether to exclude hyphenations files in the final APK.

Generalize comment.

::: intl/locales/Makefile.in:8
(Diff revision 2)
> -PATTERN_FILES = $(strip $(wildcard $(srcdir)/*/hyphenation/*.dic))
> +    PATTERN_FILES = $(strip $(wildcard $(srcdir)/*/hyphenation/*.dic))

nit: don't indent, since it's not Makefile.in convention, and you haven't done so elsewhere.

::: intl/locales/Makefile.in:14
(Diff revision 2)
> +endif

nit:
```
endif # MOZ_EXCLUDE_HYPHENATION_DICTIONARIES
```

::: mobile/android/base/AppConstants.java.in:157
(Diff revision 2)
> +     * Whether this APK was built with hyphenations files included

nit: full sentence.
(Assignee)

Comment 23

4 years ago
Comment on attachment 8675877 [details]
MozReview Request: Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander

Bug 1175555 - Build flag to exclude hyphenation dictionaries from Android builds r?nalexander
Attachment #8675877 - Flags: review+ → review?(nalexander)
(Assignee)

Updated

4 years ago
Attachment #8675877 - Flags: review?(nalexander) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a3d24ccd492
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.