Load hyphenation dictionaries from writable location

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
We are excluding the hyphenation dictionaries from the APK in the Bug 1095719. So when we exclude the hyphenation dictionary from the build, some reftests will fail. We need to package them separately and install them along with the build in this Bug 1276560.

We are loading the hyphenation dictionary into the test profile - sdcard/tests/reftest/profile/hyphenation/<files>

But we need to load the hyphenation dictionaries from the test profile directory.
(Assignee)

Updated

3 years ago
Assignee: nobody → k.krish
Blocks: 1276560
(Assignee)

Updated

3 years ago
Comment on attachment 8763334 [details]
Bug 1280757 - Load hyphenation from test profile directory

Simon, Masatoshi: You are the owners or peers of intl/ - Can anyone of you review the patch or flag someone who can? :)

We'd like to add a sub directory of the profile directory as additional directory to look for hyphenation dictionaries.
Attachment #8763334 - Flags: review?(smontagu)
Attachment #8763334 - Flags: review?(s.kaspari)
Attachment #8763334 - Flags: review?(VYV03354)
Attachment #8763334 - Flags: feedback+
Do you know why hyphenation dictionaries have been restricted to non-writable location? Or do you know who would have known that?
Flags: needinfo?(s.kaspari)
(In reply to Masatoshi Kimura [:emk] from comment #3)
> Do you know why hyphenation dictionaries have been restricted to
> non-writable location? Or do you know who would have known that?

I know nothing about the history of the hyphenation manager unfortunately. However it is loading dictionaries from the process dir:
https://dxr.mozilla.org/mozilla-central/source/intl/hyphenation/glue/nsHyphenationManager.cpp#161-170

.. and this directory is writeable and not restricted - at least on Android. And that's exactly what we want to use on Android: To reduce the app size we do not want to ship the dictionaries with the app anymore. Instead we download them at runtime into the process dir. However we can't do this when running reftests, so we would like to push them to the test profile. That's how we handle other files (e.g.) that need to exist for testing too.
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> .. and this directory is writeable and not restricted - at least on Android.

But it is usually not writable on desktop platforms, although it is possible to install Firefox to writable location.
Comment on attachment 8763334 [details]
Bug 1280757 - Load hyphenation from test profile directory

https://reviewboard.mozilla.org/r/59558/#review57062

OK, let's try and see whats going on.
Attachment #8763334 - Flags: review?(VYV03354) → review+
Comment on attachment 8763334 [details]
Bug 1280757 - Load hyphenation from test profile directory

Thank you!
Attachment #8763334 - Flags: review?(smontagu)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8763334 [details]
Bug 1280757 - Load hyphenation from test profile directory

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59558/diff/1-2/
Attachment #8763334 - Flags: review?(s.kaspari)
Attachment #8763334 - Flags: review+
Attachment #8763334 - Flags: feedback+
Comment on attachment 8763334 [details]
Bug 1280757 - Load hyphenation from test profile directory

This patch already had r+ from Masatoshi and it doesn't seem that the patch has changed (Is this just a rebase?). You can go ahead and land it if there are no other issues.
Attachment #8763334 - Flags: review?(s.kaspari)
(Assignee)

Comment 10

3 years ago
Yes it is just a rebase. I triggered a new try run for this too. I will land it asap
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dc96e5ff02fd
Load hyphenation from test profile directory. r=sebastian
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc96e5ff02fd
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.