[webkit-reviews] review requested: [Bug 10228] CSS3: Support the hyphenate property : [Attachment 59219] Implement the 'hyphens' and 'hyphenate-character' properties

[bugzilla-daemon at webkit.org]

mitz at webkit.org has asked  for review:
Bug 10228: CSS3: Support the hyphenate property
https://bugs.webkit.org/show_bug.cgi?id=10228

Attachment 59219: Implement the 'hyphens' and 'hyphenate-character' properties
https://bugs.webkit.org/attachment.cgi?id=59219&action=review

------- Additional Comments from mitz at webkit.org
(In reply to comment #25)
> (From update of attachment 59196 [details])
> > +		 if (style->hyphenateCharacter() == nullAtom)
> > +		     return CSSPrimitiveValue::createIdentifier(CSSValueAuto);
> 
> I see the use of "== nullAtom" earlier in the file. But I'm surprised we do
that instead of calling the isNull function.

Changed to use isNull().

> > +size_t lastHyphenLocation(const UChar*, size_t length, size_t
maxLocation);
> 
> To me it's not obvious what the relationship between maxLocation and length
is.

I considered appending BeforeIndex to the function name and renaming the last
parameter, but settled on just renaming maxLocation to beforeIndex.

> > +	 static CFLocaleRef locale = 0;
> > +	 if (!locale) {
> > +	     RetainPtr<CFStringRef> localeIdentifier(AdoptCF,
CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, currentSearchLocaleID(),
strlen(currentSearchLocaleID()), kCFStringEncodingASCII, False,
kCFAllocatorNull));
> > +	     locale = CFLocaleCreate(kCFAllocatorDefault,
localeIdentifier.get());
> > +	 }
> 
> I prefer initializing with a create function called in the initializer to a
separate if statement like this one.

Done.

> Why not just this?
> 
>     static bool localeIsEnglish = !strcmp("en", currentSearchLocaleID());
> 
> Seems better to me than the two booleans.

Changed (when I wrote this I thought that checking if the locale is English was
going to require multiple statements).

> > +static void adjustCharactersAndLengthForHyphen(Vector<UChar, 256>
charactersWithHyphen, RenderStyle* style, const UChar*& characters, int&
length)
> 
> Did you mean to pass a reference to the Vector here?

Yes.

> > -	 InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine,
bool& isLineEmpty, bool& previousLineBrokeCleanly, EClear* clear = 0);
> > +	 InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine,
bool& isLineEmpty, bool& previousLineBrokeCleanly, bool& hyphenated, EClear*
clear = 0);
> 
> The argument name "clear" here seems unneeded.

Removed.

> I think you might want a typedef for the repeated Vector<UChar, 256> type.

Done, although I couldn’t come up with a good name.

> The use of "const Hyphens h" also seems wrong. Just "Hyphens h" will do.

Fixed.

> Looks like EWS builds are failing on this patch. I’ll say review- based on
the EWS failures and the lack of the "&" in the Vector argument.

I am not sure what’s causing the failures. Windows appears to be failing to
merge the vcproj changes, perhaps due to different line endings. Mac might be
failing because I didn’t include an updated WebKitSystemInterface binary. As
for Chromium, I don’t know what to do besides adding Hyphenation.cpp to the
gypi file.