[Webkit-unassigned] [Bug 44288] MarkupAccumulator::appendStartMarkup should be broken down into pieces

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44288





--- Comment #12 from Ryosuke Niwa <rniwa at webkit.org>  2010-08-23 18:51:42 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.