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

7 files changed, 198 insertions, 55 deletions

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)));
   }