[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
character
https://bugs.webkit.org/show_bug.cgi?id=24342

Attachment 28254: A proposed fix
https://bugs.webkit.org/attachment.cgi?id=28254&action=review

------- 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"
for
> +	   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
iterator?

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.

+#if PLATFORM(CHROMIUM)
+    TextBreakIterator* codepointBreakIterator(const UChar*, int length);
+#endif

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.
+    //
http://source.icu-project.org/repos/icu/icu/tags/release-3-8/source/data/brkitr
/char.txt

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:
LayoutTests/editing/inserting/insert-thai-characters-001.html
> ___________________________________________________________________
> Name: svn:executable
>    + *

Please don't make new files executable.


More information about the webkit-reviews mailing list