[webkit-reviews] review granted: [Bug 134376] Remove BufferForAppendingHyphen : [Attachment 233954] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 134376: Remove BufferForAppendingHyphen
https://bugs.webkit.org/show_bug.cgi?id=134376

Attachment 233954: Patch
https://bugs.webkit.org/attachment.cgi?id=233954&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233954&action=review


> Source/WebCore/rendering/InlineTextBox.cpp:4
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2014 Apple
Inc. All rights reserved.

We can use year ranges, so this should be 2004-2011, 2014, or maybe even
2004-2014 if we made changes in 2012 and 2013.

> Source/WebCore/rendering/InlineTextBox.cpp:257
> +    if (!string.isEmpty()) {

Why are you adding this check? The old code doesn’t check for this case. If you
are fixing a bug, then should we add a test case for that bug?

> Source/WebCore/rendering/InlineTextBox.cpp:259
> +	   string = string + hyphenString;

Why not use string.append(hyphenString) instead of string = string +
hyphenString?

Irritating to have to do this memory allocation.

> Source/WebCore/rendering/InlineTextBox.h:-29
> -#include <wtf/text/StringBuilder.h>

Looks like TextIterator.cpp, and maybe other files, now needs to include
StringBuilder.h.