4.8 of 5: Xml Fixes for Merger Integration

* SimpleXmlResourceValue records all attributes except name. (I'd like to toss description, just as soon as I know I can -- bloats the serialized form.)
* Added handling for <item> to SimpleXmlResourceValue: this can be used as placeholders, or (apparently) mostly unvalidated value definitions.
* drawable can be used to put in colored backgrounds in a values.xml
* Strings, Plurals, need to read sub tags such as xliff as string
* <eat-comment/> and <skip/> need to be handled: we ignore them as all comments are eaten right now; and skip appears to be for localization diffs
* xml parser must be resilient to random characters in resources defined in values.
* Handles the <resource> not <resources> coding mistake gracefully.
* Supports a <public> declaration. This feature is more wildly used than the nonexistant documentation suggests.

--
MOS_MIGRATED_REVID=121490019

11 files changed, 407 insertions, 105 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/DataResource.java b/src/tools/android/java/com/google/devtools/build/android/DataResource.java
index 98fef179ba..343c0a9222 100644
--- a/src/tools/android/java/com/google/devtools/build/android/DataResource.java
+++ b/src/tools/android/java/com/google/devtools/build/android/DataResource.java
@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.android;
 
+import com.android.ide.common.res2.MergingException;
+
 import java.io.IOException;
 
 /**
@@ -23,5 +25,5 @@ public interface DataResource extends DataValue {
    * Write as a resource using the supplied {@link MergeDataWriter}.
    */
   void writeResource(FullyQualifiedName key, AndroidDataWritingVisitor mergedDataWriter)
-      throws IOException;
+      throws IOException, MergingException;
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/DataResourceXml.java b/src/tools/android/java/com/google/devtools/build/android/DataResourceXml.java
index 8e79ed4738..f4e4f814fc 100644
--- a/src/tools/android/java/com/google/devtools/build/android/DataResourceXml.java
+++ b/src/tools/android/java/com/google/devtools/build/android/DataResourceXml.java
@@ -79,27 +79,40 @@ public class DataResourceXml implements DataResource {
       throws XMLStreamException, FactoryConfigurationError, IOException {
     XMLEventReader eventReader =
         xmlInputFactory.createXMLEventReader(Files.newBufferedReader(path, StandardCharsets.UTF_8));
-    // TODO(corysmith): Make the xml parsing more readable.
-    while (XmlResourceValues.moveToResources(eventReader)) {
-      for (StartElement start = XmlResourceValues.findNextStart(eventReader);
-          start != null;
-          start = XmlResourceValues.findNextStart(eventReader)) {
-        ResourceType resourceType = getResourceType(start);
-        if (resourceType == DECLARE_STYLEABLE) {
-          // Styleables are special, as they produce multiple overwrite and non-overwrite values,
-          // so we let the value handle the assignments.
-          XmlResourceValues.parseDeclareStyleable(
-              fqnFactory, path, overwritingConsumer, nonOverwritingConsumer, eventReader, start);
-        } else {
-          // Of simple resources, only IDs are nonOverwriting.
-          KeyValueConsumer<DataKey, DataResource> consumer =
-              resourceType == ID ? nonOverwritingConsumer : overwritingConsumer;
-          FullyQualifiedName key =
-              fqnFactory.create(resourceType, XmlResourceValues.getElementName(start));
-          consumer.consume(
-              key, DataResourceXml.of(path, parseXmlElements(resourceType, eventReader, start)));
+    try {
+      // TODO(corysmith): Make the xml parsing more readable.
+      while (XmlResourceValues.moveToResources(eventReader)) {
+        for (StartElement start = XmlResourceValues.findNextStart(eventReader);
+            start != null;
+            start = XmlResourceValues.findNextStart(eventReader)) {
+          if (XmlResourceValues.isEatComment(start) || XmlResourceValues.isSkip(start)) {
+            continue;
+          }
+          ResourceType resourceType = getResourceType(start);
+          if (resourceType == null) {
+            throw new XMLStreamException(
+                path + " contains an unrecognized resource type:" + start, start.getLocation());
+          }
+          if (resourceType == DECLARE_STYLEABLE) {
+            // Styleables are special, as they produce multiple overwrite and non-overwrite values,
+            // so we let the value handle the assignments.
+            XmlResourceValues.parseDeclareStyleable(
+                fqnFactory, path, overwritingConsumer, nonOverwritingConsumer, eventReader, start);
+          } else {
+            // Of simple resources, only IDs are nonOverwriting.
+            KeyValueConsumer<DataKey, DataResource> consumer =
+                resourceType == ID ? nonOverwritingConsumer : overwritingConsumer;
+            FullyQualifiedName key =
+                fqnFactory.create(resourceType, XmlResourceValues.getElementName(start));
+            consumer.consume(
+                key, DataResourceXml.of(path, parseXmlElements(resourceType, eventReader, start)));
+          }
         }
       }
+    } catch (XMLStreamException e) {
+      throw new XMLStreamException(path + ":" + e.getMessage(), e.getLocation(), e);
+    } catch (RuntimeException e) {
+      throw new RuntimeException("Error parsing " + path, e);
     }
   }
 
@@ -136,6 +149,14 @@ public class DataResourceXml implements DataResource {
   private static XmlResourceValue parseXmlElements(
       ResourceType resourceType, XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
+    // Handle ids first, as they are a special kind of item.
+    if (resourceType == ID) {
+      return XmlResourceValues.parseId();
+    }
+    // Handle item stubs.
+    if (XmlResourceValues.isItem(start)) {
+      return XmlResourceValues.parseSimple(eventReader, resourceType, start); 
+    }
     switch (resourceType) {
       case STYLE:
         return XmlResourceValues.parseStyle(eventReader, start);
@@ -145,13 +166,26 @@ public class DataResourceXml implements DataResource {
         return XmlResourceValues.parsePlurals(eventReader);
       case ATTR:
         return XmlResourceValues.parseAttr(eventReader, start);
-      case ID:
-        return XmlResourceValues.parseId();
+      case LAYOUT:
+      case DIMEN:
       case STRING:
       case BOOL:
       case COLOR:
-      case DIMEN:
-        return XmlResourceValues.parseSimple(eventReader, resourceType, start.getName());
+      case FRACTION:
+      case INTEGER:
+      case DRAWABLE:
+      case ANIM:
+      case ANIMATOR:
+      case DECLARE_STYLEABLE:
+      case INTERPOLATOR:
+      case MENU:
+      case MIPMAP:
+      case PUBLIC:
+      case RAW:
+      case STYLEABLE:
+      case TRANSITION:
+      case XML:
+        return XmlResourceValues.parseSimple(eventReader, resourceType, start);
       default:
         throw new XMLStreamException(
             String.format("Unhandled resourceType %s", resourceType), start.getLocation());
diff --git a/src/tools/android/java/com/google/devtools/build/android/DataValueFile.java b/src/tools/android/java/com/google/devtools/build/android/DataValueFile.java
index 91fcc57dc4..5ea921b025 100644
--- a/src/tools/android/java/com/google/devtools/build/android/DataValueFile.java
+++ b/src/tools/android/java/com/google/devtools/build/android/DataValueFile.java
@@ -17,6 +17,8 @@ import com.google.common.base.MoreObjects;
 import com.google.devtools.build.android.proto.SerializeFormat;
 import com.google.protobuf.CodedOutputStream;
 
+import com.android.ide.common.res2.MergingException;
+
 import java.io.IOException;
 import java.io.OutputStream;
 import java.nio.file.FileSystem;
@@ -80,7 +82,7 @@ public class DataValueFile implements DataResource, DataAsset {
 
   @Override
   public void writeResource(FullyQualifiedName key, AndroidDataWritingVisitor mergedDataWriter)
-      throws IOException {
+      throws IOException, MergingException {
     mergedDataWriter.copyResource(source, key.toPathString(getSourceExtension()));
   }
 
@@ -95,7 +97,7 @@ public class DataValueFile implements DataResource, DataAsset {
   }
 
   private String getSourceExtension() {
-    // TODO(corysmith): Switch to a filename parser utility.
+    // TODO(corysmith): Find out if there is a filename parser utility.
     String fileName = source.getFileName().toString();
     int extensionStart = fileName.lastIndexOf('.');
     if (extensionStart > 0) {
diff --git a/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java b/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
index 1ed13c1057..978a4d0a69 100644
--- a/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
+++ b/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
@@ -32,13 +32,19 @@ import java.io.OutputStream;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.logging.Logger;
 
+import javax.annotation.Nullable;
 import javax.xml.namespace.QName;
 import javax.xml.stream.XMLEventReader;
 import javax.xml.stream.XMLStreamException;
 import javax.xml.stream.events.Attribute;
+import javax.xml.stream.events.Characters;
+import javax.xml.stream.events.EndElement;
 import javax.xml.stream.events.StartElement;
 import javax.xml.stream.events.XMLEvent;
 
@@ -50,13 +56,16 @@ import javax.xml.stream.events.XMLEvent;
  * declared inside the &lt;resources&gt; tag.
  */
 public class XmlResourceValues {
+  private static final Logger logger = Logger.getLogger(XmlResourceValues.class.getCanonicalName());
 
+  private static final QName TAG_EAT_COMMENT = QName.valueOf("eat-comment");
   private static final QName TAG_PLURALS = QName.valueOf("plurals");
   private static final QName ATTR_QUANTITY = QName.valueOf("quantity");
   private static final QName TAG_ATTR = QName.valueOf("attr");
   private static final QName TAG_DECLARE_STYLEABLE = QName.valueOf("declare-styleable");
   private static final QName TAG_ITEM = QName.valueOf("item");
   private static final QName TAG_STYLE = QName.valueOf("style");
+  private static final QName TAG_SKIP = QName.valueOf("skip");
   private static final QName TAG_RESOURCES = QName.valueOf("resources");
   private static final QName ATTR_FORMAT = QName.valueOf("format");
   private static final QName ATTR_NAME = QName.valueOf("name");
@@ -66,13 +75,18 @@ public class XmlResourceValues {
 
   static XmlResourceValue parsePlurals(XMLEventReader eventReader) throws XMLStreamException {
     ImmutableMap.Builder<String, String> values = ImmutableMap.builder();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_PLURALS);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isItem(element)) {
+        if (!element.isStartElement()) {
+          throw new XMLStreamException(
+              String.format("Expected start element %s", element), element.getLocation());
+        }
+        String contents = readContentsAsString(eventReader, element.asStartElement().getName());
         values.put(
             getElementAttributeByName(element.asStartElement(), ATTR_QUANTITY),
-            eventReader.getElementText());
+            contents == null ? "" : contents);
       }
     }
     return PluralXmlResourceValue.of(values.build());
@@ -81,14 +95,16 @@ public class XmlResourceValues {
   static XmlResourceValue parseStyle(XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
     Map<String, String> values = new HashMap<>();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_STYLE);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isItem(element)) {
         values.put(getElementName(element.asStartElement()), eventReader.getElementText());
       }
     }
-    return StyleXmlResourceValue.of(parseReferenceFromElementAttribute(start, ATTR_PARENT), values);
+    String parent = parseReferenceFromElementAttribute(start, ATTR_PARENT, false);
+    // Parents can be very lazily declared as just: <resource name>
+    return StyleXmlResourceValue.of(parent, values);
   }
 
   static void parseDeclareStyleable(
@@ -100,15 +116,22 @@ public class XmlResourceValues {
       StartElement start)
       throws XMLStreamException {
     List<String> members = new ArrayList<>();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_DECLARE_STYLEABLE);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isStartTag(element, TAG_ATTR)) {
         StartElement attr = element.asStartElement();
-        members.add(getElementName(attr));
-        overwritingConsumer.consume(
-            fqnFactory.create(ResourceType.ATTR, getElementName(attr)),
-            DataResourceXml.of(path, parseAttr(eventReader, start)));
+        String attrName = getElementName(attr);
+        members.add(attrName);
+        // If there is format and the next tag is a starting tag, treat it as an attr definition.
+        // Without those, it will be an attr reference.
+        if (XmlResourceValues.getElementAttributeByName(attr, ATTR_FORMAT) != null
+            || (XmlResourceValues.peekNextTag(eventReader) != null
+                && XmlResourceValues.peekNextTag(eventReader).isStartElement())) {
+          overwritingConsumer.consume(
+              fqnFactory.create(ResourceType.ATTR, attrName),
+              DataResourceXml.of(path, parseAttr(eventReader, attr)));
+        }
       }
     }
     nonOverwritingConsumer.consume(
@@ -118,8 +141,10 @@ public class XmlResourceValues {
 
   static XmlResourceValue parseAttr(XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
-    return AttrXmlResourceValue.from(
-        start, getElementAttributeByName(start, ATTR_FORMAT), eventReader);
+    XmlResourceValue value =
+        AttrXmlResourceValue.from(
+            start, getElementAttributeByName(start, ATTR_FORMAT), eventReader);
+    return value;
   }
 
   static XmlResourceValue parseId() {
@@ -127,14 +152,72 @@ public class XmlResourceValues {
   }
 
   static XmlResourceValue parseSimple(
-      XMLEventReader eventReader, ResourceType resourceType, QName startTag)
+      XMLEventReader eventReader, ResourceType resourceType, StartElement start)
+      throws XMLStreamException {
+    // Using a map to deduplicate xmlns declarations on the attributes.
+    Map<String, String> attributeMap = new LinkedHashMap<>();
+    @SuppressWarnings({
+      "cast",
+      "unchecked"
+    }) // The interface returns Iterator, force casting based on documentation.
+    Iterator<Attribute> attributes = (Iterator<Attribute>) start.getAttributes();
+    while (attributes.hasNext()) {
+      Attribute attribute = attributes.next();
+      QName name = attribute.getName();
+      // Name used as the resource key, so skip it here.
+      if (ATTR_NAME.equals(name)) {
+        continue;
+      }
+      String value = escapeXmlValues(attribute.getValue()).replace("\"", "&quot;");
+      if (!name.getNamespaceURI().isEmpty()) {
+        // Declare the xmlns here, so that the written xml will be semantically correct,
+        // if a bit verbose. This allows the resource keys to be written into a generic <resources>
+        // tag.
+        attributeMap.put("xmlns:" + name.getPrefix(), name.getNamespaceURI());
+        attributeMap.put(name.getPrefix() + ":" + attribute.getName().getLocalPart(), value);
+      } else {
+        attributeMap.put(attribute.getName().getLocalPart(), value);
+      }
+    }
+    String contents;
+    // Check and see if the element is unary. If it is, the contents is null
+    if (isEndTag(eventReader.peek(), start.getName())) {
+      contents = null;
+    } else {
+      contents = readContentsAsString(eventReader, start.getName());
+    }
+    return SimpleXmlResourceValue.of(
+        start.getName().equals(TAG_ITEM)
+            ? SimpleXmlResourceValue.Type.ITEM
+            : SimpleXmlResourceValue.Type.from(resourceType),
+        ImmutableMap.copyOf(attributeMap),
+        contents);
+  }
+
+  // TODO(corysmith): Replace this with real escaping system, preferably a performant high level xml
+  //writing library. See AndroidDataWritingVisitor TODO.
+  private static String escapeXmlValues(String data) {
+    return data.replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;");
+  }
+
+  /**
+   * Reads the xml events as a string until finding a closing tag.
+   *
+   * @param eventReader The current xml stream.
+   * @param startTag The name of the tag to close on.
+   * @return A xml escaped string representation of the xml stream
+   */
+  @Nullable
+  public static String readContentsAsString(XMLEventReader eventReader, QName startTag)
       throws XMLStreamException {
     StringBuilder contents = new StringBuilder();
     while (!isEndTag(eventReader.peek(), startTag)) {
       XMLEvent xmlEvent = eventReader.nextEvent();
       if (xmlEvent.isCharacters()) {
-        contents.append(xmlEvent.asCharacters().getData());
+        contents.append(escapeXmlValues(xmlEvent.asCharacters().getData()));
       } else if (xmlEvent.isStartElement()) {
+        // TODO(corysmith): Replace this with a proper representation of the contents that can be
+        // serialized and reconstructed appropriately without modification.
         QName name = xmlEvent.asStartElement().getName();
         contents.append("<");
         if (!name.getNamespaceURI().isEmpty()) {
@@ -163,24 +246,32 @@ public class XmlResourceValues {
         contents.append(">");
       }
     }
-    Preconditions.checkArgument(eventReader.nextEvent().asEndElement().getName().equals(startTag));
-    return SimpleXmlResourceValue.of(
-        SimpleXmlResourceValue.Type.from(resourceType), contents.toString());
+    // Verify the end element.
+    EndElement endElement = eventReader.nextEvent().asEndElement();
+    Preconditions.checkArgument(endElement.getName().equals(startTag));
+    return contents.toString();
   }
 
   /* XML helper methods follow. */
   // TODO(corysmith): Move these to a wrapper class for XMLEventReader.
-  private static String parseReferenceFromElementAttribute(StartElement element, QName name)
-      throws XMLStreamException {
+  private static String parseReferenceFromElementAttribute(
+      StartElement element, QName name, boolean requiresPrefix) throws XMLStreamException {
     String value = getElementAttributeByName(element, name);
+    if (value == null) {
+      return null;
+    }
     if (value.startsWith("?") || value.startsWith("@")) {
       return value.substring(1);
     }
+    if (!requiresPrefix) {
+      return value;
+    }
     throw new XMLStreamException(
         String.format("Invalid resource reference from %s in %s", name, element),
         element.getLocation());
   }
 
+  @Nullable
   public static String getElementAttributeByName(StartElement element, QName name) {
     Attribute attribute = element.getAttributeByName(name);
     return attribute == null ? null : attribute.getValue();
@@ -226,13 +317,34 @@ public class XmlResourceValues {
     return false;
   }
 
+  public static XMLEvent nextTag(XMLEventReader eventReader) throws XMLStreamException {
+    while (eventReader.hasNext()
+        && !(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
+      XMLEvent nextEvent = eventReader.nextEvent();
+      if (nextEvent.isCharacters() && !nextEvent.asCharacters().isIgnorableWhiteSpace()) {
+        Characters characters = nextEvent.asCharacters();
+        // TODO(corysmith): Turn into a warning with the Path is available to add to it.
+        // This case is when unexpected characters are thrown into the xml. Best case, it's a
+        // incorrect comment type...
+        logger.fine(
+            String.format(
+                "Invalid characters [%s] found at %s",
+                characters.getData(),
+                characters.getLocation().getLineNumber()));
+      }
+    }
+    return eventReader.nextEvent();
+  }
+
   public static XMLEvent peekNextTag(XMLEventReader eventReader) throws XMLStreamException {
-    while (!(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
+    while (eventReader.hasNext()
+        && !(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
       eventReader.nextEvent();
     }
     return eventReader.peek();
   }
 
+  @Nullable
   static StartElement findNextStart(XMLEventReader eventReader) throws XMLStreamException {
     while (eventReader.hasNext()) {
       XMLEvent event = eventReader.nextEvent();
@@ -245,14 +357,15 @@ public class XmlResourceValues {
 
   static boolean moveToResources(XMLEventReader eventReader) throws XMLStreamException {
     while (eventReader.hasNext()) {
-      if (findNextStart(eventReader).getName().equals(TAG_RESOURCES)) {
+      StartElement next = findNextStart(eventReader);
+      if (next != null && next.getName().equals(TAG_RESOURCES)) {
         return true;
       }
     }
     return false;
   }
 
-  public static SerializeFormat.DataValue.Builder newProtoDataBuilder(Path source) {
+  public static SerializeFormat.DataValue.Builder newSerializableDataValueBuilder(Path source) {
     SerializeFormat.DataValue.Builder builder = SerializeFormat.DataValue.newBuilder();
     return builder.setSource(builder.getSourceBuilder().setFilename(source.toString()));
   }
@@ -264,4 +377,12 @@ public class XmlResourceValues {
     return CodedOutputStream.computeUInt32SizeNoTag(value.getSerializedSize())
         + value.getSerializedSize();
   }
+
+  public static boolean isEatComment(StartElement start) {
+    return isTag(start, TAG_EAT_COMMENT);
+  }
+
+  public static boolean isSkip(StartElement start) {
+    return isTag(start, TAG_SKIP);
+  }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/ArrayXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/ArrayXmlResourceValue.java
index edb30267f7..4e26ec9993 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/ArrayXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/ArrayXmlResourceValue.java
@@ -139,7 +139,7 @@ public class ArrayXmlResourceValue implements XmlResourceValue {
   public int serializeTo(Path source, OutputStream output) throws IOException {
     return XmlResourceValues.serializeProtoDataValue(
         output,
-        XmlResourceValues.newProtoDataBuilder(source)
+        XmlResourceValues.newSerializableDataValueBuilder(source)
             .setXmlValue(
                 SerializeFormat.DataValueXml.newBuilder()
                     .addAllListValue(values)
@@ -172,13 +172,23 @@ public class ArrayXmlResourceValue implements XmlResourceValue {
   public static XmlResourceValue parseArray(XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
     List<String> values = new ArrayList<>();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = XmlResourceValues.nextTag(eventReader);
         !XmlResourceValues.isEndTag(element, start.getName());
-        element = eventReader.nextTag()) {
+        element = XmlResourceValues.nextTag(eventReader)) {
       if (XmlResourceValues.isItem(element)) {
-        values.add(eventReader.getElementText());
+        if (!element.isStartElement()) {
+          throw new XMLStreamException(
+              String.format("Expected start element %s", element), element.getLocation());
+        }
+        String contents = XmlResourceValues.readContentsAsString(eventReader,
+            element.asStartElement().getName());
+        values.add(contents != null ? contents : "");
       }
     }
-    return of(ArrayType.fromTagName(start), values);
+    try {
+      return of(ArrayType.fromTagName(start), values);
+    } catch (IllegalArgumentException e) {
+      throw new XMLStreamException(e.getMessage(), start.getLocation());
+    }
   }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/AttrXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/AttrXmlResourceValue.java
index f8273b37e6..ab2e314af0 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/AttrXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/AttrXmlResourceValue.java
@@ -84,9 +84,9 @@ public class AttrXmlResourceValue implements XmlResourceValue {
   private static final String COLOR = "color";
   private static final String REFERENCE = "reference";
   private static final String ENUM = "enum";
-  private static final String FLAG = "flag";
+  private static final String FLAGS = "flags";
   private static final QName TAG_ENUM = QName.valueOf(ENUM);
-  private static final QName TAG_FLAG = QName.valueOf(FLAG);
+  private static final QName TAG_FLAG = QName.valueOf("flag");
   private final ImmutableMap<String, ResourceXmlAttrValue> formats;
 
   private AttrXmlResourceValue(ImmutableMap<String, ResourceXmlAttrValue> formats) {
@@ -155,7 +155,7 @@ public class AttrXmlResourceValue implements XmlResourceValue {
         ImmutableMap.<String, AttrXmlResourceValue.ResourceXmlAttrValue>builder();
     for (Entry<String, SerializeFormat.DataValueXml> entry : proto.getMappedXmlValue().entrySet()) {
       switch (entry.getKey()) {
-        case FLAG:
+        case FLAGS:
           formats.put(
               entry.getKey(), FlagResourceXmlAttrValue.of(entry.getValue().getMappedStringValue()));
           break;
@@ -194,6 +194,10 @@ public class AttrXmlResourceValue implements XmlResourceValue {
     return of(formats.build());
   }
 
+  /**
+   * Creates a new {@link AttrXmlResourceValue}. Returns null if there are no formats.
+   */
+  @Nullable
   public static XmlResourceValue from(
       StartElement attr, @Nullable String format, XMLEventReader eventReader)
       throws XMLStreamException {
@@ -203,13 +207,18 @@ public class AttrXmlResourceValue implements XmlResourceValue {
     }
     XMLEvent nextTag = XmlResourceValues.peekNextTag(eventReader);
     if (nextTag != null && nextTag.isStartElement()) {
-      formatNames.add(nextTag.asStartElement().getName().getLocalPart().toLowerCase());
+      QName tagName = nextTag.asStartElement().getName();
+      if (TAG_FLAG.equals(tagName)) {
+        formatNames.add(FLAGS);
+      } else {
+        formatNames.add(tagName.getLocalPart().toLowerCase());
+      }
     }
 
     Builder<String, ResourceXmlAttrValue> formats = ImmutableMap.builder();
     for (String formatName : formatNames) {
       switch (formatName) {
-        case FLAG:
+        case FLAGS:
           Map<String, String> flags = readSubValues(eventReader, TAG_FLAG);
           endAttrElement(eventReader);
           formats.put(formatName, FlagResourceXmlAttrValue.of(flags));
@@ -285,8 +294,12 @@ public class AttrXmlResourceValue implements XmlResourceValue {
         FluentIterable.from(
             ImmutableList.of(
                 String.format("<!-- %s -->", source),
-                String.format(
-                    "<attr name='%s' format='%s'>", key.name(), Joiner.on('|').join(formatKeys))));
+                formatKeys.isEmpty()
+                    ? String.format("<attr name='%s'>", key.name())
+                    : String.format(
+                        "<attr name='%s' format='%s'>",
+                        key.name(),
+                        Joiner.on('|').join(formatKeys))));
     for (String formatKey : formatKeys) {
       iterable = formats.get(formatKey).appendTo(iterable);
     }
@@ -295,7 +308,8 @@ public class AttrXmlResourceValue implements XmlResourceValue {
 
   @Override
   public int serializeTo(Path source, OutputStream output) throws IOException {
-    SerializeFormat.DataValue.Builder builder = XmlResourceValues.newProtoDataBuilder(source);
+    SerializeFormat.DataValue.Builder builder =
+        XmlResourceValues.newSerializableDataValueBuilder(source);
     SerializeFormat.DataValueXml.Builder xmlValueBuilder =
         SerializeFormat.DataValueXml.newBuilder();
     xmlValueBuilder.setType(SerializeFormat.DataValueXml.XmlType.ATTR);
@@ -403,7 +417,7 @@ public class AttrXmlResourceValue implements XmlResourceValue {
       for (int i = 0; i < keyThenValue.length; i += 2) {
         builder.put(keyThenValue[i], keyThenValue[i + 1]);
       }
-      return new BuilderEntry(FLAG, of(builder.build()));
+      return new BuilderEntry(FLAGS, of(builder.build()));
     }
 
     @Override
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/IdXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/IdXmlResourceValue.java
index 07e3ad5940..05fe8729c0 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/IdXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/IdXmlResourceValue.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
 import com.google.devtools.build.android.proto.SerializeFormat;
+import com.google.devtools.build.android.proto.SerializeFormat.DataValueXml.XmlType;
 import com.google.protobuf.CodedOutputStream;
 
 import java.io.IOException;
@@ -60,10 +61,8 @@ public class IdXmlResourceValue implements XmlResourceValue {
   @Override
   public int serializeTo(Path source, OutputStream output) throws IOException {
     SerializeFormat.DataValue value =
-        XmlResourceValues.newProtoDataBuilder(source)
-            .setXmlValue(
-                SerializeFormat.DataValueXml.newBuilder()
-                    .setType(SerializeFormat.DataValueXml.XmlType.ID))
+        XmlResourceValues.newSerializableDataValueBuilder(source)
+            .setXmlValue(SerializeFormat.DataValueXml.newBuilder().setType(XmlType.ID))
             .build();
     value.writeDelimitedTo(output);
     return CodedOutputStream.computeUInt32SizeNoTag(value.getSerializedSize())
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/PluralXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/PluralXmlResourceValue.java
index 64397ef35f..4a814138e8 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/PluralXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/PluralXmlResourceValue.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
 import com.google.devtools.build.android.proto.SerializeFormat;
-import com.google.devtools.build.android.proto.SerializeFormat.DataValue.Builder;
 import com.google.devtools.build.android.proto.SerializeFormat.DataValueXml.XmlType;
 import com.google.protobuf.CodedOutputStream;
 
@@ -109,7 +108,8 @@ public class PluralXmlResourceValue implements XmlResourceValue {
 
   @Override
   public int serializeTo(Path source, OutputStream output) throws IOException {
-    Builder builder = XmlResourceValues.newProtoDataBuilder(source);
+    SerializeFormat.DataValue.Builder builder =
+        XmlResourceValues.newSerializableDataValueBuilder(source);
     SerializeFormat.DataValue value =
         builder
             .setXmlValue(
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/SimpleXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/SimpleXmlResourceValue.java
index ff8c6ad742..f618eb5004 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/SimpleXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/SimpleXmlResourceValue.java
@@ -15,11 +15,13 @@ package com.google.devtools.build.android.xml;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.android.AndroidDataWritingVisitor;
 import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
 import com.google.devtools.build.android.proto.SerializeFormat;
+import com.google.devtools.build.android.proto.SerializeFormat.DataValueXml.Builder;
 
 import com.android.resources.ResourceType;
 
@@ -27,8 +29,10 @@ import java.io.IOException;
 import java.io.OutputStream;
 import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.Map.Entry;
 import java.util.Objects;
 
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import javax.xml.namespace.QName;
 
@@ -46,19 +50,18 @@ import javax.xml.namespace.QName;
  */
 @Immutable
 public class SimpleXmlResourceValue implements XmlResourceValue {
-  static final QName TAG_STRING = QName.valueOf("string");
   static final QName TAG_BOOL = QName.valueOf("bool");
   static final QName TAG_COLOR = QName.valueOf("color");
   static final QName TAG_DIMEN = QName.valueOf("dimen");
+  static final QName TAG_DRAWABLE = QName.valueOf("drawable");
+  static final QName TAG_FRACTION = QName.valueOf("fraction");
+  static final QName TAG_INTEGER = QName.valueOf("integer");
+  static final QName TAG_ITEM = QName.valueOf("item");
+  static final QName TAG_PUBLIC = QName.valueOf("public");
+  static final QName TAG_STRING = QName.valueOf("string");
 
   /** Provides an enumeration resource type and simple value validation. */
   public enum Type {
-    STRING(TAG_STRING) {
-      @Override
-      public boolean validate(String value) {
-        return true;
-      }
-    },
     BOOL(TAG_BOOL) {
       @Override
       public boolean validate(String value) {
@@ -79,6 +82,47 @@ public class SimpleXmlResourceValue implements XmlResourceValue {
         // TODO(corysmith): Validate the dimension type.
         return true;
       }
+    },
+    DRAWABLE(TAG_DRAWABLE) {
+      @Override
+      public boolean validate(String value) {
+        // TODO(corysmith): Validate the drawable type.
+        return true;
+      }
+    },
+    FRACTION(TAG_FRACTION) {
+      @Override
+      public boolean validate(String value) {
+        // TODO(corysmith): Validate the fraction type.
+        return true;
+      }
+    },
+    INTEGER(TAG_INTEGER) {
+      @Override
+      public boolean validate(String value) {
+        // TODO(corysmith): Validate the integer type.
+        return true;
+      }
+    },
+    ITEM(TAG_ITEM) {
+      @Override
+      public boolean validate(String value) {
+        // TODO(corysmith): Validate the item type.
+        return true;
+      }
+    },
+    PUBLIC(TAG_PUBLIC) {
+      @Override
+      public boolean validate(String value) {
+        // TODO(corysmith): Validate the public type.
+        return true;
+      }
+    },
+    STRING(TAG_STRING) {
+      @Override
+      public boolean validate(String value) {
+        return true;
+      }
     };
     private QName tagName;
 
@@ -92,6 +136,8 @@ public class SimpleXmlResourceValue implements XmlResourceValue {
       for (Type valueType : values()) {
         if (valueType.tagName.getLocalPart().equals(resourceType.getName())) {
           return valueType;
+        } else if (resourceType.getName().equalsIgnoreCase(valueType.name())) {
+          return valueType;
         }
       }
       throw new IllegalArgumentException(
@@ -102,52 +148,109 @@ public class SimpleXmlResourceValue implements XmlResourceValue {
     }
   }
 
-  private final String value;
+  private final ImmutableMap<String, String> attributes;
+  @Nullable private final String value;
   private final Type valueType;
 
-  public static XmlResourceValue of(Type valueType, String value) {
-    return new SimpleXmlResourceValue(valueType, value);
+  public static XmlResourceValue createWithValue(Type valueType, String value) {
+    return of(valueType, ImmutableMap.<String, String>of(), value);
+  }
+
+  public static XmlResourceValue withAttributes(
+      Type valueType, ImmutableMap<String, String> attributes) {
+    return of(valueType, attributes, null);
   }
 
-  private SimpleXmlResourceValue(Type valueType, String value) {
+  public static XmlResourceValue itemWithFormattedValue(
+      ResourceType resourceType, String format, String value) {
+    return of(Type.ITEM, ImmutableMap.of("type", resourceType.getName(), "format", format), value);
+  }
+
+  public static XmlResourceValue itemWithValue(
+      ResourceType resourceType, String value) {
+    return of(Type.ITEM, ImmutableMap.of("type", resourceType.getName()), value);
+  }
+
+  public static XmlResourceValue itemPlaceHolderFor(ResourceType resourceType) {
+    return withAttributes(Type.ITEM, ImmutableMap.of("type", resourceType.getName()));
+  }
+
+  @Deprecated
+  public static XmlResourceValue of(Type valueType, @Nullable String value) {
+    return of(valueType, ImmutableMap.<String, String>of(), value);
+  }
+
+  public static XmlResourceValue of(
+      Type valueType, ImmutableMap<String, String> attributes, @Nullable String value) {
+    return new SimpleXmlResourceValue(valueType, attributes, value);
+  }
+
+  private SimpleXmlResourceValue(
+      Type valueType, ImmutableMap<String, String> attributes, String value) {
     this.valueType = valueType;
     this.value = value;
+    this.attributes = attributes;
   }
 
   @Override
   public void write(
       FullyQualifiedName key, Path source, AndroidDataWritingVisitor mergedDataWriter) {
+    StringBuilder xmlString =
+        new StringBuilder("<")
+            .append(valueType.tagName.getLocalPart())
+            .append(" name=\"")
+            .append(key.name())
+            .append("\"");
+    for (Entry<String, String> entry : attributes.entrySet()) {
+      xmlString
+          .append(" ")
+          .append(entry.getKey())
+          .append("=\"")
+          .append(entry.getValue())
+          .append("\"");
+    }
+    if (value != null) {
+      xmlString
+          .append(">")
+          .append(value)
+          .append("</")
+          .append(valueType.tagName.getLocalPart())
+          .append(">");
+    } else {
+      xmlString.append("/>");
+    }
     mergedDataWriter.writeToValuesXml(
-        key,
-        ImmutableList.of(
-            String.format("<!-- %s -->", source),
-            String.format(
-                "<%s name='%s'>%s</%s>",
-                valueType.tagName.getLocalPart(),
-                key.name(),
-                value,
-                valueType.tagName.getLocalPart())));
+        key, ImmutableList.of(String.format("<!-- %s -->", source), xmlString.toString()));
   }
 
   public static XmlResourceValue from(SerializeFormat.DataValueXml proto) {
-    return of(Type.valueOf(proto.getValueType()), proto.getValue());
+    return of(
+        Type.valueOf(proto.getValueType()),
+        ImmutableMap.copyOf(proto.getMappedStringValue()),
+        proto.hasValue() ? proto.getValue() : null);
   }
 
   @Override
   public int serializeTo(Path source, OutputStream output) throws IOException {
-    SerializeFormat.DataValue.Builder builder = XmlResourceValues.newProtoDataBuilder(source);
-    builder.setXmlValue(
+    SerializeFormat.DataValue.Builder builder =
+        XmlResourceValues.newSerializableDataValueBuilder(source);
+    Builder xmlValueBuilder =
         builder
             .getXmlValueBuilder()
             .setType(SerializeFormat.DataValueXml.XmlType.SIMPLE)
-            .setValue(value)
-            .setValueType(valueType.name()));
+            // TODO(corysmith): Find a way to avoid writing strings to the serialized format
+            // it's inefficient use of space and costs more when deserializing.
+            .putAllMappedStringValue(attributes);
+    if (value != null) {
+      xmlValueBuilder.setValue(value);
+    }
+    builder.setXmlValue(xmlValueBuilder.setValueType(valueType.name()));
     return XmlResourceValues.serializeProtoDataValue(output, builder);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(valueType, value);
+    return Objects.hash(valueType, attributes, value);
   }
 
   @Override
@@ -156,13 +259,16 @@ public class SimpleXmlResourceValue implements XmlResourceValue {
       return false;
     }
     SimpleXmlResourceValue other = (SimpleXmlResourceValue) obj;
-    return Objects.equals(valueType, other.valueType) && Objects.equals(value, other.value);
+    return Objects.equals(valueType, other.valueType)
+        && Objects.equals(attributes, attributes)
+        && Objects.equals(value, other.value);
   }
 
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(getClass())
         .add("valueType", valueType)
+        .add("attributes", attributes)
         .add("value", value)
         .toString();
   }
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/StyleXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/StyleXmlResourceValue.java
index cb73d07cd1..a7e07d6b56 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/StyleXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/StyleXmlResourceValue.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
 import com.google.devtools.build.android.proto.SerializeFormat;
+import com.google.devtools.build.android.proto.SerializeFormat.DataValueXml.XmlType;
 
 import java.io.IOException;
 import java.io.OutputStream;
@@ -85,24 +86,36 @@ public class StyleXmlResourceValue implements XmlResourceValue {
         FluentIterable.from(
                 ImmutableList.of(
                     String.format("<!-- %s -->", source),
-                    parent == null || parent.isEmpty()
-                        ? String.format("<style name='%s'>", key.name())
-                        : String.format("<style name='%s' parent='@%s'>", key.name(), parent)))
+                    String.format("<style name='%s' %s>", key.name(), parentAsXmlAttribute())))
             .append(FluentIterable.from(values.entrySet()).transform(ENTRY_TO_ITEM))
             .append("</style>"));
   }
 
+  private String parentAsXmlAttribute() {
+    if (parent == null) {
+      return "";
+    }
+    if (parent.isEmpty()) {
+      return "parent=''";
+    }
+    if (parent.startsWith("style/")) {
+      return "parent='@" + parent + "'";
+    }
+    return "parent='@style/" + parent + "'";
+  }
+
   @Override
   public int serializeTo(Path source, OutputStream output) throws IOException {
     SerializeFormat.DataValueXml.Builder xmlValueBuilder =
         SerializeFormat.DataValueXml.newBuilder()
-            .setType(SerializeFormat.DataValueXml.XmlType.STYLE)
+            .setType(XmlType.STYLE)
             .putAllMappedStringValue(values);
-    if (parent != null && !parent.isEmpty()) {
+    if (parent != null) {
       xmlValueBuilder.setValue(parent);
     }
     return XmlResourceValues.serializeProtoDataValue(
-        output, XmlResourceValues.newProtoDataBuilder(source).setXmlValue(xmlValueBuilder));
+        output,
+        XmlResourceValues.newSerializableDataValueBuilder(source).setXmlValue(xmlValueBuilder));
   }
 
   @Override
diff --git a/src/tools/android/java/com/google/devtools/build/android/xml/StyleableXmlResourceValue.java b/src/tools/android/java/com/google/devtools/build/android/xml/StyleableXmlResourceValue.java
index e46eaf3065..bed3536c80 100644
--- a/src/tools/android/java/com/google/devtools/build/android/xml/StyleableXmlResourceValue.java
+++ b/src/tools/android/java/com/google/devtools/build/android/xml/StyleableXmlResourceValue.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
 import com.google.devtools.build.android.proto.SerializeFormat;
+import com.google.devtools.build.android.proto.SerializeFormat.DataValueXml.XmlType;
 
 import java.io.IOException;
 import java.io.OutputStream;
@@ -101,10 +102,10 @@ public class StyleableXmlResourceValue implements XmlResourceValue {
   public int serializeTo(Path source, OutputStream output) throws IOException {
     return XmlResourceValues.serializeProtoDataValue(
         output,
-        XmlResourceValues.newProtoDataBuilder(source)
+        XmlResourceValues.newSerializableDataValueBuilder(source)
             .setXmlValue(
                 SerializeFormat.DataValueXml.newBuilder()
-                    .setType(SerializeFormat.DataValueXml.XmlType.STYLEABLE)
+                    .setType(XmlType.STYLEABLE)
                     .addAllListValue(attrs)));
   }