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

------- Additional Comments from Darin Adler <darin at apple.com>
Looking good.

Here's another round of comments:

A) Code inside the khtml namespace needn't specify khtml:: when using other
things in the same namespace, so remove all those khtml:: prefixes in the .cpp
files and the .h files.

B) The baseRange parameter should be:

    RangeImpl *baseRange

We use SharedPtr for member variables and return values, but not for
parameters.

C) It's more efficient to initialize variables with the right value than to
initialize them to false and then set them, so I'd prefer that the constructors
not use the set functions, and instead initialize.

D) When checking a SharedPtr for null, it's better to use isNull() or notNull()
rather than get().

E) The formatting should not be:

    if(! xxx

instead it should be:

    if (!xxx

F) Should do "using DOM::" for identifiers you use at the top of the .cpp file
for any type you are using, and not use the DOM:: prefix at each use later on.
This does not apply to .h files.

G) In selectionWithBaseRange:

+    id sel = [[WebSelection alloc] init];

that variable should be of type WebSelection *, not id. Same for the other
creation function. Also, there's no need to check for nil because: 1) init
won't ever return nil in this case, since there's no error case, and 2)
dispatching against nil safely does nothing for all the methods you're calling.


H) The selectionWithXXX methods should be covers for initWithXXX methods that
do the actual work.

I) Besides the dealloc method, you also need a finalize method that does the
same thing, since WebKit works in both GC and non-GC modes. This is only needed
when there are non-ObjC deallocations, such as in the WebSelectionPrivate
class; the finalize methods don't need to make any calls to "release".

J) I slightly prefer to leave out the parentheses on lines like this one:

+    data = new khtml::SelectionData();

K) For things internal to WebKit, we should use the term "Internal" rather than
"Private". "Private" basically means "private to the OS, not for use by
applications". Thus the WebSelectionPrivate.h header should be named
WebSelectionInternal. Also, the WebSelectionPrivate class, whatever its name,
should not be in the header because there's no use of it outside the
implementation file.

L) In Selection::baseRange, the code gets the extent and base and then checks
== with m_end. I think instead it should just get m_start and m_end, no?

M) I'm a little disappointed that this patch adds copied and pasted code. I'd
like to see this changed around so that the code is shared with the place it
was copied from.

N) m_baseIsStart ? true : false is pretty strange. Should just be
"m_baseIsStart".

O) The use of "deprecate" is incorrect in this comment:

+// The following deprecate setSelectedDOMRange:affinity: and complement
selectedDOMRange and
+// selectionAffinity, in interface WebView (WebViewEditing)

It's true that the new methods supplant the old ones, but it's incorrect
grammar to say that a method "deprecates" another method. You should reword the
comment (and I'm not really suggesting you use the word "supplant" :-).

P) Perhaps we should add a selectionDirection accessor to complement
selectionAffinity. It seems like the odd man out at this point.