[Webkit-unassigned] [Bug 10228] CSS3: Support the hyphenate property
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jun 20 16:09:51 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=10228
mitz at webkit.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #59196|0 |1
is obsolete| |
Attachment #59219| |review?
Flag| |
--- Comment #26 from mitz at webkit.org 2010-06-20 16:09:50 PST ---
Created an attachment (id=59219)
--> (https://bugs.webkit.org/attachment.cgi?id=59219)
Implement the 'hyphens' and 'hyphenate-character' properties
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list