MarkupAccumulator::appendStartMarkup consists of one giant switch statement in which all kinds of nodes are serialized. We should separate the serialization code for each type of node into a separate member functions of MarkupAccumulator.
Created attachment 64897 [details] cleanup
(In reply to comment #1) > Created an attachment (id=64897) [details] > cleanup Whole CharEntity business is very ugly. It'll be greatly appreciated if anyone can tell me a better way of defining an array of CharEntity's.
Attachment 64897 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Attachment 64897 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] > Total errors found: 1 in 2 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. This is a willful violation of the style guideline because the patch becomes unintelligible (svn diff tries to replace appendStartMarkup by appendElement, interleaving bits of pieces of both functions).
Comment on attachment 64897 [details] cleanup Thanks for working on this code. It's blowing my mind too much to review properly. WebCore/ChangeLog:14 + No new tests. (OOPS!) This can't land with OOPS
If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass.
Created attachment 64901 [details] fixed "No new tests. (OOPS!)" (In reply to comment #5) > (From update of attachment 64897 [details]) > Thanks for working on this code. It's blowing my mind too much to review properly. > > WebCore/ChangeLog:14 > + No new tests. (OOPS!) > This can't land with OOPS Sorry, I meant to change that line to "No new tests are added since this is a cleanup." Fixed. (In reply to comment #6) > If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass. Which function / part of code are you referring to?
> > If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass. > > Which function / part of code are you referring to? Oh, I just noticed that the style bot was red. I'd just work through the file until check-webkit-style passes. That makes future changes easier to review because we don't have to worry about existing bad style.
Attachment 64901 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 64993 [details] cleanup Removed escapeEntityReferences because that can be done in a separate patch
Comment on attachment 64993 [details] cleanup WebCore/editing/markup.cpp:490 + const QualifiedName* parentName = 0; I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imaging its any slower since QualifiedName it itself just a RefPtr. WebCore/editing/markup.cpp:489 + bool documentIsHTML = text->document()->isHTMLDocument(); Seems this is only used in one place in this function. WebCore/editing/markup.cpp:580 + append(out, attribute->name().localName()); is localName all caps or something? Confused why this is needed. WebCore/editing/markup.cpp:587 + // We don't want to complete file:/// URLs because it may contain sensitive information Are there other file-like urls that could get us into trouble? WebCore/editing/markup.cpp:604 + RefPtr<CSSMutableStyleDeclaration> style = static_cast<HTMLElement*>(element)->getInlineStyleDecl()->copy(); Wow, this block wants to be broken up again too. :) r+ since you're just moving code. But you might consider my comments for a follow-up patch. I suspect we also need more testing here, but that can come when we start fixing stuff here. :) Thank you SO MUCH for working on this!
(In reply to comment #11) > (From update of attachment 64993 [details]) > WebCore/editing/markup.cpp:490 > + const QualifiedName* parentName = 0; > I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imaging its any slower since QualifiedName it itself just a RefPtr. Will ask on webkit-dev since QualifiedName's default constructor is hidden behind a flag. > WebCore/editing/markup.cpp:489 > + bool documentIsHTML = text->document()->isHTMLDocument(); > Seems this is only used in one place in this function. Will fix. > WebCore/editing/markup.cpp:580 > + append(out, attribute->name().localName()); > is localName all caps or something? Confused why this is needed. toString gives back namespace:localName and we want to avoid that if the document is not XML. > WebCore/editing/markup.cpp:587 > + // We don't want to complete file:/// URLs because it may contain sensitive information > Are there other file-like urls that could get us into trouble? I'm not sure. Probably need some security review here. > WebCore/editing/markup.cpp:604 > + RefPtr<CSSMutableStyleDeclaration> style = static_cast<HTMLElement*>(element)->getInlineStyleDecl()->copy(); > Wow, this block wants to be broken up again too. :) Yes. But I'm thinking of doing this whole style thing up front so that we don't have to special-case style attribute at the very end. > r+ since you're just moving code. But you might consider my comments for a follow-up patch. I suspect we also need more testing here, but that can come when we start fixing stuff here. :) Since this code is shared in copying & innerHTML, I'd suspect that the test coverage isn't so bad. Changing one or two lines will definitely result in failure of several tests but I wouldn't mind having more structured tests as well. > Thank you SO MUCH for working on this! My pleasure, thanks for the review.
Committed r65854: <http://trac.webkit.org/changeset/65854>
http://trac.webkit.org/changeset/65854 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/65856 http://trac.webkit.org/changeset/65857 http://trac.webkit.org/changeset/65854 http://trac.webkit.org/changeset/65855
(In reply to comment #11) > (From update of attachment 64993 [details]) > WebCore/editing/markup.cpp:490 > + const QualifiedName* parentName = 0; > I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imagine its any slower since QualifiedName it itself just a RefPtr. I think a pointer is a good way to handle an optional value. Also, a RefPtr is slower than a raw pointer.