Bug 44288 – MarkupAccumulator::appendStartMarkup should be broken down into pieces

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 64897 [details]
cleanup

[Ryosuke Niwa]

(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.

[WebKit Review Bot]

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.

[Ryosuke Niwa]

(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).

[Adam Barth]

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

[Adam Barth]

If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass.

[Ryosuke Niwa]

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?

[Adam Barth]

> > 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.

[WebKit Review Bot]

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.

[Ryosuke Niwa]

Created attachment 64993 [details]
cleanup

Removed escapeEntityReferences because that can be done in a separate patch

[Eric Seidel (no email)]

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!

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Committed r65854: <http://trac.webkit.org/changeset/65854>

[WebKit Review Bot]

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

[Darin Adler]

(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.