[webkit-reviews] review denied: [Bug 21108] Impossible to add an attribute to a node without attributes : [Attachment 33339] Add Attributes to Nodes

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 21108: Impossible to add an attribute to a node without attributes
https://bugs.webkit.org/show_bug.cgi?id=21108

Attachment 33339: Add Attributes to Nodes
https://bugs.webkit.org/attachment.cgi?id=33339&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
Thanks for the video! It helped a lot.

I wonder if a icon (button looking) would be better than the text ellipsis. Or
move the ellipsis to the inside of the tag so it is where the new attribute
will be insterted? What do you think?

> +	       var span = document.createElement('span');
> +	       span.className = 'add-attribute';
> +	       span.innerText = '…';
> +	       this.listItemElement.appendChild(span);
> +	       this._addAttributeElement = span;

Better to use a escaped character (\uXXXX) instead of UTF8 here. Also we use
double quotes for string almost esclusivly in our JS.

The textContent property is better to use than innerText. It has no IE
weirdness and is standard.

> +		   this._addAttributeElement.parentNode.removeChild(
this._addAttributeElement );

No spaces needed here.

> +	   if ( tag.getElementsByClassName('webkit-html-attribute').length > 0
) {

No spaces needed here.

> +	   return this._startEditingAttribute(attr, { target: attr });

No spaces needed here.