.
Created attachment 326060 [details] Patch
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.
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...
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.
Created attachment 326485 [details] Patch
Created attachment 326486 [details] Patch Whoops. Forgot tests.
Comment on attachment 326486 [details] Patch Clearing flags on attachment: 326486 Committed r224648: <https://trac.webkit.org/changeset/224648>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35562282>