Bug 179283 – Web Inspector: support undo/redo of insertAdjacentHTML

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 326060 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 326060 [details]
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.

[Devin Rousso]

Created attachment 326439 [details]
Patch

It took way too long to figure out that I needed to put & after NodeVector in order to not copy a Ref...

[Joseph Pecoraro]

Comment on attachment 326439 [details]
Patch

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

r=me

> Source/WebCore/dom/Element.h:315
> +    ExceptionOr<void> insertAdjacentHTML(const String& where, const String& html, std::optional<NodeVector&> nodes);

I think the name `nodes` could be `addedNodes` and it would be clearer.

[Devin Rousso]

Created attachment 326485 [details]
Patch

[Devin Rousso]

Created attachment 326486 [details]
Patch

Whoops.  Forgot tests.

[WebKit Commit Bot]

Comment on attachment 326486 [details]
Patch

Clearing flags on attachment: 326486

Committed r224648: <https://trac.webkit.org/changeset/224648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562282>