Bug 44854 – MarkupAccumulator should be broken down into two classes

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Currently, MarkupAccumulator does two things
1. Serialize DOM tree to generate HTML / XHTML markup
2. Stylize markup to preserve the style.

Because 2 is an extension to 1, we should separate 2 into a separate class that inherits from MarkupAccumulator.

This separation is necessary to merge WebPageSerializer with MarkupAccumulator
http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebPageSerializerImpl.cpp
My current plan is to make a class inherited from MarkupAccumulator and override appending
and URL resolution mechanism. However, since WebPageSerializer needs to push a partial result via IPC
as soon as we filled up a buffer, we cannot support wrapWithNode in this new class.
Separating MarkupAccumulator and "Styled"MarkupAccumulator will solve this problem.

[Darin Adler]

(In reply to comment #0)
> 2. Stylize markup to preserve the style.

Could you be more specific about what "style" means here? Maybe an example?

[Ryosuke Niwa]

Created attachment 65883 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #1)
> (In reply to comment #0)
> > 2. Stylize markup to preserve the style.
> 
> Could you be more specific about what "style" means here? Maybe an example?

First, the range version of createMarkup only copies rendered nodes (this seems to be a bug).  When  DoesFullySelectNode = AnnotateForInterchange, we do more than that.  We copy HTML as it is rendered so we add styles added by CSS rules to inline styles, we call convertHTMLTextToInterchangeFormat on all text nodes, and so on.

But having such annotation-related code inside the serialization code seems rather odd, and I'd like to isolate that outside of MarkupAccumulator.

[Tony Chang]

Comment on attachment 65883 [details]
Patch

> Index: WebCore/ChangeLog
> +        MarkupAccumulator should be broken down into two classes
> +        https://bugs.webkit.org/show_bug.cgi?id=44854

Please add a short sentence why we want to have 2 classes (so we can expose this to webkit so embedders can implement save page).

> Index: WebCore/editing/markup.cpp
> +    String takeResults();
> +protected:

Nit: Add a blank line before protected:

> +    bool shouldAnnotate() { return m_shouldAnnotate == AnnotateForInterchange; }
> +private:

Nit: blank line before private:

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 65883 [details])
> > Index: WebCore/ChangeLog
> > +        MarkupAccumulator should be broken down into two classes
> > +        https://bugs.webkit.org/show_bug.cgi?id=44854
> 
> Please add a short sentence why we want to have 2 classes (so we can expose this to webkit so embedders can implement save page).

Would the following sound good to you?

        Extracted wrapWithNode, wrapWithStyleNode, stringValueForRange, renderedText, removeExteriorStyles,
        shouldAnnotate, m_shouldAnnotate, and m_reversedPrecedingMarkup from MarkupAccumulator to create
        StyledMarkupAccumulator in order to isolate annotation related code and prepending of text.

        Isolating MarkupAccumulator as a separate class has two advantages:
        1. Isolated serialization code is easier to understand and easier to security-review.
        2. Embedder can use MarkupAccumulator to implement "Save as" feature.

        Also made takeResults, appendText, and appendElement in MarkupAccumulator virtual to override in
        StyledMarkupAccumulator because prepending text requires overriding takeResults, appendText needs
        to append only rendered text when shouldAnnotate() is true, and appendElement requires a different
        behavior when shouldAnnotate() is true or when called inside wrapWithNode with convertBlocksToInlines = true.

        No new tests are added since this is a cleanup.

[Tony Chang]

(In reply to comment #5)
> Would the following sound good to you?

Sure, that sounds fine to me.

[Ryosuke Niwa]

Committed r67064: <http://trac.webkit.org/changeset/67064>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67064 might have broken Leopard Intel Debug (Tests)