[webkit-reviews] review granted: [Bug 44288] MarkupAccumulator::appendStartMarkup should be broken down into pieces : [Attachment 64993] cleanup

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 44288: MarkupAccumulator::appendStartMarkup should be broken down into
pieces
https://bugs.webkit.org/show_bug.cgi?id=44288

Attachment 64993: cleanup
https://bugs.webkit.org/attachment.cgi?id=64993&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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!