[webkit-reviews] review denied: [Bug 21712] Indent on li creates new ol instead of merging with existing ol, numbering incorrect. : [Attachment 31700] The patch for IndentOutdentCommand with 10 tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at google.com>'s
request for review:
Bug 21712: Indent on li creates new ol instead of merging with existing ol,
numbering incorrect.
https://bugs.webkit.org/show_bug.cgi?id=21712

Attachment 31700: The patch for IndentOutdentCommand with 10 tests
https://bugs.webkit.org/attachment.cgi?id=31700&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this!

> +	       Element* enclosingListItem =
static_cast<Element*>(enclosingListChild(endOfCurrentParagraph.deepEquivalent()
.node()));

It's dangerous to do a typecast. You have to have some reason to know the type
is correct. If enclosingListChild guarantees it returns an Element*, then your
patch should change the function's return type. If it does not guarantee that,
then you need a type check before casting to Element*. Or it's possible that
enclosingListChild doesn't always return an Element*, but for some reason you
know that in this case it does.

> +	       Element* previousListItem =
enclosingListItem->previousElementSibling();
> +	       Element* nextListItem = enclosingListItem->nextElementSibling();


These element sibling functions will skip over intervening text nodes. That
means there could be an arbitrary amount of text, perhaps multiple pages of
text, between these list items. In that case, the behavior of this code seems
incorrect; list items will hop across text when indenting. You should make a
test case like this and I think then you'll see this code needs to be changed.

> +	       QualifiedName listTag = listNode->tagQName();

It's more efficient to have this local be a const QualifiedName&.

> +		   appendNode(listItem, (Element*)previousListItem);

The typecast here is currently unnecessary. Please remove it. But probably to
fix the issues above you'll change the code further. Also, it should be
static_cast, not a C++-style cast.

> +		   newListNode = (Element*)previousListItem;

Ditto.

> +	       }
> +	       else if (nextListItem && nextListItem->hasTagName(listTag)) {

The else should be on the same line with the closing brace.

> +		   insertNodeAt(listItem, Position((Element*)nextListItem, 0));


Unnecessary typecast.

> +		   newListNode = (Element*)nextListItem;

Ditto.

> +	       }
> +	       else {

The else should be on the same line with the closing brace.

I'm also not entirely certain we want this editing behavior in all contexts. I
guess we probably do. Someone here at Apple should check to make sure we want
this behavior in Apple's Mail application.

review- because of the issues mentioned above