[webkit-reviews] review denied: [Bug 4582] Editing delegates need more selection state : [Attachment 3513] patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Duncan Wilcox <duncan at mclink.it>'s
request for review:
Bug 4582: Editing delegates need more selection state
http://bugzilla.opendarwin.org/show_bug.cgi?id=4582

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

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like a good direction, and pretty good patch. Here is a list of comments
based on my read-through of the patch:

1) Since "Web" is the prefix, analogous to "NS", the functions to create
WebSelection can just be called, e.g., "selectionWithBaseRange" rather than
"webSelectionWIthBaseRange".

2) I'm not sure comfortable with the "expanded range" terminology -- "expand
using granularity" was an internal name until this point, and not an API
concept. How is this notion handled in NSTextView? Is there a better name for
this? Can this just be called the "range" without an adjective? Or will that be
even more confusing?

3) webSelectionWithBaseRange needs to autorelease its result.

4) The setBaseRange: method should use the "retain before release" idiom rather
that doing the "!=" comparison. Same for the other setter.

5) Please don't include commented-out printf statements.

6) I think the direction() method of khtml::Selection should be a boolean with
a descriptive name rather than having a special meaning for 0 vs. 1. Given the
name direction() it's not clear, for example, whether it would be 0 vs. 1 or -1
vs. 1 or something else.

7) The import of "WebViewPrivate.h" should not be right up against the
declaration that follows it (needs a blank line).

8) The WebEditingDelegatePendingPublic should be in a
WebEditingDelegateInternal.h header. The current patch has it appearing in two
places.

9) The big ? : expression in shouldChangeSelectionTo would be easier to read if
it was written as an if statement. Also, I'd like to see the editingDelegate in
a local variable. Finally, if we're going to be doing one respondsToSelector
call, I suggest we do two and don't bother with the "forwarder".

10) I don't like the idea that the WebSelection has a separate expanded range
and granularity. When it's created in any way expect when making it from a
khtml::Selection you can end up with an expanded range that is inconsistent
with the claimed granularity, such as in the calls that use unionDOMRange. Is
there something we can do to eliminate this? Or can you prove to me that it's
not a problem?

11) The "shouldChangeSelectedRange" code should be shared between the bridge
and WebView. In general, it's good to have the bridge call over to WebView
methods and remove bridge code.

12) This comment:

+// These replace setSelectedDOMRange:affinity: and selectedDOMRange, in
interface WebView (WebViewEditing)

is slightly misleading, since the old calls will continue to work,
selectionDOMRange (at least) is still useful, and the calls also replace the
ones that get/set affinity and granularity.

This looks to me like it's going to be a great change.