[webkit-reviews] review denied: [Bug 6987] Implement maxlength for new text fields : [Attachment 6834] new patch to address Justin's comments

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied 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 6834: new patch to address Justin's comments
http://bugzilla.opendarwin.org/attachment.cgi?id=6834&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
New top-level classes should be in separate files, even though we didn't split
render_block.cpp yet. So HTMLAnonymousDivElementImpl should be in its own file
as should InsertTextEventImpl.

We might be able to come up with a better name for the
HTMLAnonymousDivElementImpl class too. Since its implementation has a special
case for the case where its parent has a RenderTextField for a renderer, it
should probably have a name and location specific to its use instead of
pretending to be more general purpose. For example, it could go in the
rendering directory even though it's a subclass of a DOM element class, since
it's used as part of form element rendering.

HTMLAnonymousDivElementImpl::defaultEventHandler checks if the parent's
renderer is a text field, but it doesn't check if its parent is an
HTMLInputElementImpl -- instead it assumes that. That should be checked too.

As we discussed, khtmlInsertTextEvent does not need to be an
InsertTextEventImpl.

There's no need to add an unused isAnonymousDiv() function.

+    HTMLAnonymousDivElementImpl(DocumentImpl *doc, NodeImpl* shadowParent =
0);

Should just be DocumentImpl*. No need to give a name.

+    virtual void defaultEventHandler(EventImpl *evt);

Similarly, should just be EventImpl*, no need for "evt".

In HTMLInputElementImpl, I'd like to see the enforcement of range from 0 to
1024 be when m_maxLen is set, not in the maxLength() accessor.

+	 return (m_div->textContent());

No need for parentheses on a return statement like this.

     macro(khtmlHorizontalmousewheel) \
+    macro(khtmlBeforeInsertText) \
+    macro(khtmlInsertText) \
     macro(khtmlMove) \

The other events are in alphabetical order, so you should move BeforeInsertText
up.

+    InsertTextEventImpl(const AtomicString &);
+    InsertTextEventImpl(const AtomicString &, String);

No spaces before the & characters.

+    virtual void setText(String);

Why is this function virtual? Also, it should take a const String&, not a
String.

We might want to consider a version of the BeforeInsertText event that sends a
DOM fragment through instead of a string -- it seems a bit limiting to have
this paste preflighting be specific to plain text pasting -- but perhaps the
plain text one is easier to implement for now.

You should undo the change to ReplaceSelectionCommand.h.

The code in HTMLAnonymousDivElementImpl::defaultEventHandler calls
subtreeHasChanged *before* calling the default handler. That seems like it's
too early -- will the insertion be done at that point? If this event is fired
only after text is inserted then perhaps it should be named InsertedText,
TextInserted, or AfterInsertText?