Bug 71357

Summary: Web Inspector: Cannot edit elements commented with <!--
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Suggested solution
pfeldman: review-
[PATCH] Comments addressed
pfeldman: review-
[PATCH] Comments addressed - 2 none

Description Alexander Pavlov (apavlov) 2011-11-02 04:52:14 PDT
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
Comment 1 Alexander Pavlov (apavlov) 2011-11-03 06:05:24 PDT
Created attachment 113471 [details]
[PATCH] Suggested solution
Comment 2 Pavel Feldman 2011-11-03 06:57:49 PDT
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.
Comment 3 Alexander Pavlov (apavlov) 2011-11-03 10:09:21 PDT
Created attachment 113515 [details]
[PATCH] Comments addressed
Comment 4 Pavel Feldman 2011-11-05 09:05:47 PDT
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.
Comment 5 Alexander Pavlov (apavlov) 2011-11-07 00:41:37 PST
Created attachment 113832 [details]
[PATCH] Comments addressed - 2
Comment 6 WebKit Review Bot 2011-11-07 01:59:44 PST
Comment on attachment 113832 [details]
[PATCH] Comments addressed - 2

Clearing flags on attachment: 113832

Committed r99401: <http://trac.webkit.org/changeset/99401>
Comment 7 WebKit Review Bot 2011-11-07 01:59:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Yuta Kitamura 2011-11-07 02:51:44 PST
Committed r99402: <http://trac.webkit.org/changeset/99402>