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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 111035: Patch
https://bugs.webkit.org/attachment.cgi?id=111035&action=review

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


Please consider making trimFragment take PassRefPtr but this patch is probably
landable as is.

> Source/WebCore/editing/markup.cpp:711
> +static void trimFragment(DocumentFragment* fragment, Node*
nodeBeforeContext, Node* nodeAfterContext)

This function should probably take PassRefPtr to nodes.

> Source/WebCore/editing/markup.cpp:721
> +	   node->parentNode()->removeChild(node.get(), ec);

I'd add an assertion that node doesn't contain nodeAfterContext.

> Source/WebCore/editing/markup.cpp:734
> +PassRefPtr<DocumentFragment> createFragmentFromMarkupWithContext(Document*
document, const String& markup, unsigned fragmentStart, unsigned fragmentEnd,
const String& baseURL, FragmentScriptingPermission scriptingPermission)

Nit: Probably split this into two lines.

> Source/WebCore/editing/markup.cpp:763
> +    if (!(nodeBeforeContext && nodeAfterContext))

I think !nodeBeforeContext || ! nodeAfterContext looked better.