Bug 44854 - MarkupAccumulator should be broken down into two classes
Summary: MarkupAccumulator should be broken down into two classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 45624
  Show dependency treegraph
 
Reported: 2010-08-29 22:55 PDT by Ryosuke Niwa
Modified: 2010-09-12 16:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (22.77 KB, patch)
2010-08-29 23:37 PDT, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-08-29 22:55:53 PDT
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.
Comment 1 Darin Adler 2010-08-29 23:19:26 PDT
(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?
Comment 2 Ryosuke Niwa 2010-08-29 23:37:56 PDT
Created attachment 65883 [details]
Patch
Comment 3 Ryosuke Niwa 2010-08-30 10:08:27 PDT
(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.
Comment 4 Tony Chang 2010-09-08 12:26:24 PDT
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:
Comment 5 Ryosuke Niwa 2010-09-08 14:31:38 PDT
(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.
Comment 6 Tony Chang 2010-09-08 14:33:33 PDT
(In reply to comment #5)
> Would the following sound good to you?

Sure, that sounds fine to me.
Comment 7 Ryosuke Niwa 2010-09-08 23:32:36 PDT
Committed r67064: <http://trac.webkit.org/changeset/67064>
Comment 8 WebKit Review Bot 2010-09-09 12:59:13 PDT
http://trac.webkit.org/changeset/67064 might have broken Leopard Intel Debug (Tests)