Bug 148268

Summary:

Web Inspector: REGRESSION(173684): Edit as HTML not working

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, cdumez, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	none
[PATCH] Proposed Fix	cdumez: review+, bburg: commit-queue-
[PATCH] For Landing	none

Joseph Pecoraro

Reported 2015-08-20 17:03:05 PDT

* SUMMARY Edit as HTML not working in DOM Tree. * STEPS TO REPRODUCE 1. Inspector <body> on about:blank 2. Right click "Edit as HTML" for <body> element 3. Commit "<body><h1>Test</h1></body>" => DOM is unchanged, expected <h1>Test</h1> but it doesn't exist * NOTES - Regressed with <http://trac.webkit.org/changeset/173684>

Attachments
[PATCH] Proposed Fix (9.07 KB, patch) 2015-08-20 18:15 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[PATCH] Proposed Fix (9.01 KB, patch) 2015-08-20 18:56 PDT, Joseph Pecoraro	cdumez: review+ bburg: commit-queue-	Details Formatted Diff Diff
[PATCH] For Landing (9.43 KB, patch) 2015-08-21 11:43 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2015-08-20 17:03:16 PDT

<rdar://problem/22364817>

Joseph Pecoraro

Comment 2 2015-08-20 17:04:30 PDT

I'm going to write at least a basic test for DOM.setOuterHTML to make sure we don't break it again!

Joseph Pecoraro

Comment 3 2015-08-20 18:15:32 PDT

Created attachment 259546 [details] [PATCH] Proposed Fix

Joseph Pecoraro

Comment 4 2015-08-20 18:56:39 PDT

Created attachment 259559 [details] [PATCH] Proposed Fix Removed some trailing whitespace.

Nikita Vasilyev

Comment 5 2015-08-20 19:13:39 PDT

Is it a dupe of https://bugs.webkit.org/show_bug.cgi?id=147736? (I haven't looked closely into the issue.)

Chris Dumez

Comment 6 2015-08-20 19:22:05 PDT

Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review > Source/WebCore/inspector/DOMPatchSupport.cpp:-377 > - Node* node = parentNode->firstChild(); Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything.

Joseph Pecoraro

Comment 7 2015-08-20 20:11:56 PDT

*** Bug 147736 has been marked as a duplicate of this bug. ***

Blaze Burg

Comment 8 2015-08-21 09:46:50 PDT

Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review The setOuterHTML test is making me happy. I think our testing-fu is increasing. Please address Chris' question, though. It seems to be the meat of the fix, but I also have no idea why it is correct without spending a lot of time reading DOMPatch. That code is pretty gnarly, I'm surprised the Googlers didn't add tests. (edit: they did write tests, lots of them. In particular, they also tested undo of setOuterHTML. You can see all of their tests here: http://trac.webkit.org/browser/trunk/LayoutTests/inspector/elements?rev=107399&order=name) > LayoutTests/inspector/dom/getOuterHTML.html:20 > + InspectorTest.log(outerHTML); I would like us to get in the habit of labelling stuff that we dump. Here, the expected file would be more comprehensible if it said above: "Dumping outerHTML for element div#x:" > LayoutTests/inspector/dom/setOuterHTML.html:31 > +</div>`.trim(); awesome. > LayoutTests/inspector/dom/setOuterHTML.html:66 > + InspectorTest.expectThat(outerHTML === replacementOuterHTML, "The outerHTML should be what was just set"); Nit: period > LayoutTests/inspector/dom/setOuterHTML.html:77 > + InspectorTest.assert(domNode, "DOMNode exists"); Nit: period

Joseph Pecoraro

Comment 9 2015-08-21 11:31:10 PDT

Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review >> Source/WebCore/inspector/DOMPatchSupport.cpp:-377 >> - Node* node = parentNode->firstChild(); > > Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything. The optimization changes were wrong for a few reasons: - The loop would break out when we reached the end of the list of children (e.g. if !node) but that is fine if newMap/newList wanted to add a new node - `node` is not longer at index `i` when insertBefore inserts a node before it. At that point, `node` would be at index `i + 1`, so future uses of `node` would likely be off-shifted. I could, and did try correcting while keeping the optimizations but reading through DOMPatchSupport I couldn't be sure I wasn't overlooking any particular cases without writing tons more tests. Also, the loop right after this does the same thing without this attempt at an optimization, so this at least keeps them consistent.

Chris Dumez

Comment 10 2015-08-21 11:34:41 PDT

Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review r=me for the C++ part. I trust you and Brian for the inspector tests. >>> Source/WebCore/inspector/DOMPatchSupport.cpp:-377 >>> - Node* node = parentNode->firstChild(); >> >> Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything. > > The optimization changes were wrong for a few reasons: > > - The loop would break out when we reached the end of the list of children (e.g. if !node) but that is fine if newMap/newList wanted to add a new node > - `node` is not longer at index `i` when insertBefore inserts a node before it. At that point, `node` would be at index `i + 1`, so future uses of `node` would likely be off-shifted. > > I could, and did try correcting while keeping the optimizations but reading through DOMPatchSupport I couldn't be sure I wasn't overlooking any particular cases without writing tons more tests. Also, the loop right after this does the same thing without this attempt at an optimization, so this at least keeps them consistent. Ok, so we're basically modifying the children as we iterate over them :/

Joseph Pecoraro

Comment 11 2015-08-21 11:43:39 PDT

Created attachment 259636 [details] [PATCH] For Landing Addressed comments.

WebKit Commit Bot

Comment 12 2015-08-21 12:37:09 PDT

Comment on attachment 259636 [details] [PATCH] For Landing Clearing flags on attachment: 259636 Committed r188768: <http://trac.webkit.org/changeset/188768>

WebKit Commit Bot

Comment 13 2015-08-21 12:37:14 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.