[webkit-reviews] review denied: [Bug 10228] CSS3: Support the hyphenate property : [Attachment 59196] Implement the 'hyphens' and 'hyphenate-character' properties
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jun 20 12:48:54 PDT 2010
Darin Adler <darin at apple.com> has denied mitz at webkit.org's request for review:
Bug 10228: CSS3: Support the hyphenate property
https://bugs.webkit.org/show_bug.cgi?id=10228
Attachment 59196: Implement the 'hyphens' and 'hyphenate-character' properties
https://bugs.webkit.org/attachment.cgi?id=59196&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> + 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.
More information about the webkit-reviews
mailing list