688779 - restore en-US hyphenation to the Android package

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, restore the en-US hyphenation dictionary to the Android product

In bug 686637, all the hyphenation files were removed from the APK, along with the code that extracts them to the filesystem so that libhyphen can load them, in order to avoid the cost of extracting so many files at first-run time.

However, this regresses the CSS -moz-hyphens:auto feature that we shipped in FF6, and that is in use on the web. (In FF6, we had support for English hyphenation only; a large collection of resources for additional languages landed for FF8.)

A solution that allows us to keep the hyphenation resources in the product, but avoid the cost of unpacking them, is awaiting review in bug 655337. However, pending this solution, I think we should keep the en-US hyphenation data in the Android product, as shipped in FF6, so that while we may delay introducing support for more languages until bug 655337 lands, we won't actually regress existing behavior.

Accordingly, the attached patch restores the code to unpack the hyphenation dictionary from the APK on first run, but packages only the en-US dictionary. This will avoid the large first-run hit that we suffered from unpacking several dozen dictionaries, so the impact on the initial user experience should be minimal.

Comment 1

[Doug Turner (:dougt)]

Do you think it's right that -moz-hyphens:auto works for only EN_US text?  Wouldn't it be better all off or all on?  (where all on == all locales that we ship on)?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

In the long run, I'd like to get bug 655337 reviewed and landed so we can determine the performance impact. I think that should happen in Fx10, after Fx9 merges to Aurora. I don't think we want this intermediate patch as a stopgap for Fx10 - let's go straight to the solution in bug 655337.

Also, I know it's a bit heavy handed, but I want to remove all extractions, including the en-US hyphenation file, from Fx8 and Fx9. We have been working hard to remove all extractions in order to find ways to improve startup performance. Yes it creates a "regression" by removing en-US hypenations, but we'll get it back in the product soon with the work in bug 655337.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> In the long run, I'd like to get bug 655337 reviewed and landed so we can
> determine the performance impact. I think that should happen in Fx10, after
> Fx9 merges to Aurora. I don't think we want this intermediate patch as a
> stopgap for Fx10 - let's go straight to the solution in bug 655337.

Yes, Fx10 should have bug 655337, so we don't need this as a stopgap there.

But for Fx8 and Fx9 it would allow us to fix the extraction-on-first-run issue without regressing the feature that we shipped in Fx6 and Fx7. I think that's worthwhile - especially as it's essentially zero risk (just restoring the situation we had in Fx6, prior to the addition of 30+ more hyphenation dictionaries, which was what added an arguably unacceptable burden to the initial startup).

> Also, I know it's a bit heavy handed, but I want to remove all extractions,
> including the en-US hyphenation file, from Fx8 and Fx9. We have been working
> hard to remove all extractions in order to find ways to improve startup
> performance. Yes it creates a "regression" by removing en-US hypenations,
> but we'll get it back in the product soon with the work in bug 655337.

If this affected _every_ startup, I could accept that it's necessary, but I don't believe the impact of the en-US file _alone_ on the initial startup _only_ is significant enough to justify removing the feature.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Doug Turner (:dougt) from comment #1)
> Do you think it's right that -moz-hyphens:auto works for only EN_US text? 
> Wouldn't it be better all off or all on?  (where all on == all locales that
> we ship on)?

The collection of languages for which we support hyphenation is unrelated to the locale of the browser; the consensus was that we should support hyphenation for the same collection of languages on all installations, as far as possible.

Supporting only en-US was clearly just a first step when the feature was introduced, and the intention was always to support as many languages as possible over time. But (crucially), we are currently shipping support for hyphenating en-US content, and people are using it, so removing it creates a regression on the web. We haven't yet shipped support for other languages, so we can decide to wait for bug 655337 before doing so; this may _delay_ them but does not _regress_ what we've already shipped (and documented).

Comment 5

[Doug Turner (:dougt)]

makes sense.  Does this regress startup perf at all?

Comment 6

[Jonathan Kew [:jfkthame]]

Not that I can tell.

In theory it must add a small amount to first-run startup, but as it's only unpacking one file (rather than over 30 of them, which was what bug 686637 dealt with), the "regression" is negligible in comparison to the overall install-and-start process. We'll get rid of it completely when bug 655337 lands, but in the meantime this is "lost in the noise" anyway.

There's no impact on normal startup (after the file was unpacked on first run), as we don't load the dictionary until/unless a page actually requests auto-hyphenation.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 562073 [details] [diff] [review]
patch, restore the en-US hyphenation dictionary to the Android product

please watch Ts when this lands.

Comment 8

[Jonathan Kew [:jfkthame]]

Landed on inbound, with followup changeset to re-enable the reftests for en-US hyphenation:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6129ccfa2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3430e302d5bd

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/062de7617f24
https://hg.mozilla.org/mozilla-central/rev/d19199a9d831

I accidentally pushed this patch to mozilla-central while it was living on inbound.  On the next merge, it will be merged.  Sorry for the mess!

Comment 10

[(no longer active)]

Merged from inbound: https://hg.mozilla.org/mozilla-central/rev/1d6129ccfa2b

Comment 11

[(no longer active)]

Also: https://hg.mozilla.org/mozilla-central/rev/3430e302d5bd

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 562073 [details] [diff] [review]
patch, restore the en-US hyphenation dictionary to the Android product

Requesting approval for mozilla-aurora and mozilla-beta.

We shipped auto-hyphenation support for English content in FF6 and FF7, and web authors are making use of the feature.

Bug 686637 regressed this by removing _all_ hyphenation resources from the Android package (not only the 30+ _additional_ languages that we have on trunk, and that were adding an unacceptable level of overhead to the first-run startup).

This patch restores the en-US data, so as to maintain the existing behavior that we have in the release product, and avoid this regression. (Bug 655337 should eventually allow us to ship more dictionaries without added startup cost, but is not yet reviewed.)

The risk of this patch is minimal, as it includes nothing new, it just restores what we already shipped in FF6 and FF7.

Comment 13

[Asa Dotzler [:asa]]

Please correct me if I'm mis-stating the situation here. 

We added hyphenation support but it caused a startup perf hit so we removed hyphenation. Now we think we've got a lesser hit on perf and we'd like to add partial hyphenation support back in. 

Not shipping some hyphenation would be a regression for some number of websites or web developers already counting on this very new feature.

I'm suspect that there are m/any sites using this capability and I'm generally opposed to taking changes like this in Beta. I also don't think this is going to drive Firefox mobile adoption in either direction. If there are major sites that we know we'd be regressing and/or large numbers of Firefox mobile users that would be adversely affected, please say so. Otherwise, this should probably just ride the trains up normally.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Asa Dotzler [:asa] from comment #13)
> Please correct me if I'm mis-stating the situation here. 

OK, I'll take a shot at that! :)

> We added hyphenation support but it caused a startup perf hit so we removed
> hyphenation.

No; we initially added hyphenation support with an English dictionary (in FF6). I have not seen any claims that this caused a startup perf hit. (It must in theory have carried a slight first-run cost, but for the single file involved this was not significant.)

Subsequently, on the FF8 train, we added dictionaries for over 30 more languages. This still did not cause a *startup* perf hit, but does cause a *first-run only* hit because we don't yet have support for accessing them directly from the APK file (bug 655337).

> Now we think we've got a lesser hit on perf and we'd like to
> add partial hyphenation support back in. 

Because of the first-run cost of unpacking all these files, bug 686637 pulled *all* of the hyphenation files out of the package - including the originally-shipped en-US file as well as the 30+ newly-added ones. While this obviously resolves the added first-run cost of the extra files, I believe pulling the already-shipped English support along with the rest was going a step too far. (And said so in that bug, but people went ahead and landed the patch in its overly-drastic form anyhow.)

> Not shipping some hyphenation would be a regression for some number of
> websites or web developers already counting on this very new feature.

Yes.

> I'm suspect that there are m/any sites using this capability 

I don't know how many there are, but it's certainly not difficult to find examples; a Google search for "moz-hyphens" returns nearly 7000 hits. Plenty of them are people blogging (enthusiastically) about the feature, but also using it in their stylesheets; e.g. http://www.ashrobbins.com/2011/09/make-it-look-nicer/.

> and I'm
> generally opposed to taking changes like this in Beta. I also don't think
> this is going to drive Firefox mobile adoption in either direction. If there
> are major sites that we know we'd be regressing and/or large numbers of
> Firefox mobile users that would be adversely affected, please say so.
> Otherwise, this should probably just ride the trains up normally.

Bug 686637 was pushed to Aurora on 2011-09-26, immediately before the migration to Beta. So the regression (of disabling all hyphenation, including English) effectively went straight into Beta, with no significant period of testing/exposure/evaluation on Aurora. When landing that patch, mfinkle commented that "It's possible we could reconsider and talk about bug 688779...", but the timing did not allow for any "reconsideration" between that landing and the merge to Beta; so that's why this is now a request for Beta approval.

Comment 15

[Jet Villegas (inactive)]

We'd really like to land this in FF8 as hyphenation is even more important when wrapping for small mobile screens. FF10 gets English hyphenation back, but it would be ideal to get this into FF8 as well. Can we get some startup profiling on trunk to see if it's acceptable there? If so, please consider this patch for FF8 & 9.

Comment 16

[Asa Dotzler [:asa]]

Mark looked at this on the trunk and the performance impact is very minimal so we agreed to take this back into Aurora and Beta.

Comment 17

[Jonathan Kew [:jfkthame]]

Thanks, pushed to Aurora and Beta:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0da47992b626
https://hg.mozilla.org/releases/mozilla-beta/rev/276ccee22d0b