[webkit-reviews] review granted: [Bug 111228] InsertUnorderedList can lead to lost content and assertions in moveParagraphs : [Attachment 191630] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 111228: InsertUnorderedList can lead to lost content and assertions in
moveParagraphs
https://bugs.webkit.org/show_bug.cgi?id=111228

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

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


Please fix the test before you land it.

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:10
> +function styleList() {

styleList is a vague name.

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:11
> +    var newElem = document.createElement('div');

Please don't abbreviate element as elem.

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:14
> +    var textNode = document.createTextNode('x');
> +    newElem.appendChild(textNode);

We could do this in one line.

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:15
> +    root.insertBefore(newElem, root.firstChild);

Where is root coming from?

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:22
> +		testRunner.dumpAsText();

Nit: tab. cq-.

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:26
> +	root.offsetHeight;

Why do we need to force layout here? SelectAll is going to do that anyways

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:34
> +	finalMarkup = root.innerHTML;
> +	listIsRemoved = finalMarkup.indexOf("ul") == -1;

Why don't you just check document.getElementsByTagName('ul').length == 0
instead?

>
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-
inline.html:38
> +		root.parentNode.removeChild(root);

Nit: tab.