[webkit-reviews] review denied: [Bug 31680] WebCore::Document::updateLayoutIgnorePendingStylesheets NULL pointer : [Attachment 51994] v2; add a guard on setBaseAndExtent()

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 31680: WebCore::Document::updateLayoutIgnorePendingStylesheets NULL pointer
https://bugs.webkit.org/show_bug.cgi?id=31680

Attachment 51994: v2; add a guard on setBaseAndExtent()
https://bugs.webkit.org/attachment.cgi?id=51994&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 The fix also adds a null guard for canonicalPosition() just in case.

We don't usually add null checks just in case, especially in performance
sensitive code. These can confuse people looking at the code to think that this
can legitimately happen.

You could add an ASSERT - it would be useless for catching errors, because we
crash soon enough anyway, but it would serve as documentation that nodes are
supposed to have a non-null document here.

+    // Because we don't know how to "select" ownerless nodes, we take them as
null.

I was going to suggest raising an exception instead (probably
INVALID_ACCESS_ERR). I'm not sure if that's a good idea. But it made me think
about nodes from different documents. Is there a check somewhere that we don't
set the selection base and extent to nodes from another document?

+It is OK not to crash.

A better way to say this would be "PASSED if didn't crash".