[Webkit-unassigned] [Bug 24342] [Chromium] cannot insert a Thai character after a Thai prepend character

[bugzilla-daemon at webkit.org]

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


ap at webkit.org changed:

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




------- Comment #3 from ap at webkit.org  2009-03-04 00:17 PDT -------
(From update of attachment 28254)
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.


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