[webkit-reviews] review denied: [Bug 62112] Context-aware HTML paste for Chromium : [Attachment 110209] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 62112: Context-aware HTML paste for Chromium
https://bugs.webkit.org/show_bug.cgi?id=62112

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

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


r- due to the comment that we have failing tests.

>> Source/WebCore/ChangeLog:12
>> +	    Covered by existing tests.
> 
> There are 11 tests which fail on Linux, 1 which fails on Mac, and 1 test case
that crashes (not the scrollbar crash) if it's run in a certain order. I'm
still investigating them.

I can't r+ this patch until I see those failures.

> Source/WebCore/editing/markup.cpp:786
> +    RefPtr<Range> range =
Range::create(pageForPaste()->mainFrame()->document(),
> +	   positionAfterNode(fragmentStartNode).parentAnchoredEquivalent(),
> +	   positionBeforeNode(fragmentEndNode).parentAnchoredEquivalent());

Knowing there was some crash, is it possible that fragmentStart and fragmentEnd
are never found? Also is it possible that fragment start and fragment ends are
before and after html element respectively? In those cases, these lines might
blow up so you may want to have some nullity check here.