Merge page serializers [10/12]

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 6f26b42564efa38e3be47e401e68365d564c4383..a7c6ffd0d36109194c4d0c279731c0e79b946074 100644
--- a/Source/core/page/PageSerializer.cpp
+++ b/Source/core/page/PageSerializer.cpp
@@ -135,11 +135,11 @@ 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);
 }
 
 bool SerializerMarkupAccumulator::shouldIgnoreAttribute(const Attribute& attribute)
@@ -151,21 +151,27 @@ bool SerializerMarkupAccumulator::shouldIgnoreAttribute(const Attribute& attribu
     return MarkupAccumulator::shouldIgnoreAttribute(attribute);
 }
 
-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);
 
+    // FIXME: Refactor MarkupAccumulator so it is easier to append an element like this, without special cases for XHTML

[hajimehoshi, 2015/06/11 01:40:32]

nit: Use TODO(yourname) instead.

[hajimehoshi, 2015/06/11 02:30:16]

Now MarkupFormatter has two modes represented by enum SerializationType (XML or
not). I suggest to expand this to have XHTML mode so that we can delegate these
special cases to MarkupFormatter. yosin@ or I am going to do that. What do you
think?

[Tiger (Sony Mobile), 2015/06/11 08:15:16]

Done.

[Tiger (Sony Mobile), 2015/06/11 08:15:16]

It would be nice to have that delegated yes. Is another mode needed or can XML
mode be used for XHTML as well?

As an alternative, how feasible is to create a temporary HTMLMetaElement and
print it using the existing methods in the serializer?

[hajimehoshi, 2015/06/11 08:37:12]

Yes, XHTML is slightly different from XML in that XHTML needs a space char
before closing a tag '/>' as you did.
Good point. I definitely recommend to create a temporary object and use
appendElement so far.

[Tiger (Sony Mobile), 2015/06/11 10:22:10]

This is only recommended for compatibility but not required as far as I know.
This does seem to have its own set of problems. Mainly creating a new Elements
requires a non-const Document reference. I'm not sure why. PageSerializer only
have a const reference to the document which makes sense, and it is probably a
good idea to keep it that way. Disregarding the const problem, this at least
feels slightly better:

  RefPtrWillBeRawPtr<HTMLMetaElement> meta =
HTMLMetaElement::create(const_cast<Document&>(*m_document));
  meta->setAttribute(HTMLNames::http_equivAttr, "Content-Type");
  String content = String::format("%s; charset=%s",
m_document->suggestedMIMEType().utf8().data(),
m_document->charset().utf8().data());
  meta->setAttribute(HTMLNames::contentAttr, AtomicString(content));
  MarkupAccumulator::appendElement(result, *meta.release(), nullptr);

[yosin_UTC9, 2015/06/12 07:07:31]

How about using  |Document* document = element->document();| and use local
variable |documnet| instead of |m_document|.

Yes, it is rather hacky, but not so strange since |element| parameter isn't
const.

[philipj_slow, 2015/06/12 07:58:49]

Actually XHTML does not need a space before the slash, I believe that style came
from trying to write polyglot documents that would work in IE6 way back, and
that no browser has had any problem with e.g. |<img src="image"/>| with no space
since then. Is this the only difference, or are there other differences with an
XHTML serializer? If the output is expected to actually be parsed as XML and not
HTML, then serializing it as XML ought to be just fine...

     if (isHTMLHeadElement(element)) {
-        out.appendLiteral("<meta charset=\"");
-        out.append(m_document->charset());
-        out.appendLiteral("\">");
+        result.appendLiteral("<meta http-equiv=\"Content-Type\" content=\"");
+        result.append(m_document->suggestedMIMEType());

[hajimehoshi, 2015/06/11 01:40:32]

Use MarkupFormatter::appendAttributeValue for escaping

[Tiger (Sony Mobile), 2015/06/11 08:15:16]

Done.

+        result.appendLiteral("; charset=");

[hajimehoshi, 2015/06/11 01:41:40]

last '"' is missing?

[Tiger (Sony Mobile), 2015/06/11 08:15:16]

The last " is on line 166/168. The complete tag will look like this:
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">

+        result.append(m_document->charset());

[hajimehoshi, 2015/06/11 01:41:41]

MarkupFormatter::appendAttributeValue

[Tiger (Sony Mobile), 2015/06/11 08:15:17]

Done.

+        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, const Element& element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& result, const Element& element, Namespaces* namespaces)
 {
     if (!element.isFrameOwnerElement())
         return;
@@ -182,7 +188,7 @@ void SerializerMarkupAccumulator::appendCustomAttributes(StringBuilder& out, con
 
     // We need to give a fake location to blank frames so they can be referenced by the serialized frame.
     url = m_serializer->urlForBlankFrame(toLocalFrame(frame));
-    appendAttribute(out, element, Attribute(frameOwnerURLAttributeName(frameOwner), AtomicString(url.string())), namespaces);
+    appendAttribute(result, element, Attribute(frameOwnerURLAttributeName(frameOwner), AtomicString(url.string())), namespaces);
 }
 
 void SerializerMarkupAccumulator::appendStartTag(Node& node, Namespaces* namespaces)