[webkit-reviews] review denied: [Bug 6893] REGRESSION: Major bug with TinyMCE, no value submitted from textarea : [Attachment 6695] patch

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Fri Feb 24 19:54:44 PST 2006

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6893: REGRESSION: Major bug with TinyMCE, no value submitted from textarea

Attachment 6695: patch

------- Additional Comments from Darin Adler <darin at apple.com>
In new code, please use String instead of DOMString. DOMString is a synonym
allowed for backward compatibility, but should not be used in anything new.

Since HTMLTextAreaElementImpl::attach and HTMLTextAreaElementImpl::detach no
longer do any special work and just call the base class, we should remove them

Why change notification to aNotification in textDidChange?

Comment says that turning \r into \n is "just for good measure". But isn't it
important to do it for form uploading so that all line breaks are the same?

+	 [string replaceOccurrencesOfString:@"\r" withString:@"\n"
options:NSLiteralSearch range:range];
+	 [string replaceOccurrencesOfString:@"\r\n" withString:@"\n"
options:NSLiteralSearch range:range];

The above code will change \r\n into \n\n, right? We need to do the \r\n case

Why add the blank lines after calling textDidChangeInTextArea?

Shouldn't we do the "rangeToNormalize" logic before calling

It's fragile to count on the fact that rangeToNormalize is still a good range
-- if we're off by a character we'll get an Objective-C exception. Are we
certain the text field can't change between where rangeToNormalize is set and
where it's used?

Now that newlineSet is not a character set any more, maybe we can just use
plain old "find" calls instead of an NSScanner, but I suppose it's not
important if this code is going away.

Setting review- because of the "\r\n" after "\r" issue and normalizing before
calling textChanged() on the widget.

More information about the webkit-reviews mailing list