[webkit-reviews] review denied: [Bug 4582] Editing delegates need more selection state : [Attachment 3766] 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 3766: patch
http://bugzilla.opendarwin.org/attachment.cgi?id=3766&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great; this is going to be an excellent improvement to the API. I have a
lot of comments.

--- Semantics

A) I'm not sure I understand the rule about m_extended and the concept of how
SelectionData handles the extending. Why does a newly constructed SelectionData
have m_extended set to true? Is the base range passed in actually an
"already-extended" range? Is the fact that the extended range is computed at
the time you get the range a feature of the class, and if so, why do we ever
store the extended range, since that gives a different semantic? Can we do away
with m_extended entirely?

B) Reading the SelectionData code closely, it looks like the class will behave
differently if you pass in a "detached" range than if you pass in NULL for the
range. If you pass in a detached range, you'll actually get assertion failures
later in toSelection(). I think it's worth a bit more thought to make this edge
case more well defined. Are detached ranges allowed as parameters or not? If
not, lets assert when they are passed in. If so, then what's the behavior when
we have one.

C) The SelectionData constructors that take a RangeImpl * adopt that range.
That's different from what setBaseRange does. I'm not sure the difference is a
good idea. Also, because of this subtlety, if we aren't going to change it, I
suggest marking the SelectionData constructor that takes just a RangeImpl *
explicit so it can't be used implicitly for type conversion.

D) Since SelectionData has to implement its own copy constructor, then it
either needs an implementation of the assignment operator, or to declare it and
not implement it. To be specific, the assignment operator would result in two
SelectionData objects that both hold a pointer to the same base range, while
the copy constructor would copy the base range.

E) I don't understand how the "getDocument != NULL" assertion inside
SelectionData::toSelection is guaranteed. It seems to me that we could easily
run into a situation where a selection object outlasts the document it was made
from, and thus we need defined behavior. We should only use ASSERT for things
we know will be true for some reason, for example because it's a caller
responsibility or an impossible situation prevented by the structure of the
code. Otherwise we have to consider the exceptional case and decide what our
behavior will be. And I think this is a case like that.

F) There's a difference between a function that returns a range that the caller
owns and can modify and a function that returns a range that you need to copy
if you want to modify it. We need to document which of these functions has
which semantic. This applies to both SelectionData and WebSelection because of
the nature of the DOMRange class.

--- Naming

G) It seems that khtml::SelectionData should really be named khtml::Selection
for the same reason that WebSelection has its name; we should look for another
name for the current khtml::Selection class and at least propose it at the time
of this patch. Actually doing the renaming could be postponed, but having a
planned new name for the classes might influence the names of some functions

H) As long as we're adding new source files, we should follow the new standard
of naming files after the classes in them in mixed case. I know the old code
doesn't match that, but we're planning on renaming the old files soonish. So it
should be "SelectionData.h" and "SelectionData.cpp".

I) Do we really need the word "Text" in WebTextSelectionDirection and
WebTextSelectionGranularity? I think that these concepts apply equally well for
non-text selections in a WebView so we can probably just call them
WebSelectionDirection and WebSelectionGranularity.

--- Factoring and organization

J) Leaving the EDirection enum inside the Selection class creates a situation
where the SelectionData's header file has to include Selection's header file.
That's really unfortunate, since Selection is a more-complex class that has to
include many other headers. I think we'd be way better off if we made a global
ESelectionDirection and put it in its own header file as with affinity.

If this is done, then the SelectionData header can get away with just
forward-declaring the Selection class. In fact, the only reason it needs to
declare it at all is the "toSelection()" function, and I don't think that needs
to be a member function since it can be implemented entirely with
SelectionData's public interface.

K) I don't think the setSelectionInPart function makes logical sense as a
member function of SelectionData. It makes sense to have the function, but
perhaps it should be either a free-standing function or a member of another
class. Same comment for WebSelection. In this case, we should let the method on
the bridge be the one that pulls the SelectionData out of the WebSelection and
calls the appropriate C++ and omit the -[WebSelection
_setSelectionInPart:closeTyping:] method.

--- Other code details

L) Since SelectionData is in the khtml namespace, it shouldn't use the khtml
prefix in the declaration of khtml::Selection for the return type of
toSelection().

M) I don't see the point in having a getter for SelectionData::extended() if
it's private. The class should just use the m_extended variable directly.

N) I don't understand the purpose of the line of code that says:

    class DOM::RangeImpl;

in the selectiondata.h header. I believe that this syntax does not work to
forward-delare a class. So either we're including a header that
forward-declares the class already and don't need this line, or we need to use
the syntax that does work for forward declaration which is this:

    namespace DOM {
	class RangeImpl;
    }

Same comment about "class khtml::Selection" inside selectiondata.cpp.

O) The include guard macro in selectiondata.h has the name WEBSELECTION_H in
it; needs to be fixed.

P) There's no need for selectiondata.h to include "text_granularity.h" if it's
also including "selection.h". In general we'd like the list of includes to be
minimal.

Q) This line of code:

    m_baseRange = baseRange ?
SharedPtr<RangeImpl>(baseRange->cloneRange(exceptioncode)) :
SharedPtr<RangeImpl>();

can be written as:

    m_baseRange = baseRange ? baseRange->cloneRange(exceptioncode) : NULL;

and I think it reads a little better. Don't you think?

Similarly, this:

    if (m_baseRange.isNull())
	return SharedPtr<RangeImpl>();

can be written as this:

    if (!m_baseRange)
	return NULL;

In general, the "isNull()" function is deprecated these days. Although if you
think it makes the code read better, then it's fine to keep using it.

R) We usually leave out the space in lines like this one:

    if (! rangeCollapsed)

and write them like this:

    if (!rangeCollapsed)

S) The WebSelection class needs a "designated initializer". See the guidelines
at
<http://developer.apple.com/documentation/Cocoa/Conceptual/CocoaObjects/Article
s/ObjectCreation.html>. I'd suggest that all the other initializers call
initWithBaseRange:affinity:direction:granularity: and that one call [super
init] rather than [self init].

T) -[WebSelectionInternal init] should probably call [super init];

U) -[WebSelectionInternal finalize] needs to call [super finalize].

WebSelectionInternal.h should forward-declare khtml::SelectionData rather than
including the header.

V) This:

    SharedPtr<RangeImpl> i = makeRange(m_start, m_end);
    return i;

reads better as:

    return makeRange(m_start, m_end);

This:

    SelectionData sel(rangeOfContents(root).get());
    return part()->shouldEndEditing(sel);

reads better as:

    return part()->shouldEndEditing(rangeOfContents(root).get());

This:

    khtml::SelectionData sd = _part->selectionData();
    return [WebSelection _selectionWithSelectionData:&sd];

reads better as:

    return [WebSelection _selectionWithSelectionData:&_part->selectionData()];

W) Since the WebSelection private API is all Objective-C++, maybe the
SelectionData parameters should be const & instead of const * typed to help
make it clearer that it's copying the data and not keeping a pointer.

X) We don't need to use WebDefaultEditingDelegate if we're doing explicit
respondsToSelector: checks. Instead it's fine to just have the callers return
YES. The reason WebDefaultEditingDelegate exists is for the
_editingDelegateForwarder technique, and if we aren't using that technique with
a particular method, then it's best to leave it out of the default delegate. I
think we will retire the forwarder technique soon; it doesn't save much code or
read particularly nicely and it's considerably slower than an explicit check
for the common case of a delegate that does not implement a particular method.