[webkit-reviews] review canceled: [Bug 84646] Implement DocumentFragment.innerHTML : [Attachment 138447] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has canceled Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 84646: Implement DocumentFragment.innerHTML
https://bugs.webkit.org/show_bug.cgi?id=84646

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

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


Clearing r? since I don't think you're looking for a formal review.

> Source/WebCore/dom/DocumentFragment.cpp:104
> +    if (!document()->isHTMLDocument()) {
> +	   ec = INVALID_STATE_ERR;
> +	   return;
> +    }

The last time I checked, folks on public-webapp wanted to be able to parse both
HTML and SVG properly.
If we're going without a proper SVG support, then I'd like to know why.

Also we should do a feature-announcement for this since this is a pretty
significant change to the DOM API.

> Source/WebCore/dom/DocumentFragment.cpp:107
> +    RefPtr<DocumentFragment> fragment = createFragmentFromSource(html,
document(), 0 /* contextElement */, ec);
> +    if (fragment)

WebKit style is to declare fragment inside the if's parenthesis or exit early
when !fragment.
Also, we don't normally add inline comments like /* contextElement */.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1470
> +	   if (isCaptionColOrColgroupTag(token.name()) ||
isTableBodyContextTag(token.name()))
> +	       setInsertionMode(InTableMode);
> +	   else if (token.name() == trTag)
> +	       setInsertionMode(InTableBodyMode);
> +	   else if (isTableCellContextTag(token.name()))
> +	       setInsertionMode(InRowMode);
> +	   else
> +	       setInsertionMode(InBodyMode);

It seems like a bad idea to modify insertion mode like this. Can't we by-pass
the code where the insertion mode is checked?