Bug 43467 - Allow the language for hyphenation to be specified
Summary: Allow the language for hyphenation to be specified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 23:16 PDT by mitz
Modified: 2010-08-04 23:53 PDT (History)
6 users (show)

See Also:


Attachments
Add -webkit-hyphenate-locale (28.92 KB, patch)
2010-08-03 23:18 PDT, mitz
darin: review+
Details | Formatted Diff | Diff
Add -webkit-hyphenate-locale (112.75 KB, patch)
2010-08-04 13:38 PDT, mitz
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2010-08-03 23:16:09 PDT
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.
Comment 1 mitz 2010-08-03 23:18:24 PDT
Created attachment 63418 [details]
Add -webkit-hyphenate-locale
Comment 2 WebKit Review Bot 2010-08-03 23:19:36 PDT
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.
Comment 3 Darin Adler 2010-08-03 23:29:46 PDT
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.
Comment 4 mitz 2010-08-03 23:43:05 PDT
(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!
Comment 5 Simon Fraser (smfr) 2010-08-04 08:07:32 PDT
Is there any W3C draft covering these hyphenate properties?
Comment 6 Simon Fraser (smfr) 2010-08-04 08:16:03 PDT
All I can find is http://www.w3.org/TR/css3-gcpm/#hyphenation, and it doesn't have a hyphentate-locale property.
Comment 7 mitz 2010-08-04 08:42:10 PDT
(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.
Comment 8 mitz 2010-08-04 13:38:13 PDT
Created attachment 63488 [details]
Add -webkit-hyphenate-locale
Comment 9 WebKit Review Bot 2010-08-04 13:43:22 PDT
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.
Comment 10 Simon Fraser (smfr) 2010-08-04 13:58:44 PDT
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?
Comment 11 mitz 2010-08-04 14:29:22 PDT
Fixed in <http://trac.webkit.org/projects/webkit/changeset/64677> (addressed Simon’s comments).
Comment 12 Eric Seidel (no email) 2010-08-04 23:40:19 PDT
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.
Comment 14 mitz 2010-08-04 23:50:23 PDT
(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?
Comment 15 Eric Seidel (no email) 2010-08-04 23:53:30 PDT
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.