[webkit-reviews] review denied: [Bug 67764] Crash in WebCore::CompositeEditCommand::insertNodeAt : [Attachment 129024] Updated Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Parag Radke <parag at motorola.com>'s
request for review:
Bug 67764: Crash in WebCore::CompositeEditCommand::insertNodeAt
https://bugs.webkit.org/show_bug.cgi?id=67764

Attachment 129024: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=129024&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=129024&action=review


r- due to the bad test description and comment.

> Source/WebCore/editing/CompositeEditCommand.cpp:1037
> +	       // for VisiblePositions V1 and V2, it's possible V1 != V2 but
still editing code treat them as same. 
> +	       // Ref : FIXME @ Position.h :inline bool operator==(const
Position& a, const Position& b) 

I don't understand why the fact two VisiblePositions being treated same is
resulting in this code since that's not true.
I think what you meant is Position. But the fact multiple Positions can be
treated as the same position is a well-known fact and doesn't deserve a
comment.
Instead, you should explain why you're calling rendersInDifferentPosition in
this particular situation.

> LayoutTests/editing/deleting/delete-block-merge-contents-025-expected.txt:1
> +This is for bug-67764,to Pass this testcase it should not crash.

Please explain what the test is doing. Saying that it's for the bug 67764
doesn't explain anything about the nature of the test.