[webkit-reviews] review denied: [Bug 27038] htmlediting.cpp needs more utility functions to fix the bug 26816 : [Attachment 32402] removed the change to CompositeEditCommand.cpp

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Ryosuke Niwa <rniwa at google.com>'s
request for review:
Bug 27038: htmlediting.cpp needs more utility functions to fix the bug 26816
https://bugs.webkit.org/show_bug.cgi?id=27038

Attachment 32402: removed the change to CompositeEditCommand.cpp
https://bugs.webkit.org/attachment.cgi?id=32402&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Style:
 63	short comparePoint(Node* refNode, int offset, ExceptionCode& ec) const;


I would have used early return:
 215	 Element* enclosingCell = static_cast<Element*>(enclosingNodeOfType(p,
&isTableCell, true));
 216	 // Since enclosingNodeOfType won't search beyond the highest root
editable node,
 217	 // this code works even if the closest table cell was outside of the
root editable node.
 218	 return enclosingCell ? enclosingCell : editableRootForPosition(p);

This could assert ASSERT(node->parentNode()):
 592 VisiblePosition visiblePositionBeforeNode(const Node* node)

Please add a FIXME: to the other createRange:
 612 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const
VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec)

PassRefPtr seems the wrong type for these args:
 627 PassRefPtr<Range> extendRangeToWrappingNodes(PassRefPtr<Range>
rangeToExtend, PassRefPtr<Range> maximumRange, Node* rootNode)

Explicit visiblePosition construction is not needed here:
 1076	  return visiblePositionBeforeNode(node) ==
VisiblePosition(selectedRange.startPosition())
(but it also doesn't hurt)

Style:
 65 Element* unsplittableElementForPosition(const Position& p);
 78 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const
VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec);
 145 bool isNodeVisiblyContainedWithin(Node* node, const Range& selectedRange);


Looks fine.  Normally we don't add unused code, but I can see how this will
help make your future patches smaller.

Please re-submit a fixed version, and then we might wait to land this until the
other patch which depends on these changes is approved to.