Merge page serializers [10/12]

Source/core/page/PageSerializer.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 117 matching lines @@
     , m_serializer(serializer)
     , m_document(&document)
     , m_nodes(nodes)
 {
 }
 
 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)
 {
     PageSerializer::Delegate* delegate = m_serializer->delegate();
     if (delegate)
         return delegate->shouldIgnoreAttribute(attribute);
 
     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;
 
     const HTMLFrameOwnerElement& frameOwner = toHTMLFrameOwnerElement(element);
     Frame* frame = frameOwner.contentFrame();
     // FIXME: RemoteFrames not currently supported here.
     if (!frame || !frame->isLocalFrame())
         return;
 
     KURL url = toLocalFrame(frame)->document()->url();
     if (url.isValid() && !url.protocolIsAbout())
         return;
 
     // 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)
 {
     MarkupAccumulator::appendStartTag(node, namespaces);
     m_nodes.append(&node);
 }
 
 void SerializerMarkupAccumulator::appendEndTag(const Element& element)
 {
@@ skipping 222 matching lines @@
 
     return fakeURL;
 }
 
 PageSerializer::Delegate* PageSerializer::delegate()
 {
     return m_delegate.get();
 }
 
 } // namespace blink