Flip on @+id parsing by default

Avoid writing the non-values IdXmlResourceValues
to the merged values.xml file. It's redundant
since the merged resources will include the src
layout / menu, etc. file. E.g., adds 25KB out
of 250KB to some merged values.xml.

It can also change the way R.fields are initialized
(ids from values.xml are numbered before other
sources), that would have changed the numbering in
AndroidIntegratinoTest#testAndroidBinaryResourceShrinking
and in AndroidResourceClassWriterAaptTest.

--
MOS_MIGRATED_REVID=130789333

4 files changed, 41 insertions, 46 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
index 3cd7d2c45f..f330b95f9e 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
@@ -157,7 +157,6 @@ public class AndroidDataMerger {
 
   private final SourceChecker deDuplicator;
   private final ListeningExecutorService executorService;
-  private boolean parseIds;
 
   /** Creates a merger with no path deduplication and a default {@link ExecutorService}. */
   public static AndroidDataMerger createWithDefaults() {
@@ -187,10 +186,6 @@ public class AndroidDataMerger {
     this.executorService = executorService;
   }
 
-  void enableIdParsing() {
-    this.parseIds = true;
-  }
-
   /**
    * Merges a list of {@link DependencyAndroidData} with a {@link UnvalidatedAndroidData}.
    *
@@ -304,8 +299,7 @@ public class AndroidDataMerger {
 
     try {
       // Extract the primary resources.
-      ParsedAndroidData parsedPrimary = parseIds ? ParsedAndroidData.parseWithIds(primaryData)
-          : ParsedAndroidData.from(primaryData);
+      ParsedAndroidData parsedPrimary = ParsedAndroidData.from(primaryData);
 
       // Create the builders for the final parsed data.
       final ParsedAndroidData.Builder primaryBuilder = ParsedAndroidData.Builder.newBuilder();
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 13d44d9081..38e2f52124 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
@@ -17,6 +17,7 @@ import static com.android.resources.ResourceType.DECLARE_STYLEABLE;
 import static com.android.resources.ResourceType.ID;
 import static com.android.resources.ResourceType.PUBLIC;
 
+import com.android.SdkConstants;
 import com.android.resources.ResourceType;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
@@ -304,9 +305,29 @@ public class DataResourceXml implements DataResource {
       throw new IllegalArgumentException(resource + " is not a combinable with " + this);
     }
     DataResourceXml xmlResource = (DataResourceXml) resource;
-    // TODO(corysmith): Combine the sources so that we know both of the originating files.
-    // For right now, use the current source.
     return createWithNamespaces(
-        source, xml.combineWith(xmlResource.xml), namespaces.union(xmlResource.namespaces));
+        combineSources(xmlResource.source),
+        xml.combineWith(xmlResource.xml),
+        namespaces.union(xmlResource.namespaces));
+  }
+
+  private Path combineSources(Path otherSource) {
+    // TODO(corysmith): Combine the sources so that we know both of the originating files.
+    // For now, prefer sources that have explicit definitions (values/ and not layout/), since the
+    // values are ultimately written out to a merged values.xml. Sources from layout/menu, etc.
+    // can come from "@+id" definitions.
+    boolean thisInValuesFolder = isInValuesFolder(source);
+    boolean otherInValuesFolder = isInValuesFolder(otherSource);
+    if (thisInValuesFolder && !otherInValuesFolder) {
+      return source;
+    }
+    if (!thisInValuesFolder && otherInValuesFolder) {
+      return otherSource;
+    }
+    return source;
+  }
+
+  public static boolean isInValuesFolder(Path source) {
+    return source.getParent().getFileName().toString().startsWith(SdkConstants.FD_RES_VALUES);
   }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
index c5ca80d365..3a56088a4b 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
@@ -13,18 +13,16 @@
 // limitations under the License.
 package com.google.devtools.build.android;
 
+import com.android.SdkConstants;
+import com.android.ide.common.res2.MergingException;
+import com.android.resources.FolderTypeRelationship;
+import com.android.resources.ResourceFolderType;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.android.xml.StyleableXmlResourceValue;
-
-import com.android.SdkConstants;
-import com.android.ide.common.res2.MergingException;
-import com.android.resources.FolderTypeRelationship;
-import com.android.resources.ResourceFolderType;
-
 import java.io.IOException;
 import java.nio.file.FileVisitOption;
 import java.nio.file.FileVisitResult;
@@ -42,7 +40,6 @@ import java.util.Map.Entry;
 import java.util.Objects;
 import java.util.Set;
 import java.util.logging.Logger;
-
 import javax.annotation.concurrent.Immutable;
 import javax.annotation.concurrent.NotThreadSafe;
 import javax.xml.stream.XMLStreamException;
@@ -65,7 +62,6 @@ public class ParsedAndroidData {
     private final Map<DataKey, DataAsset> assets;
     private final Set<MergeConflict> conflicts;
     private final List<Exception> errors = new ArrayList<>();
-    private boolean parseIds;
 
     public Builder(
         Map<DataKey, DataResource> overwritingResources,
@@ -86,11 +82,6 @@ public class ParsedAndroidData {
       return new Builder(overwritingResources, combiningResources, assets, conflicts);
     }
 
-    Builder enableIdParsing() {
-      this.parseIds = true;
-      return this;
-    }
-
     private void checkForErrors() throws MergingException {
       if (!errors.isEmpty()) {
         MergingException mergingException =
@@ -130,8 +121,7 @@ public class ParsedAndroidData {
       return new ResourceFileVisitor(
           new OverwritableConsumer<>(overwritingResources, conflicts),
           new CombiningConsumer(combiningResources),
-          errors,
-          parseIds);
+          errors);
     }
 
     AssetFileVisitor assetVisitorFor(Path path) {
@@ -257,7 +247,6 @@ public class ParsedAndroidData {
     private final List<Exception> errors;
     private ResourceFolderType folderType;
     private FullyQualifiedName.Factory fqnFactory;
-    private final boolean parseIds;
 
     /**
      * Resource folders with XML files that may contain "@+id".
@@ -273,12 +262,10 @@ public class ParsedAndroidData {
     ResourceFileVisitor(
         KeyValueConsumer<DataKey, DataResource> overwritingConsumer,
         KeyValueConsumer<DataKey, DataResource> combiningResources,
-        List<Exception> errors,
-        boolean parseIds) {
+        List<Exception> errors) {
       this.overwritingConsumer = overwritingConsumer;
       this.combiningResources = combiningResources;
       this.errors = errors;
-      this.parseIds = parseIds;
     }
 
     @Override
@@ -312,8 +299,7 @@ public class ParsedAndroidData {
                 combiningResources);
           } else if (folderType != null) {
             FullyQualifiedName key = fqnFactory.parse(path);
-            if (parseIds
-                && ID_PROVIDING_RESOURCE_TYPES.contains(folderType)
+            if (ID_PROVIDING_RESOURCE_TYPES.contains(folderType)
                 && path.getFileName().toString().endsWith(SdkConstants.DOT_XML)) {
               DataValueFileWithIds.parse(
                   XmlResourceValues.getXmlInputFactory(),
@@ -382,21 +368,6 @@ public class ParsedAndroidData {
     return pathWalker.createParsedAndroidData();
   }
 
-  /**
-   * Parses resource symbols including "@+id/resourceName" (optional for now).
-   *
-   * @see ParsedAndroidData#from(UnvalidatedAndroidDirectories)
-   */
-  @VisibleForTesting
-  static ParsedAndroidData parseWithIds(UnvalidatedAndroidDirectories primary)
-      throws IOException, MergingException {
-    Builder builder = Builder.newBuilder().enableIdParsing();
-    final ParsedAndroidDataBuildingPathWalker pathWalker =
-        ParsedAndroidDataBuildingPathWalker.create(builder);
-    primary.walk(pathWalker);
-    return pathWalker.createParsedAndroidData();
-  }
-
   private final ImmutableSet<MergeConflict> conflicts;
   private final ImmutableMap<DataKey, DataResource> overwritingResources;
   private final ImmutableMap<DataKey, DataResource> combiningResources;
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 ec870d31b5..5bd74f96ef 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
@@ -17,6 +17,7 @@ import com.google.common.base.MoreObjects;
 import com.google.devtools.build.android.AndroidDataWritingVisitor;
 import com.google.devtools.build.android.AndroidDataWritingVisitor.StartTag;
 import com.google.devtools.build.android.AndroidResourceClassWriter;
+import com.google.devtools.build.android.DataResourceXml;
 import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.XmlResourceValue;
 import com.google.devtools.build.android.XmlResourceValues;
@@ -65,6 +66,14 @@ public class IdXmlResourceValue implements XmlResourceValue {
   @Override
   public void write(
       FullyQualifiedName key, Path source, AndroidDataWritingVisitor mergedDataWriter) {
+    if (!DataResourceXml.isInValuesFolder(source)) {
+      /* Don't write IDs that were never defined in values, into the merged values.xml, to preserve
+       * the way initializers are assigned in the R class. Depends on
+       * DataResourceXml#combineSources to accurately determine when a value is ever defined in a
+       * values file.
+       */
+      return;
+    }
     StartTag startTag =
         mergedDataWriter
             .define(key)