[webkit-reviews] review granted: [Bug 191335] Add an editing command for creating and inserting child lists : [Attachment 354128] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 191335: Add an editing command for creating and inserting child lists
https://bugs.webkit.org/show_bug.cgi?id=191335

Attachment 354128: Patch

https://bugs.webkit.org/attachment.cgi?id=354128&action=review




--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 354128
  --> https://bugs.webkit.org/attachment.cgi?id=354128
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354128&action=review

> Source/WebCore/editing/EditorCommand.cpp:1600
> +	   { "InsertOrderedChildList", { executeInsertOrderedChildList,
supported, enabledInRichlyEditableText, stateOrderedList, valueNull,
notTextInsertion, doNotAllowExecutionWhenDisabled } },

It's a bit strange to call this editor command insert * child list since it can
insert the outermost list as well.
How about InsertNestedOrderedList? Also, is it expected that this feature is
exposed to the Web via execCommand?

> Source/WebCore/editing/InsertChildListCommand.cpp:52
> +    auto& document = this->document();

Do we really need this local variable? Calling document() in each place where
it's used seems fine.
If we're using a local variable, I'd prefer having a Ref/RefPtr here instead of
a reference here.
Who knows what happens to m_document.

> Source/WebCore/editing/InsertChildListCommand.cpp:58
> +	   setEndingSelection({{ newListItem.ptr(),
Position::PositionIsBeforeChildren }, DOWNSTREAM });

This is a bit cryptic why don't we explicitly say Position like so:
{Position { newListItem.ptr(), Position::PositionIsBeforeChildren }, DOWNSTREAM
}

>
LayoutTests/editing/execCommand/list-insertion-with-child-lists-expected.txt:94
> +|	     <li>

Let's add test cases for when a list appears inside a table cell inside a list,
as well as a list inside pre, or the content you're listening is a pre.

> LayoutTests/editing/execCommand/list-insertion-with-child-lists.html:16
> +    [..."foo"].map(typeCharacterCommand);

Haha, this is pretty neat.