[webkit-reviews] review denied: [Bug 36481] Web Inspector: Edit Tag Names : [Attachment 51811] [PATCH] "Edit Tag" and "Add Child Element"

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 36481: Web Inspector: Edit Tag Names
https://bugs.webkit.org/show_bug.cgi?id=36481

Attachment 51811: [PATCH] "Edit Tag" and "Add Child Element"
https://bugs.webkit.org/attachment.cgi?id=51811&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +void InspectorBackend::addChildElement(long callId, long nodeId)
> +{

This code should be in InspectorDOMAgent that is responsible for mutating DOM.
See other dom-related methods in backend. InspectorBackend also should not be
HTMLNames aware. Sorry if I missed it previously.
InspectorBackend is just a facade to InspectorController from the front-end
side. It only handles trivial InspectorController duties in place. Heavy-weight
stuff goes either into InspectorController or InspectorDOMAgent or to where
appropriate.

> +void InspectorBackend::changeTagName(long callId, long nodeId, const
AtomicString& tagName)

Ditto. Please look at other DOM-related methods.

> +	   if
(WebInspector.ElementsTreeElement.EditTagBlacklist.indexOf(this.representedObje
ct.localName) === -1) {

Nit: I got used to using maps for these even if there are not too many items.
Appending .keySet() to array definition makes a set.

> +	   contextMenu.appendItem(WebInspector.UIString("Add Child Element"),
this._addChildElement.bind(this));

Please do not ignore my concerns about adding text nodes or mixed content. It
really bothers me and I think I have a strong point.

> +	       // FIXME: Short delay for the UI to catch up.
> +	       setTimeout(function() {

See suggestions on the bug comments.