[webkit-reviews] review granted: [Bug 36481] Web Inspector: Edit Tag Names : [Attachment 51850] [PATCH] Part 1 - Add "Edit Tag" on Double Click

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted 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 51850: [PATCH] Part 1 - Add "Edit Tag" on Double Click
https://bugs.webkit.org/attachment.cgi?id=51850&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +void InspectorDOMAgent::changeTagName(long callId, long nodeId, const
AtomicString& tagName, bool expanded)
> +{
> +    Node* oldNode = nodeForId(nodeId);
> +    if (!oldNode) {
> +	   // Use -1 to denote an error condition.
> +	   m_frontend->didChangeTagName(callId, -1);
> +	   return;

Assert non-element case early as well?

> +    }
> +
> +    // FIXME: Exception codes are ignored until the end.
> +    ExceptionCode code;
> +    RefPtr<Node> newNode = oldNode->document()->createElement(tagName,
code);
> +
> +    // Copy over the original node's attributes.
> +    if (oldNode->isElementNode() && newNode->isElementNode()) {
> +	   Element* oldElem = static_cast<Element*>(oldNode);
> +	   Element* newElem = static_cast<Element*>(newNode.get());
> +
> +	   newElem->copyNonAttributeProperties(oldElem);
> +	   if (oldElem->attributes())
> +	      
newElem->attributes()->setAttributes(*(oldElem->attributes(true)));
> +    }
> +
> +    // Copy over the original node's children.
> +    Node* child;
> +    while ((child = oldNode->firstChild()))
> +	   newNode->appendChild(child, code);

If you ignore previous call code, you should at least clear it before the
subsequent one I guess.

>  
> +    _tagNameEditingCommitted: function(element, newText, oldText, tagName,
moveDirection)
> +    {
> +	   delete this._editing;
> +
> +	   function cancel()
> +	   {
> +	       var closingTagElement = this._distinctClosingTagElement();
> +	       if (closingTagElement)
> +		   closingTagElement.textContent = "</" + tagName + ">";
> +
> +	       this._editingCancelled(element, tagName);
> +	       moveToNextAttributeIfNeeded.call(this);
> +	   }
> +
> +	   function moveToNextAttributeIfNeeded()

Nit: can we generalize traversal logic more and extract it from here?

> +	   try {
> +	       document.createElement(newText);
> +	   } catch(e) {
> +	       cancel.call(this);
> +	       return;
> +	   }

I am not sure this is necessary. You should rely on error codes on backend
side.