[webkit-reviews] review denied: [Bug 24342] [Chromium] cannot insert a Thai character after a Thai prepend character : [Attachment 28254] A proposed fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 4 00:17:42 PST 2009

Alexey Proskuryakov <ap at webkit.org> has denied Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 24342: [Chromium] cannot insert a Thai character after a Thai prepend

Attachment 28254: A proposed fix

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
It is definitely appropriate to fix this for all platforms that use ICU. Are
all the ICU functions used here available in at least ICU 3.2?

> +	   This change creates a new break iterator "codepointBreakIterator"
> +	   inputting characters.

The name "codepointBreakIterator" is extremely misleading. I would expect it to
move by a code point (i.e. one or two UTF-16 code units).

What is this new iterator actually used for? It's called from
RenderText::previousOffset() and RenderText::nextOffset(), which are used for
far more than just typing text. Is it perhaps a general cursor movement

Please note that cursor movement conventions may be different on each platform,
so we should clearly document what this iterator does conceptually, so that
platform-specific overrides could be added safely.

+    TextBreakIterator* codepointBreakIterator(const UChar*, int length);

As mentioned by Mark, this shouldn't be Chromium-specific.

 #include "TextBreakIteratorInternalICU.h"
+#include "AtomicString.h"

Please keep include lists sorted. But I do not think you need AtomicString
here. The rules string is only used once, and it is immediately destroyed.

+    if (!iterator)
+      return 0;

Incorrect indentation here.

+    if (U_FAILURE(setTextStatus))
+      return 0;

And here.

+    // This rule is based on the character-break iterator rules of ICU 3.8.
+    //

As mentioned above, the name "codepointBreakIterator" is not appropriate. It's
good to tell where these rules come from, but this comment should also expand
on what they are expected to do.

+    return setUpIteratorWithRules(createdCodepointBreakIterator,
+	 staticCodepointBreakIterator, kRules, string, length);

There is no need to break this line, it's quite short.

> Property changes on:
> ___________________________________________________________________
> Name: svn:executable
>    + *

Please don't make new files executable.

More information about the webkit-reviews mailing list