Web Inspector: Cannot edit elements commented with <!--

[Alexander Pavlov (apavlov)]

What steps will reproduce the problem?
1. Navigate to phrack.com/issues.html
2. Open developer tools 
3. Right click on super secret hidden Kiwicon endorsement


What is the expected result?

4. Get "Edit as HTML" option
5. Uncomment

What happens instead?

4. No option to edit, only Inspect Element and Word Wrap.

Please provide any additional information below. Attach a screenshot if
possible.

If I comment out any element in this fashion I cannot uncomment it via the dev tools, i have to reload the page.

I don't see why the dev tools can't edit commented elements. That doesnt make sense.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=92605

[Alexander Pavlov (apavlov)]

Created attachment 113471 [details]
[PATCH] Suggested solution

[Pavel Feldman]

Comment on attachment 113471 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=113471&action=review

r- for two branches in setOuterHTML

> LayoutTests/inspector/elements/edit-dom-actions.html:165
> +        if (typeof dataNode === "string")

You should use different method for comment nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:772
> +        RefPtr<DocumentFragment> fragment = DocumentFragment::create(document);

You should use this branch for both cases.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:776
> +            return;

Assign errorString here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:781
> +            return;

ditto

> Source/WebCore/inspector/InspectorDOMAgent.cpp:791
> +        if (ec)

ditto

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1061
> +    _populateCommentContextMenu: function(contextMenu, commentNode)

You should call this method from _populateTagContextMenu (and call it _populateNodeContextMenu). Should also work for Text nodes.

[Alexander Pavlov (apavlov)]

Created attachment 113515 [details]
[PATCH] Comments addressed

[Pavel Feldman]

Comment on attachment 113515 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=113515&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:751
>      HTMLElement* element = assertHTMLElement(errorString, nodeId);

If user passes TextNode id into this method, he will get an error message saying that it should always be id of the Element. But we do support nodes (see comment above). You should either support more node types and remove this assertion or not call getOuterHTML for non-Element nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:772
> +    if (document->isHTMLDocument())

You could put this check earlier. Can this happen at all?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:787
> +        requiresTotalUpdate = htmlElement->tagName() == "HTML" || htmlElement->tagName() == "BODY" || htmlElement->tagName() == "HEAD";

no need to cast, tagName() is nodeName() for elements.

[Alexander Pavlov (apavlov)]

Created attachment 113832 [details]
[PATCH] Comments addressed - 2

[WebKit Review Bot]

Comment on attachment 113832 [details]
[PATCH] Comments addressed - 2

Clearing flags on attachment: 113832

Committed r99401: <http://trac.webkit.org/changeset/99401>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Committed r99402: <http://trac.webkit.org/changeset/99402>