[webkit-reviews] review denied: [Bug 36481] Web Inspector: Edit Tag Names : [Attachment 51398] [PATCH] Adds "Add Child" and "Edit Tag" to the Context Menu

[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 51398: [PATCH] Adds "Add Child" and "Edit Tag" to the Context Menu
https://bugs.webkit.org/attachment.cgi?id=51398&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Looks cool! Menu gets populated!!!

General notes go first:
- Given a node with N children, Add Child only solves 1/N part of the problem.
I.e. Besides "Add Child" we need "Insert Above" and "Insert Below".
- I did not track it in the code, but according to the video, adding text nodes
capability is missing
- Taking #2 to the greater level, I think Add / Insert Child should be
implemented as Edit as HTML. I.e. accept free flow text as an input. That way
user would be able to paste portions of HTML as a source for new nodes. I know
we lose nice structuring of added nodes, but we gain huge flexibility in
return.



> +using namespace HTMLNames;
> +

I think our relationship with HTMLNames is not strong enough to start using
using.

> +	   frontend->didRemoveNode(callId, -1);

Bug. -> didAddChild ?

> +
> +	   newElem->copyNonAttributeProperties(oldElem);

Does this take care of JS properties on wrappers?

> +    if (code) {
> +	   frontend->didChangeTagName(callId, -3);

Do you handle these error codes? If not, why differentiating?

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

Oh no.

I'll review front-end code in greater detail later.