[webkit-reviews] review denied: [Bug 179283] Web Inspector: support undo/redo of insertAdjacentHTML : [Attachment 326060] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 179283: Web Inspector: support undo/redo of insertAdjacentHTML
https://bugs.webkit.org/show_bug.cgi?id=179283

Attachment 326060: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 326060
  --> https://bugs.webkit.org/attachment.cgi?id=326060
Patch

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

Nice test! This looks good to me, but I will want to see a version that shares
the Element algorithm. We should never duplicate core algorithms (even if this
seems relative simple).

> Source/JavaScriptCore/inspector/protocol/DOM.json:225
> +	       "name": "insertAdjacentHTML",

I think this would go better down by "setOuterHTML".

> Source/WebCore/dom/Element.cpp:3669
> +ExceptionOr<NodeVector> Element::nodesForInsertedAdjacentHTML(const String&
position, const String& html)

I feel like the algorithm should really be one place. That could be done with
something like:

    ExceptionOr<void> Element::insertAdjacentHTML(const String& where, const
String& markup, std::optional<NodeVector>& nodes)
    {
	... /* this will only collect nodes if (nodes) */
    }

    ExceptionOr<void> Element::insertAdjacentHTML(const String& where, const
String& markup)
    {
	return insertAdjacentHTML(where, markup, std::nullopt);
    }

> Source/WebCore/inspector/DOMEditor.cpp:280
> +	   : Action("InsertAdjacentHTML")

Is the name actually used for anything? I wonder if we should just remove it.
Or we can move to ASCIILiteral("...") to save a copy.

> Source/WebCore/inspector/DOMEditor.h:75
>      class ReplaceWholeTextAction;
> +    class InsertAdjacentHTMLAction;

Nit: `sort` these

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:125
> +    void insertAdjacentHTML(ErrorString&, int nodeId, const String&
position, const String& html) override;

Nit: The order of these should match the JSON, so preferably after
setOuterHTML.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:508
> +	   // COMPATIBILITY (iOS 11.1): DOM.insertAdjacentHTML did not exist.

Nit: Just say "COMPATIBILITY (iOS 11.0)", since `11.0` is the last versioned
protocol that was different.

> LayoutTests/inspector/dom/insertAdjacentHTML-expected.txt:1
> +Test for DOM.setOuterHTML on a page without a documentElement.

Fix this title.