[webkit-reviews] review denied: [Bug 4682] -[WebHTMLView firstRectForCharacterRange:] is using _selectedRange instead of the given range if no marked text : [Attachment 4835] proposed patch

[bugzilla-request-daemon at opendarwin.org]

Justin Garcia <justin.garcia at apple.com> has denied Darin Adler
<darin at apple.com>'s request for review:
Bug 4682: -[WebHTMLView firstRectForCharacterRange:] is using _selectedRange
instead of the given range if no marked text
http://bugzilla.opendarwin.org/show_bug.cgi?id=4682

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

------- Additional Comments from Justin Garcia <justin.garcia at apple.com>
>From -[WebHTMLView firstRectForCharacterRange]:

+    // Just to match NSTextView's behavior. Regression tests cannot detect
this;
+    // to reproduce, use a test application from
http://bugzilla.opendarwin.org/show_bug.cgi?id=4682
+    // (type something; set start=1, length=-1).
+    if ((theRange.location + theRange.length < theRange.location) &&
(theRange.location + theRange.length != 0))
+	 theRange.length = 0;

It sounds like you're saying that start=1 length=-1 is a case where this
special case code is needed.  But if that's true, why the && (theRange.location
+ theRange.length != 0)?  (1 + -1 == 0).

Also I think that:
theRange.length < 0
is clearer than:
theRange.location + theRange.length < theRange.location

Also I think that:
+    if (!range)
+	 return NSMakeRect(0,0,0,0);
Should probably be:
+    if (!range || ![range startContainer])
+	 return NSMakeRect(0,0,0,0);
You took out the check for ![range startContainer] and added !range, so perhaps
you have a good reason for not including ![range startContainer].

>From [WebCoreBridge convertNSRangeToDOMRange:]:

+    if (nsrange.location > INT_MAX)
+	 return 0;
+    if (nsrange.length > INT_MAX || nsrange.location + nsrange.length >
INT_MAX)
+	 nsrange.length = INT_MAX - nsrange.location;
+

I think that this would be more appropriate done in -[WebCoreBridge
convertToDOMRange], since that is where the NSRange is given to a function that
expects signed ints.

r- for now.