RESOLVED FIXED 148268
Web Inspector: REGRESSION(173684): Edit as HTML not working 
https://bugs.webkit.org/show_bug.cgi?id=148268
Summary Web Inspector: REGRESSION(173684): Edit as HTML not working
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
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (9.01 KB, patch)
2015-08-20 18:56 PDT, Joseph Pecoraro 		cdumez: review+
bburg: commit-queue-
DetailsFormatted DiffDiff
[PATCH] For Landing (9.43 KB, patch)
2015-08-21 11:43 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
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.