[webkit-reviews] review granted: [Bug 6987] Implement maxlength for new text fields : [Attachment 6878] patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 6987: Implement maxlength for new text fields
http://bugzilla.opendarwin.org/show_bug.cgi?id=6987

Attachment 6878: patch
http://bugzilla.opendarwin.org/attachment.cgi?id=6878&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
One major difference between this maxLength implementation and the one I did in
KWQTextField is that this one limits you to a certain number of UTF-16
characters. But the one in KWQTextField limits you to a certain number of
"composed character sequences". That means that an e with an umlaut over it
counts as 1 character even though it can be two Unicode characters in a row
(the e followed by the non-spacing umlaut) and a single Japanese character that
is outside the "BMP" that requires two UTF-16 codes (a "surrogate pair") to
encode also counts as a single character.

The code that deals with this in KWQTextField is
_KWQ_numComposedCharacterSequences and
_KWQ_truncateToNumComposedCharacterSequences:.

We will need to replicate this, although I guess it's fine not to at first, but
I'd like to see another bug about that.

To tell if a character is half of a surrogate pair, you use macros in
<unicode/utf16.h>, such as U16_LENGTH or U16_IS_LEAD. To tell if the character
is going to combine with the one before it is more difficult. There's code in
CoreFoundation that does this analysis and I presume there's some way to do it
with ICU, but I don't know what that is.

In addition to determining such things, code will have to be careful not to do
math on the length of strings, since composing means that "length of A plus
length of B" is not necessarily the same as "length of A plus B".

HTMLTextFieldInnerElementImpl does not need an explicit destructor. The
compiler-generated destructor will do the job. So don't declare or define a
destructor.

+void HTMLTextFieldInnerElementImpl::defaultEventHandler(EventImpl *evt)

The above should have the * next to the EventImpl.

The various files that say:

\ No newline at end of file

should be fixed so they have a newline at the end of the file.

RenderTextField.h should not include HTMLTextFieldInnerElementImpl.h. To
compile, all it needs is a forward declaration of the class. The .cpp file can
include the complete header.

     if (m_div) {
-	 m_div->removeEventListener(DOMCharacterDataModifiedEvent,
m_mutationListener.get(), false);
	 m_div->detach();
     }

Should remove the braces from the above.

+	     m_div = new HTMLTextFieldInnerElementImpl(document(), element());

node() is more efficient than element() in cases like the above.

There's no need to include "PlatformString.h" in BeforeTextInsertedEventImpl.h
-- dom2_eventsimpl.h contains classes with String in them, and because of that
it has the class defined already.

+    BeforeTextInsertedEventImpl(String&);

That should be "const String&".

In the ReplaceSelectionCommand code to send BeforeTextInsertedEvent, I notice
that you compute the text before you check if there's a frame. That's
unnecessary and a little bit unclear. I suggest computing the range and the
text right when you are about to use it.

I'd also like to see you declare "ec" as ExceptionCode instead of int for
clarity and name it ec in both places you use it instead of calling it
exceptionCode in some places.

These issues are minor ones. r=me