[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
bugzilla-daemon at webkit.org
Sat Mar 27 00:16:14 PDT 2010
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.
More information about the webkit-reviews
mailing list