[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 bugzilla-request-daemon at opendarwin.org
Sat Mar 4 11:40:21 PST 2006

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

Attachment 6834: new patch to address Justin's comments

------- 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

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

+    HTMLAnonymousDivElementImpl(DocumentImpl *doc, NodeImpl* shadowParent =

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

+    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

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?

More information about the webkit-reviews mailing list