Bug 29292

Created attachment 39679 [details] Proposed patch

Comment on attachment 39679 [details] Proposed patch > + if (!hasAttribute(maxlengthAttr)) > + return; > + bool ok; > + unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok); > + if (!ok) > + return; It's a shame to do two hash table lookups here for the maxlength attribute. And there's no reason for that check. toUInt will give false for "ok" when maxlengthAttr is null, so I believe the hasAttribute check is unnecessary extra work. > + int currentLength = InputElement::numGraphemeClusters(toRenderTextControl(renderer())->text().impl()); > + int selectionLength = InputElement::numGraphemeClusters(plainText(document()->frame()->selection()->selection().toNormalizedRange().get()).impl()); I think these functions should just be public functions, perhaps in PlatformString.h, rather than members of the InputElement class. It also seems inconvenient to have to pass them StringImpl* rather than just a String. As we make these functions more public it makes sense to me to make them more normal functions and less special-purpose. > + if (newLength < static_cast<int>(proposedValue.length())) { > + String string = proposedValue; > + return string.left(newLength); > + } > + return proposedValue; This can just be written: return proposedValue.left(newLength); The left function already has optimization for the case where the length is >= the length of the string, and you don't need to redo it. > + bool ok; > + unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok); > + return ok ? maxLength : 0; toUInt already guarantees it will return 0 if the string can't be parsed, so you can just write: return getAttribute(maxlengthAttr).string().toUInt(); > + static String sanitizeUserInputValue(const String&, int); It's not good to leave out the argument name here for the int. It's clear what this int would be. So it needs a name. I'm going to say review- because I think at least some of my suggestions above really ought to be done.

Created attachment 39745 [details] Proposed patch (rev.2)

(In reply to comment #2) Thank you for the helpful comments. > (From update of attachment 39679 [details]) > > + if (!hasAttribute(maxlengthAttr)) > > + return; > > + bool ok; > > + unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok); > > + if (!ok) > > + return; > > It's a shame to do two hash table lookups here for the maxlength attribute. And > there's no reason for that check. toUInt will give false for "ok" when > maxlengthAttr is null, so I believe the hasAttribute check is unnecessary extra > work. Removed hasAttribute(). > > + int currentLength = InputElement::numGraphemeClusters(toRenderTextControl(renderer())->text().impl()); > > + int selectionLength = InputElement::numGraphemeClusters(plainText(document()->frame()->selection()->selection().toNormalizedRange().get()).impl()); > > I think these functions should just be public functions, perhaps in > PlatformString.h, rather than members of the InputElement class. It also seems > inconvenient to have to pass them StringImpl* rather than just a String. As we > make these functions more public it makes sense to me to make them more normal > functions and less special-purpose. Moved the grapheme functions to String class. > > + if (newLength < static_cast<int>(proposedValue.length())) { > > + String string = proposedValue; > > + return string.left(newLength); > > + } > > + return proposedValue; > > This can just be written: > > return proposedValue.left(newLength); Done. > > + bool ok; > > + unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok); > > + return ok ? maxLength : 0; > > toUInt already guarantees it will return 0 if the string can't be parsed, so > you can just write: > > return getAttribute(maxlengthAttr).string().toUInt(); Done. > > + static String sanitizeUserInputValue(const String&, int); > > It's not good to leave out the argument name here for the int. It's clear what > this int would be. So it needs a name. Added the argument name.

Comment on attachment 39745 [details] Proposed patch (rev.2) > + if (isEmpty()) > + return 0; This special case for empty and null strings is not needed. > + TextBreakIterator* it = characterBreakIterator(characters(), length()); > + if (!it) > + return 0; This is not new code, but it's really non-useful to return 0 here. It would be way better to just return length(). > + if (isEmpty()) > + return 0; This special case for empty and null strings is not needed. > + TextBreakIterator* it = characterBreakIterator(characters(), length()); > + if (!it) > + return 0; This is not new code, but it's really non-useful to return 0 here. It would be better to return min(length(), numGraphemeClusters). r=me as-is, though

Created attachment 39769 [details] Proposed patch (rev.3) Clean up String::numGraphemeClusters() and String::numCharactersInGraphemeClusters() for Comment #5.

Comment on attachment 39769 [details] Proposed patch (rev.3) > + unsigned maxLength = static_cast<unsigned>(data.maxLength()); // maxLength() never be negative. Coding style says one space before the "//". The grammar here is wrong. It could be "can never be negative" or "never is negative" instead. > + unsigned appendableLength = maxLength > baseLength ? maxLength - baseLength : 0; Extra space here after the equal sign. Another way to write this is: max(maxLength, baseLength) - baseLength Your way is probably clearer, though. > + TextBreakIterator* it = characterBreakIterator(characters(), length()); > + if (!it) > + return length(); The value length() is not right here. It should be min(length(), numGraphemeClusters) instead. I'm going to say review+ because realistically characterBreakIterator will not return 0 so that comment is not important.

Comment on attachment 39769 [details] Proposed patch (rev.3) Darin had review comments, so this can't be auto-committed unless you post a new patch.

(In reply to comment #8) > Darin had review comments, so this can't be auto-committed unless you post a > new patch. My comments are optional, though. We could land it as-is if Kent wants to.

Created attachment 39781 [details] Proposed patch (rev.4) > > + unsigned maxLength = static_cast<unsigned>(data.maxLength()); // maxLength() never be negative. > Coding style says one space before the "//". Fixed. > The grammar here is wrong. It could be "can never be negative" or "never is > negative" instead. Fixed. > > + unsigned appendableLength = maxLength > baseLength ? maxLength - baseLength : 0; > Extra space here after the equal sign. Fixed. > > + TextBreakIterator* it = characterBreakIterator(characters(), length()); > The value length() is not right here. It should be min(length(), > numGraphemeClusters) instead. I don't think so. Suppose that the string is "\ud800\udc00", of which length() is 2 and has 1 grapheme. numCharactersInGraphmeClusters(1) with this string should return 2. If we specified min(length(), nmGraphemeClusters) to characerBreakIterator(), the result would be 1.

(In reply to comment #10) > Suppose that the string is "\ud800\udc00", of which length() is 2 and has 1 > grapheme. numCharactersInGraphmeClusters(1) with this string should return 2. > If we specified min(length(), nmGraphemeClusters) to characerBreakIterator(), > the result would be 1. But the whole point is that when the iterator can't be created we have to assume that each character is a separate grapheme cluster because we don’t know any better. Suppose that the string is "ab". numCharactersInGraphemeClusters(1) should return 1, not 2. If we specify length(), the result would be 2. Obviously we can’t handle things correctly without the character iterator, but it seems clear that treating the entire string as one giant grapheme cluster is not the way to go. And that’s what returning length() does.

Created attachment 39807 [details] Proposed patch (rev.5) > Suppose that the string is "ab". numCharactersInGraphemeClusters(1) should > return 1, not 2. If we specify length(), the result would be 2. ok, I changed it to: unsigned String::numCharactersInGraphemeClusters(unsigned numGraphemeClusters) const { TextBreakIterator* it = characterBreakIterator(characters(), length()); if (!it) return min(length(), numGraphemeClusters);

Created attachment 39855 [details] Proposed patch (rev.6) resources/ -> script-tests/

Comment on attachment 39745 [details] Proposed patch (rev.2) Removing Darin's r+ from this obsolete patch.

Comment on attachment 39769 [details] Proposed patch (rev.3) Removing Darin's r+ from this obsolete patch.

Comment on attachment 39855 [details] Proposed patch (rev.6) Darin reviewed the previous patch, so it should be easy for him to r+ this one. I'd like to leave it for him to do that. I would need to read up to make sure that your numCharactersInGraphemeClusters changes are OK.... especially since they're not explained in the ChangeLog. :(

Comment on attachment 39855 [details] Proposed patch (rev.6) Clearing flags on attachment: 39855 Committed r48698: <http://trac.webkit.org/changeset/48698>

The patch was landed.

This broke Facebook. See bug 29922.