Bug 43467 – Allow the language for hyphenation to be specified

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

Add a -webkit-hyphenate-locale property whose value can be either auto or a locale identifier string. The initial value is auto and the property is inherited. When the value is a locale identifier, hyphenation should follow the rules for the specified locale.

Created attachment 63418 [details]
Add -webkit-hyphenate-locale

[WebKit Review Bot]

Attachment 63418 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:388:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 63418 [details]
Add -webkit-hyphenate-locale

> +static CFLocaleRef getCFLocale(const AtomicString& localeIdentifier)

Can you come up with a name for this that doesn't include the word "get"? Maybe you could just use the word "find" instead? Or "compute"?

> +static bool localeIsEnglish(const AtomicString& localeIdentifier)

Is there any way for this function to share code with getCFLocale in HyphenationCF.cpp? They seem to have many many lines of code in common. Maybe a template or something? I suppose HyphenationMac.mm will be deleted once SnowLeopard is obsolete, but that might take a long time!

> -            bool canHyphenate = style->hyphens() == HyphensAuto;
> +            bool canHyphenate = style->hyphens() == HyphensAuto && WebCore::canHyphenate(style->hyphenateLocale());

"hyphenate locale" is such an unfortunate term. I know the CSS property has to have that name because it's in the standard, but "hyphenation locale" is so much more readable. Same thought about "hyphenate character", perhaps "hyphenation string" is the best name to use internally in places like RenderStyle.

(In reply to comment #3)
> (From update of attachment 63418 [details])
> > +static CFLocaleRef getCFLocale(const AtomicString& localeIdentifier)
> 
> Can you come up with a name for this that doesn't include the word "get"? Maybe you could just use the word "find" instead? Or "compute"?

What’s your objection to “get”? The FontCache class has a bunch of get* methods, and this function does the same thing, except it’s not a member function in a *Cache class. I don’t like “find” (reminds me of HashMap::find(), which cannot add entries) and I this isn’t really computing anything.

> > +static bool localeIsEnglish(const AtomicString& localeIdentifier)
> 
> Is there any way for this function to share code with getCFLocale in HyphenationCF.cpp? They seem to have many many lines of code in common. Maybe a template or something? I suppose HyphenationMac.mm will be deleted once SnowLeopard is obsolete, but that might take a long time!

Maybe a template or something. I am going to attempt this.

> > -            bool canHyphenate = style->hyphens() == HyphensAuto;
> > +            bool canHyphenate = style->hyphens() == HyphensAuto && WebCore::canHyphenate(style->hyphenateLocale());
> 
> "hyphenate locale" is such an unfortunate term. I know the CSS property has to have that name because it's in the standard, but "hyphenation locale" is so much more readable. Same thought about "hyphenate character", perhaps "hyphenation string" is the best name to use internally in places like RenderStyle.

1. Unlike hyphenate-character, which comes from a CSS Working Draft, hyphenate-locale is not currently part of any CSS Working Draft or Recommendation. I think the name hyphenate-character is awful. I chose the name hyphenate-locale to match, because having "hyphenation-locale" in addition to "hyphenate-character" and "hyphens" as property names seemed the worst.

2. I am going to change the names in RenderStyle (I actually used hyphenationLocale() in my first version of this).

Thanks for the review!

[Simon Fraser (smfr)]

Is there any W3C draft covering these hyphenate properties?

[Simon Fraser (smfr)]

All I can find is http://www.w3.org/TR/css3-gcpm/#hyphenation, and it doesn't have a hyphentate-locale property.

(In reply to comment #5)
> Is there any W3C draft covering these hyphenate properties?

From comment #4:
> 1. Unlike hyphenate-character, which comes from a CSS Working Draft, hyphenate-locale is not currently part of any CSS Working Draft or Recommendation.

Created attachment 63488 [details]
Add -webkit-hyphenate-locale

[WebKit Review Bot]

Attachment 63488 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:388:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 63488 [details]
Add -webkit-hyphenate-locale

 +                      i = m_cache.size();
I'd find it slightly clearer if, rather than reassigning to 'i', you used a new variable ("foundIndex" or something). Took me a while to grok why you reassigned to i.

WebCore/platform/text/cf/HyphenationCF.cpp:66
 +  //    RetainPtr<CFLocaleRef> locale = getCachedLocaleProperty<LocaleCFLocale>(localeIdentifier);
Remove commented code?

Fixed in <http://trac.webkit.org/projects/webkit/changeset/64677> (addressed Simon’s comments).

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/LayoutTests/fast/text/hyphenate-locale.html seems to be very flaky.  It's failing on the buildbots as well as the commit-queue.

[Eric Seidel (no email)]

Example failure:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r64714%20(18761)/fast/text/hyphenate-locale-pretty-diff.html

(In reply to comment #13)
> Example failure:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r64714%20(18761)/fast/text/hyphenate-locale-pretty-diff.html

Is this the missing AppleLanguages default issue again?

[Eric Seidel (no email)]

Might be.  I went ahead and defined AppleLanguages on the commit-queue machine just in case.  I don't have access to the leopard buildbot to know if it has AppleLanguages set or not.