Merges the two different page serializers

Source/core/page/PageSerializer.cpp

Index: Source/core/page/PageSerializer.cpp
diff --git a/Source/core/page/PageSerializer.cpp b/Source/core/page/PageSerializer.cpp
index 59a6542c5de492030ac17ef04b00ca0c42031d2c..623e117265c00ca1c7ab9c37290d74133c1ac0fa 100644
--- a/Source/core/page/PageSerializer.cpp
+++ b/Source/core/page/PageSerializer.cpp
@@ -53,6 +53,7 @@
 #include "core/html/HTMLInputElement.h"
 #include "core/html/HTMLLinkElement.h"
 #include "core/html/HTMLStyleElement.h"
+#include "core/html/ImageDocument.h"
 #include "core/html/parser/HTMLMetaCharsetParser.h"
 #include "core/frame/Frame.h"
 #include "core/page/Page.h"
@@ -61,6 +62,7 @@
 #include "core/rendering/style/StyleFetchedImage.h"
 #include "core/rendering/style/StyleImage.h"
 #include "platform/SerializedResource.h"
+#include "wtf/OwnPtr.h"

[abarth-chromium, 2013/11/14 16:55:03]

Undoubtedly one of these other headers pulls in OwnPtr.h :)

 #include "wtf/text/CString.h"
 #include "wtf/text/StringBuilder.h"
 #include "wtf/text/TextEncoding.h"
@@ -105,12 +107,11 @@ public:
     virtual ~SerializerMarkupAccumulator();
 
 protected:
-    virtual void appendText(StringBuilder& out, Text*);
-    virtual void appendElement(StringBuilder& out, Element*, Namespaces*);
-    virtual void appendCustomAttributes(StringBuilder& out, Element*, Namespaces*);
-    virtual void appendEndTag(Node*);
+    virtual void appendText(StringBuilder&, Text*) OVERRIDE;
+    virtual void appendElement(StringBuilder&, Element*, Namespaces*) OVERRIDE;
+    virtual void appendCustomAttributes(StringBuilder&, Element*, Namespaces*) OVERRIDE;
+    virtual void appendEndTag(Node*) OVERRIDE;
 
-private:
     PageSerializer* m_serializer;
     Document* m_document;
 };
@@ -120,34 +121,38 @@ SerializerMarkupAccumulator::SerializerMarkupAccumulator(PageSerializer* seriali
     , m_serializer(serializer)
     , m_document(document)
 {
+

[abarth-chromium, 2013/11/14 16:55:03]

This blank line is not needed.

 }
 
 SerializerMarkupAccumulator::~SerializerMarkupAccumulator()
 {
 }
 
-void SerializerMarkupAccumulator::appendText(StringBuilder& out, Text* text)
+void SerializerMarkupAccumulator::appendText(StringBuilder& result, Text* text)
 {
     Element* parent = text->parentElement();
     if (parent && !shouldIgnoreElement(parent))
-        MarkupAccumulator::appendText(out, text);
+        MarkupAccumulator::appendText(result, text);
 }
 
-void SerializerMarkupAccumulator::appendElement(StringBuilder& out, Element* element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendElement(StringBuilder& result, Element* element, Namespaces* namespaces)
 {
     if (!shouldIgnoreElement(element))
-        MarkupAccumulator::appendElement(out, element, namespaces);
+        MarkupAccumulator::appendElement(result, element, namespaces);
 
     if (element->hasTagName(HTMLNames::headTag)) {
-        out.append("<meta charset=\"");
-        out.append(m_document->charset());
-        out.append("\">");
+        result.appendLiteral("<meta charset=\"");
+        result.append(m_document->charset());
+        if (m_document->isXHTMLDocument())
+            result.appendLiteral("\" />");
+        else
+            result.appendLiteral("\">");
     }
 
     // FIXME: For object (plugins) tags and video tag we could replace them by an image of their current contents.
 }
 
-void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& out, Element* element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& result, Element* element, Namespaces* namespaces)
 {
     if (!element->isFrameOwnerElement())
         return;
@@ -163,7 +168,7 @@ void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& out, Ele
 
     // We need to give a fake location to blank frames so they can be referenced by the serialized frame.
     url = m_serializer->urlForBlankFrame(frame);
-    appendAttribute(out, element, Attribute(frameOwnerURLAttributeName(*frameOwner), url.string()), namespaces);
+    appendAttribute(result, element, Attribute(frameOwnerURLAttributeName(*frameOwner), url.string()), namespaces);
 }
 
 void SerializerMarkupAccumulator::appendEndTag(Node* node)
@@ -172,10 +177,99 @@ void SerializerMarkupAccumulator::appendEndTag(Node* node)
         MarkupAccumulator::appendEndTag(node);
 }
 
-PageSerializer::PageSerializer(Vector<SerializedResource>* resources)
+
+

[abarth-chromium, 2013/11/14 16:55:03]

You've got two extra blank lines here.

+class LinkChangeSerializerMarkupAccumulator : public SerializerMarkupAccumulator {
+public:
+    LinkChangeSerializerMarkupAccumulator(PageSerializer*, Document*, Vector<Node*>*, LinkLocalPathMap*, String);
+
+protected:
+    virtual void appendElement(StringBuilder&, Element*, Namespaces*) OVERRIDE;
+    virtual void appendAttribute(StringBuilder&, Element*, const Attribute&, Namespaces*) OVERRIDE;
+
+private:
+    // local_links_ include all pair of local resource path and corresponding
+    // original link.
+    LinkLocalPathMap* m_replaceLinks;
+    String m_directoryName;
+};
+
+LinkChangeSerializerMarkupAccumulator::LinkChangeSerializerMarkupAccumulator(PageSerializer* serializer, Document* document, Vector<Node*>* nodes, LinkLocalPathMap* links, String directoryName)
+    : SerializerMarkupAccumulator(serializer, document, nodes)
+    , m_replaceLinks(links)
+    , m_directoryName(directoryName)
+{
+}
+
+void LinkChangeSerializerMarkupAccumulator::appendElement(StringBuilder& result, Element* element, Namespaces* namespaces)
+{
+    // TODO: or move to append open tag / close tag

[abarth-chromium, 2013/11/14 16:55:03]

TODO -> FIXME

Also, please use complete sentences in comments.

+
+    if (element->hasTagName(HTMLNames::baseTag)) {
+        // Comment the BASE tag when serializing dom.
+        result.append("<!--");
+    } else if (element->hasTagName(HTMLNames::htmlTag)) {
+        // Add MOTW (Mark of the Web) declaration before html tag.
+        // See http://msdn2.microsoft.com/en-us/library/ms537628(VS.85).aspx.
+        result.append(String::format("\n<!-- saved from url=(%04d)%s -->\n",
+            static_cast<int>(m_document->url().string().utf8().length()),
+            m_document->url().string().utf8().data()));
+    }
+
+    SerializerMarkupAccumulator::appendElement(result, element, namespaces);
+
+    if (element->hasTagName(HTMLNames::baseTag)) {
+        // Comment the BASE tag when serializing dom.
+        result.appendLiteral("-->");
+
+        // Append a new base tag declaration.
+        result.appendLiteral("<base href=\".\"");
+        if (!m_document->baseTarget().isEmpty()) {
+            result.appendLiteral(" target=\"");
+            result.append(m_document->baseTarget());
+            result.append('"');
+        }
+        if (m_document->isXHTMLDocument())
+            result.appendLiteral(" />");
+        else
+            result.appendLiteral(">");
+    }
+}
+void LinkChangeSerializerMarkupAccumulator::appendAttribute(StringBuilder& result, Element* element, const Attribute& attribute, Namespaces* namespaces)
+{
+    if (m_replaceLinks && element->isURLAttribute(attribute)
+        && !element->isJavaScriptURLAttribute(attribute)) {
+
+        // Get the absolute link

[abarth-chromium, 2013/11/14 16:55:03]

You can remove these comments that just way what the code does.  We can
determine that by looking at the code.  Comments that explain why the code does
what it does are useful, however.

+        String completeURL = m_document->completeURL(attribute.value());
+
+        // Check whether we have local files for those link.
+        if (m_replaceLinks->contains(completeURL)) {
+            // TODO: refactor markupaccumulator, reuse code

[abarth-chromium, 2013/11/14 16:55:03]

TODO -> FIXME

+            result.append(' ');
+            result.append(attribute.name().toString());
+            result.appendLiteral("=\"");
+            if (!m_directoryName.isEmpty()) {
+                result.appendLiteral("./");
+                result.append(m_directoryName);
+                result.append('/');
+            }
+            result.append(m_replaceLinks->get(completeURL));
+            result.appendLiteral("\"");
+            return;
+        }
+    }
+    MarkupAccumulator::appendAttribute(result, element, attribute, namespaces);
+}
+
+

[abarth-chromium, 2013/11/14 16:55:03]

You've got an extra blank line here.

+PageSerializer::PageSerializer(Vector<SerializedResource>* resources, LinkLocalPathMap* urls, String directory)
     : m_resources(resources)
+    , m_URLs(urls)
+    , m_directory(directory)
     , m_blankFrameCounter(0)
 {
+
 }
 
 void PageSerializer::serialize(Page* page)
@@ -199,15 +293,22 @@ void PageSerializer::serializeFrame(Frame* frame)
         return;
     }
 
-    Vector<Node*> nodes;
-    SerializerMarkupAccumulator accumulator(this, document, &nodes);
-    WTF::TextEncoding textEncoding(document->charset());
-    CString data;
-    if (!textEncoding.isValid()) {
-        // FIXME: iframes used as images trigger this. We should deal with them correctly.
+    // If frame is an image document, add the image and don't continue
+    if (document->isImageDocument()) {
+        ImageDocument* imageDocument = toImageDocument(document);
+        addImageToResources(imageDocument->cachedImage(), imageDocument->imageElement()->renderer(), url);
         return;
     }
-    String text = accumulator.serializeNodes(document, IncludeNode);
+
+    Vector<Node*> nodes;
+    WTF::OwnPtr<SerializerMarkupAccumulator> accumulator;

[abarth-chromium, 2013/11/14 16:55:03]

No need for WTF:: here.  OwnPtr has a using WTF::OwnPtr directive in the header.

+    if (m_URLs) {
+        accumulator = adoptPtr(new LinkChangeSerializerMarkupAccumulator(this, document, &nodes, m_URLs, m_directory));
+    } else {
+        accumulator = adoptPtr(new SerializerMarkupAccumulator(this, document, &nodes));
+    }

[abarth-chromium, 2013/11/14 16:55:03]

No need for { } in either branch of the if here.

+    String text = accumulator->serializeNodes(document, IncludeNode);
+    WTF::TextEncoding textEncoding(document->charset());
     CString frameHTML = textEncoding.normalizeAndEncode(text, WTF::EntitiesForUnencodables);
     m_resources->append(SerializedResource(url, document->suggestedMIMEType(), SharedBuffer::create(frameHTML.data(), frameHTML.length())));
     m_resourceURLs.add(url);
@@ -219,8 +320,10 @@ void PageSerializer::serializeFrame(Frame* frame)
 
         Element* element = toElement(node);
         // We have to process in-line style as it might contain some resources (typically background images).
-        if (element->isStyledElement())
+        if (element->isStyledElement()) {
             retrieveResourcesForProperties(element->inlineStyle(), document);
+            retrieveResourcesForProperties(element->presentationAttributeStyle(), document);
+        }
 
         if (element->hasTagName(HTMLNames::imgTag)) {
             HTMLImageElement* imageElement = toHTMLImageElement(element);
@@ -311,7 +414,7 @@ void PageSerializer::addImageToResources(ImageResource* image, RenderObject* ima
     if (!shouldAddURL(url))
         return;
 
-    if (!image || image->image() == Image::nullImage())
+    if (!image->hasImage() || image->image() == Image::nullImage())
         return;
 
     RefPtr<SharedBuffer> data = imageRenderer ? image->imageForRenderer(imageRenderer)->data() : 0;
@@ -383,3 +486,4 @@ KURL PageSerializer::urlForBlankFrame(Frame* frame)
 }
 
 }
+

[abarth-chromium, 2013/11/14 16:55:03]

No need for this blank line.