Bug 52268

Created attachment 78634 [details] Patch

Attachment 78634 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/html/HTMLElement.cpp:379: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/html/HTMLElement.cpp:391: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/html/HTMLElement.cpp:393: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/html/HTMLElement.cpp:396: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/html/HTMLElement.cpp:493: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/html/HTMLElement.cpp:506: Use 0 instead of NULL. [readability/null] [5] Total errors found: 6 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 78645 [details] Patch Fixed style violations in moved code.

Comment on attachment 78645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78645&action=review > Source/WebCore/html/HTMLElement.cpp:381 > + ec = 0; I think this is generally the callers responsibility in WebCore. > Source/WebCore/html/HTMLElement.cpp:390 > + fragment->appendChild(Text::create(document(), text.substring(lineStart, i - lineStart)), ec); If this can run arbitrary javascript, "this" could get deleted, no? Do we need to suspend mutation events during this? > Source/WebCore/html/HTMLElement.cpp:399 > + lineStart = i + 1; I find it difficult to read this loop and understand what its doing. I can't tell if that's a variable naming problem, the way the blocks are split up, or just my own thick-headedness at this hour. > Source/WebCore/html/HTMLElement.cpp:498 > + textPrev->appendData(textNode->data(), ec); Does this cause JS to run? If so, our pointers could go invalid. > Source/WebCore/html/HTMLElement.cpp:511 > + RefPtr<Text> textNext = static_cast<Text*>(next.get()); > + RefPtr<Text> textNode = static_cast<Text*>(node); > + textNode->appendData(textNext->data(), ec); Seems we just did this above. Maybe there is code to share here?

Created attachment 78735 [details] Patch Thanks for the review, see comments inline. > > Source/WebCore/html/HTMLElement.cpp:381 > > + ec = 0; > I think this is generally the callers responsibility in WebCore. Thanks, fixed. > > Source/WebCore/html/HTMLElement.cpp:390 > > + fragment->appendChild(Text::create(document(), text.substring(lineStart, i - lineStart)), ec); > > If this can run arbitrary javascript, "this" could get deleted, no? Do we need to suspend mutation events during this? As the fragment isn't attached to the document yet I don't see how one would listen to that mutation event. The text-node-append-data-remove-crash.html tests this. > > Source/WebCore/html/HTMLElement.cpp:399 > > + lineStart = i + 1; > I find it difficult to read this loop and understand what its doing. I can't tell if that's a variable > naming problem, the way the blocks are split up, or just my own thick-headedness at this hour. I agree, it's not the easiest code to read. I moved it out of the setInnerText method and didn't want to make too many changes to it. > > Source/WebCore/html/HTMLElement.cpp:498 > > + textPrev->appendData(textNode->data(), ec); > > Does this cause JS to run? If so, our pointers could go invalid. We hold RefPtrs for both nodes so it should be safe. This code has been replaced with a call to mergeWithNextTextNode in the latest patch. > > Source/WebCore/html/HTMLElement.cpp:511 > > + RefPtr<Text> textNext = static_cast<Text*>(next.get()); > > + RefPtr<Text> textNode = static_cast<Text*>(node); > > + textNode->appendData(textNext->data(), ec); > Seems we just did this above. Maybe there is code to share here? Good idea, broke out the merging logic into a helper funciton.

Created attachment 78736 [details] Patch Removed unnecessary import.

Comment on attachment 78736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78736&action=review I think the biggest trouble with this patch is that you're inheriting some less-than-perfectly designed code, which has historically been poorly tested. It's difficult for me to draw the line between your current definite improvement of that code, and the ideal for said code. This current iteration is better than your last one, and *certainly* way beter than what we had before. If I were writing this patch, I'd want to go one more round. But I'm also open to the idea of landing this and iterating further in a separate patch or at a later time. > Source/WebCore/html/HTMLElement.cpp:387 > + if (c == '\n' || c == '\r') { I feel like this should be an early contineue, but that gets a bit ugly with the need for prev = c. > Source/WebCore/html/HTMLElement.cpp:403 > + if (length > lineStart) > + fragment->appendChild(Text::create(document(), text.substring(lineStart, length - lineStart)), ec); This is repeated from above, but missing the ec check. Is that intentional? how do we exercise this case? > Source/WebCore/html/HTMLElement.cpp:460 Should just early return instead of making a long if block. > Source/WebCore/html/HTMLElement.cpp:498 > + RefPtr<Node> prev = previousSibling(); > + RefPtr<Node> next = nextSibling(); > + if (text.isEmpty() && (!prev || !prev->isTextNode()) && (!next || !next->isTextNode())) { > + parent->replaceChild(Text::create(document(), ""), this, ec); > + return; > + } I'm not sure I understand this quirk or why it needs to be a separate if. I assume it's tested? > Source/WebCore/html/HTMLElement.cpp:506 > + if (text.contains('\r') || text.contains('\n')) > + newChild = textToFragment(text, ec); > + else > + newChild = Text::create(document(), text); I would just have put this if inside textToFragment, making ti just reutrn a Text node if there are no \n, \r.

Thanks for the feedback Eric, I'll do another round and try to clean it up some more.

Created attachment 78769 [details] Patch Rewrote the textToFragment method and made all the changes you suggested except for your last comment about making textToFragment return a text node for strings without a line break. The textToFragment method is used by setInnerText which requires a fragment. I didn't want to make this patch any larger by changing that but I'd be happy to make that change in a later patch if you think it's worthwhile.

Comment on attachment 78769 [details] Patch Thanks.

Comment on attachment 78769 [details] Patch Rejecting attachment 78769 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: outTests/fast/dom/set-outer-text.html patching file LayoutTests/fast/dom/text-node-append-data-remove-crash-expected.txt (Stripping trailing CRs from patch.) patching file LayoutTests/fast/dom/text-node-append-data-remove-crash.html Hunk #1 FAILED at 13. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/dom/text-node-append-data-remove-crash.html.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7592004

Created attachment 78835 [details] Patch Converted line endings for text-node-append-data-remove-crash.html to unix style in order to make the submit queue happy.

Comment on attachment 78835 [details] Patch Rejecting attachment 78835 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build'..." exit_code: 2 Last 500 characters of output: DE_VERSION_MINOR 0320 setenv YACC /Developer/usr/bin/yacc /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLElement.o /mnt/git/webkit-commit-queue/Source/WebCore/html/HTMLElement.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/7503017

Created attachment 78851 [details] Patch Made mergeWithNextTextNode function static to make XCode happy.

Comment on attachment 78851 [details] Patch Let's try again! :)

Comment on attachment 78851 [details] Patch Yay!

The commit-queue encountered the following flaky tests while processing attachment 78851 [details]: http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 78851 [details] Patch Clearing flags on attachment: 78851 Committed r75738: <http://trac.webkit.org/changeset/75738>

All reviewed patches have been landed. Closing bug.