Use more const references in MarkupAccumulator and its subclasses

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 50 matching lines @@
 #include "core/rendering/style/StyleImage.h"
 #include "platform/SerializedResource.h"
 #include "platform/graphics/Image.h"
 #include "wtf/text/CString.h"
 #include "wtf/text/StringBuilder.h"
 #include "wtf/text/TextEncoding.h"
 #include "wtf/text/WTFString.h"
 
 namespace WebCore {
 
-static bool isCharsetSpecifyingNode(Node* node)
+static bool isCharsetSpecifyingNode(const Node& node)
 {
-    if (!node->isHTMLElement())
+    if (!node.isHTMLElement())
         return false;
 
-    HTMLElement* element = toHTMLElement(node);
-    if (!element->hasTagName(HTMLNames::metaTag))
+    const HTMLElement& element = toHTMLElement(node);
+    if (!element.hasTagName(HTMLNames::metaTag))
         return false;
     HTMLAttributeList attributes;
-    if (element->hasAttributes()) {
-        for (unsigned i = 0; i < element->attributeCount(); ++i) {
-            const Attribute* attribute = element->attributeItem(i);
+    if (element.hasAttributes()) {
+        for (unsigned i = 0; i < element.attributeCount(); ++i) {
+            const Attribute* attribute = element.attributeItem(i);
             // FIXME: We should deal appropriately with the attribute if they have a namespace.
             attributes.append(std::make_pair(attribute->name().localName(), attribute->value().string()));
         }
     }
     WTF::TextEncoding textEncoding = encodingFromMetaAttributes(attributes);
     return textEncoding.isValid();
 }
 
-static bool shouldIgnoreElement(Element* element)
+static bool shouldIgnoreElement(const Element& element)
 {
-    return element->hasTagName(HTMLNames::scriptTag) || element->hasTagName(HTMLNames::noscriptTag) || isCharsetSpecifyingNode(element);
+    return element.hasTagName(HTMLNames::scriptTag) || element.hasTagName(HTMLNames::noscriptTag) || isCharsetSpecifyingNode(element);
 }
 
 static const QualifiedName& frameOwnerURLAttributeName(const HTMLFrameOwnerElement& frameOwner)
 {
     // FIXME: We should support all frame owners including applets.
     return frameOwner.hasTagName(HTMLNames::objectTag) ? HTMLNames::dataAttr : HTMLNames::srcAttr;
 }
 
 class SerializerMarkupAccumulator FINAL : public MarkupAccumulator {
 public:
-    SerializerMarkupAccumulator(PageSerializer*, Document*, Vector<Node*>*);
+    SerializerMarkupAccumulator(PageSerializer*, const Document&, Vector<const Node*>*);
     virtual ~SerializerMarkupAccumulator();
 
 protected:
-    virtual void appendText(StringBuilder& out, Text*) OVERRIDE;
-    virtual void appendElement(StringBuilder& out, Element*, Namespaces*) OVERRIDE;
-    virtual void appendCustomAttributes(StringBuilder& out, Element*, Namespaces*) OVERRIDE;
-    virtual void appendEndTag(Node*) OVERRIDE;
+    virtual void appendText(StringBuilder& out, const Text&) OVERRIDE;
+    virtual void appendElement(StringBuilder& out, const Element&, Namespaces*) OVERRIDE;
+    virtual void appendCustomAttributes(StringBuilder& out, const Element&, Namespaces*) OVERRIDE;
+    virtual void appendEndTag(const Node&) OVERRIDE;
 
 private:
     PageSerializer* m_serializer;
-    Document* m_document;
+    const Document& m_document;
 };
 
-SerializerMarkupAccumulator::SerializerMarkupAccumulator(PageSerializer* serializer, Document* document, Vector<Node*>* nodes)
+SerializerMarkupAccumulator::SerializerMarkupAccumulator(PageSerializer* serializer, const Document& document, Vector<const Node*>* nodes)
     : MarkupAccumulator(nodes, ResolveAllURLs)
     , m_serializer(serializer)
     , m_document(document)
 {
 }
 
 SerializerMarkupAccumulator::~SerializerMarkupAccumulator()
 {
 }
 
-void SerializerMarkupAccumulator::appendText(StringBuilder& out, Text* text)
+void SerializerMarkupAccumulator::appendText(StringBuilder& out, const Text& text)
 {
-    Element* parent = text->parentElement();
-    if (parent && !shouldIgnoreElement(parent))
+    Element* parent = text.parentElement();
+    if (parent && !shouldIgnoreElement(*parent))
         MarkupAccumulator::appendText(out, text);
 }
 
-void SerializerMarkupAccumulator::appendElement(StringBuilder& out, Element* element, Namespaces* namespaces)
+void SerializerMarkupAccumulator::appendElement(StringBuilder& out, const Element& element, Namespaces* namespaces)
 {
     if (!shouldIgnoreElement(element))
         MarkupAccumulator::appendElement(out, element, namespaces);
 
-    if (element->hasTagName(HTMLNames::headTag)) {
+    if (element.hasTagName(HTMLNames::headTag)) {
         out.append("<meta charset=\"");
-        out.append(m_document->charset());
+        out.append(m_document.charset());
         out.append("\">");
     }
 
     // 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& out, const Element& element, Namespaces* namespaces)
 {
-    if (!element->isFrameOwnerElement())
+    if (!element.isFrameOwnerElement())
         return;
 
-    HTMLFrameOwnerElement* frameOwner = toHTMLFrameOwnerElement(element);
-    Frame* frame = frameOwner->contentFrame();
+    const HTMLFrameOwnerElement& frameOwner = toHTMLFrameOwnerElement(element);
+    Frame* frame = frameOwner.contentFrame();
     if (!frame)
         return;
 
     KURL url = frame->document()->url();
     if (url.isValid() && !url.isBlankURL())
         return;
 
     // 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), AtomicString(url.string())), namespaces);
+    appendAttribute(out, element, Attribute(frameOwnerURLAttributeName(frameOwner), AtomicString(url.string())), namespaces);
 }
 
-void SerializerMarkupAccumulator::appendEndTag(Node* node)
+void SerializerMarkupAccumulator::appendEndTag(const Node& node)
 {
-    if (node->isElementNode() && !shouldIgnoreElement(toElement(node)))
+    if (node.isElementNode() && !shouldIgnoreElement(toElement(node)))
         MarkupAccumulator::appendEndTag(node);
 }
 
 PageSerializer::PageSerializer(Vector<SerializedResource>* resources)
     : m_resources(resources)
     , m_blankFrameCounter(0)
 {
 }
 
 void PageSerializer::serialize(Page* page)
 {
     serializeFrame(page->mainFrame());
 }
 
 void PageSerializer::serializeFrame(Frame* frame)
 {
-    Document* document = frame->document();
-    KURL url = document->url();
+    ASSERT(frame->document());
+    Document& document = *frame->document();
+    KURL url = document.url();
     if (!url.isValid() || url.isBlankURL()) {
         // For blank frames we generate a fake URL so they can be referenced by their containing frame.
         url = urlForBlankFrame(frame);
     }
 
     if (m_resourceURLs.contains(url)) {
         // FIXME: We could have 2 frame with the same URL but which were dynamically changed and have now
         // different content. So we should serialize both and somehow rename the frame src in the containing
         // frame. Arg!
         return;
     }
 
-    WTF::TextEncoding textEncoding(document->charset());
+    WTF::TextEncoding textEncoding(document.charset());
     if (!textEncoding.isValid()) {
         // FIXME: iframes used as images trigger this. We should deal with them correctly.
         return;
     }
 
-    Vector<Node*> serializedNodes;
+    Vector<const Node*> serializedNodes;
     SerializerMarkupAccumulator accumulator(this, document, &serializedNodes);
     String text = accumulator.serializeNodes(document, IncludeNode);
     CString frameHTML = textEncoding.normalizeAndEncode(text, WTF::EntitiesForUnencodables);
-    m_resources->append(SerializedResource(url, document->suggestedMIMEType(), SharedBuffer::create(frameHTML.data(), frameHTML.length())));
+    m_resources->append(SerializedResource(url, document.suggestedMIMEType(), SharedBuffer::create(frameHTML.data(), frameHTML.length())));
     m_resourceURLs.add(url);
 
-    for (Vector<Node*>::iterator iter = serializedNodes.begin(); iter != serializedNodes.end(); ++iter) {
-        Node* node = *iter;
-        if (!node->isElementNode())
+    for (Vector<const Node*>::iterator iter = serializedNodes.begin(); iter != serializedNodes.end(); ++iter) {
+        ASSERT(*iter);
+        const Node& node = **iter;
+        if (!node.isElementNode())
             continue;
 
-        Element* element = toElement(node);
+        const Element& element = toElement(node);
         // We have to process in-line style as it might contain some resources (typically background images).
-        if (element->isStyledElement())
-            retrieveResourcesForProperties(element->inlineStyle(), document);
+        if (element.isStyledElement())
+            retrieveResourcesForProperties(element.inlineStyle(), document);
 
-        if (element->hasTagName(HTMLNames::imgTag)) {
-            HTMLImageElement* imageElement = toHTMLImageElement(element);
-            KURL url = document->completeURL(imageElement->getAttribute(HTMLNames::srcAttr));
-            ImageResource* cachedImage = imageElement->cachedImage();
-            addImageToResources(cachedImage, imageElement->renderer(), url);
-        } else if (element->hasTagName(HTMLNames::inputTag)) {
-            HTMLInputElement* inputElement = toHTMLInputElement(element);
-            if (inputElement->isImageButton() && inputElement->hasImageLoader()) {
-                KURL url = inputElement->src();
-                ImageResource* cachedImage = inputElement->imageLoader()->image();
-                addImageToResources(cachedImage, inputElement->renderer(), url);
+        if (element.hasTagName(HTMLNames::imgTag)) {
+            const HTMLImageElement& imageElement = toHTMLImageElement(element);
+            KURL url = document.completeURL(imageElement.getAttribute(HTMLNames::srcAttr));
+            ImageResource* cachedImage = imageElement.cachedImage();
+            addImageToResources(cachedImage, imageElement.renderer(), url);
+        } else if (element.hasTagName(HTMLNames::inputTag)) {
+            const HTMLInputElement& inputElement = toHTMLInputElement(element);
+            if (inputElement.isImageButton() && inputElement.hasImageLoader()) {
+                KURL url = inputElement.src();
+                ImageResource* cachedImage = const_cast<HTMLInputElement&>(inputElement).imageLoader()->image();
+                addImageToResources(cachedImage, inputElement.renderer(), url);
             }
-        } else if (element->hasTagName(HTMLNames::linkTag)) {
-            HTMLLinkElement* linkElement = toHTMLLinkElement(element);
-            if (CSSStyleSheet* sheet = linkElement->sheet()) {
-                KURL url = document->completeURL(linkElement->getAttribute(HTMLNames::hrefAttr));
+        } else if (element.hasTagName(HTMLNames::linkTag)) {
+            const HTMLLinkElement& linkElement = toHTMLLinkElement(element);
+            if (CSSStyleSheet* sheet = linkElement.sheet()) {
+                KURL url = document.completeURL(linkElement.getAttribute(HTMLNames::hrefAttr));
                 serializeCSSStyleSheet(sheet, url);
                 ASSERT(m_resourceURLs.contains(url));
             }
-        } else if (element->hasTagName(HTMLNames::styleTag)) {
-            HTMLStyleElement* styleElement = toHTMLStyleElement(element);
-            if (CSSStyleSheet* sheet = styleElement->sheet())
+        } else if (element.hasTagName(HTMLNames::styleTag)) {
+            const HTMLStyleElement& styleElement = toHTMLStyleElement(element);
+            if (CSSStyleSheet* sheet = styleElement.sheet())
                 serializeCSSStyleSheet(sheet, KURL());
         }
     }
 
     for (Frame* childFrame = frame->tree().firstChild(); childFrame; childFrame = childFrame->tree().nextSibling())
         serializeFrame(childFrame);
 }
 
 void PageSerializer::serializeCSSStyleSheet(CSSStyleSheet* styleSheet, const KURL& url)
 {
     StringBuilder cssText;
     for (unsigned i = 0; i < styleSheet->length(); ++i) {
         CSSRule* rule = styleSheet->item(i);
         String itemText = rule->cssText();
         if (!itemText.isEmpty()) {
             cssText.append(itemText);
             if (i < styleSheet->length() - 1)
                 cssText.append("\n\n");
         }
-        Document* document = styleSheet->ownerDocument();
+        ASSERT(styleSheet->ownerDocument());

[adamk, 2014/01/28 22:12:35]

This makes me wonder when ownerDocument() can return null...

[Inactive, 2014/01/29 17:13:21]

Based on its implementation, it can. I don't see any reason why it could not
return null here. I think we should add a null check, what do you think?

[adamk, 2014/01/29 17:26:26]

Sorry, I was too vague. I don't think it's possible for ownerDocument to be NULL
here, so the ASSERT seems better to me (if we're serializing a stylesheet, I'd
really think it's associated with a document). But there's no need to ASSERT
each time through the loop, let's move that ASSERT up to the to of the method.
Sound ok? If you'd feel better with the bailout, move that to the top and add a
FIXME.

+        Document& document = *styleSheet->ownerDocument();
         // Some rules have resources associated with them that we need to retrieve.
         if (rule->type() == CSSRule::IMPORT_RULE) {
             CSSImportRule* importRule = toCSSImportRule(rule);
-            KURL importURL = document->completeURL(importRule->href());
+            KURL importURL = document.completeURL(importRule->href());
             if (m_resourceURLs.contains(importURL))
                 continue;
             serializeCSSStyleSheet(importRule->styleSheet(), importURL);
         } else if (rule->type() == CSSRule::FONT_FACE_RULE) {
             retrieveResourcesForProperties(toCSSFontFaceRule(rule)->styleRule()->properties(), document);
         } else if (rule->type() == CSSRule::STYLE_RULE) {
             retrieveResourcesForProperties(toCSSStyleRule(rule)->styleRule()->properties(), document);
         }
     }
 
@@ skipping 43 matching lines @@
 void PageSerializer::addFontToResources(FontResource* font)
 {
     if (!font || !shouldAddURL(font->url()) || !font->isLoaded() || !font->resourceBuffer()) {
         return;
     }
     RefPtr<SharedBuffer> data(font->resourceBuffer());
 
     addToResources(font, data, font->url());
 }
 
-void PageSerializer::retrieveResourcesForProperties(const StylePropertySet* styleDeclaration, Document* document)
+void PageSerializer::retrieveResourcesForProperties(const StylePropertySet* styleDeclaration, Document& document)
 {
     if (!styleDeclaration)
         return;
 
     // The background-image and list-style-image (for ul or ol) are the CSS properties
     // that make use of images. We iterate to make sure we include any other
     // image properties there might be.
     unsigned propertyCount = styleDeclaration->propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i) {
         RefPtr<CSSValue> cssValue = styleDeclaration->propertyAt(i).value();
         retrieveResourcesForCSSValue(cssValue.get(), document);
     }
 }
 
-void PageSerializer::retrieveResourcesForCSSValue(CSSValue* cssValue, Document* document)
+void PageSerializer::retrieveResourcesForCSSValue(CSSValue* cssValue, Document& document)
 {
     if (cssValue->isImageValue()) {
         CSSImageValue* imageValue = toCSSImageValue(cssValue);
         StyleImage* styleImage = imageValue->cachedOrPendingImage();
         // Non cached-images are just place-holders and do not contain data.
         if (!styleImage || !styleImage->isImageResource())
             return;
 
         addImageToResources(styleImage->cachedImage(), 0, styleImage->cachedImage()->url());
     } else if (cssValue->isFontFaceSrcValue()) {
         CSSFontFaceSrcValue* fontFaceSrcValue = toCSSFontFaceSrcValue(cssValue);
         if (fontFaceSrcValue->isLocal()) {
             return;
         }
 
-        addFontToResources(fontFaceSrcValue->fetch(document));
+        addFontToResources(fontFaceSrcValue->fetch(&document));
     } else if (cssValue->isValueList()) {
         CSSValueList* cssValueList = toCSSValueList(cssValue);
         for (unsigned i = 0; i < cssValueList->length(); i++)
             retrieveResourcesForCSSValue(cssValueList->item(i), document);
     }
 }
 
 KURL PageSerializer::urlForBlankFrame(Frame* frame)
 {
     HashMap<Frame*, KURL>::iterator iter = m_blankFrameURLs.find(frame);
     if (iter != m_blankFrameURLs.end())
         return iter->value;
     String url = "wyciwyg://frame/" + String::number(m_blankFrameCounter++);
     KURL fakeURL(ParsedURLString, url);
     m_blankFrameURLs.add(frame, fakeURL);
 
     return fakeURL;
 }
 
 }