[Webkit-unassigned] [Bug 10228] CSS3: Support the hyphenate property

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=10228


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59196|review?                     |review-
               Flag|                            |




--- Comment #25 from Darin Adler <darin at apple.com>  2010-06-20 12:48:55 PST ---
(From update of attachment 59196)
> +            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.

> +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.

> +    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.

> +    static bool checkedLocale = false;
> +    static bool localeIsEnglish = false;
> +    if (!checkedLocale)
> +        localeIsEnglish = !strcmp("en", currentSearchLocaleID());
> +    if (!localeIsEnglish)
> +        return 0;

Why not just this?

    static bool localeIsEnglish = !strcmp("en", currentSearchLocaleID());

Seems better to me than the two booleans.

> Index: WebCore/rendering/InlineTextBox.cpp
> ===================================================================
> --- WebCore/rendering/InlineTextBox.cpp	(revision 61502)
> +++ WebCore/rendering/InlineTextBox.cpp	(working copy)
> @@ -1,7 +1,7 @@
>  /*
>   * (C) 1999 Lars Knoll (knoll at kde.org)
>   * (C) 2000 Dirk Mueller (mueller at kde.org)
> - * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Library General Public
> @@ -103,6 +103,16 @@ RenderObject::SelectionState InlineTextB
>      return state;
>  }
>  
> +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?

> -    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.

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

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

The line breaking code looks right to me. I think the test cases are the important bit here.

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.