apply code review recs

ampproject · Jun 27, 2016 · 23181c7 · 23181c7
1 parent 8382726
commit 23181c7
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 19 deletions.
diff --git a/build-system/runner/dist/runner.jar b/build-system/runner/dist/runner.jar
diff --git a/build-system/runner/src/org/ampproject/AmpCommandLineRunner.java b/build-system/runner/src/org/ampproject/AmpCommandLineRunner.java
@@ -36,7 +36,7 @@ public class AmpCommandLineRunner extends CommandLineRunner {
    */
   private boolean typecheck_only = false;
 
-  private boolean production_env = true;
+  private boolean is_production_env = true;
 
   /**
    * List of string suffixes to eliminate from the AST.
@@ -54,7 +54,7 @@ protected AmpCommandLineRunner(String[] args) {
     }
     CompilerOptions options = super.createOptions();
     options.setCollapseProperties(true);
-    AmpPass ampPass = new AmpPass(getCompiler(), production_env, suffixTypes);
+    AmpPass ampPass = new AmpPass(getCompiler(), is_production_env, suffixTypes);
     options.addCustomPass(CustomPassExecutionTime.BEFORE_OPTIMIZATIONS, ampPass);
     options.setDevirtualizePrototypeMethods(true);
     options.setExtractPrototypeMemberDeclarations(true);
@@ -95,19 +95,17 @@ protected void setTypeCheckOnly(boolean value) {
   }
 
   protected void setProductionFlag(boolean value) {
-    production_env = value;
+    is_production_env = value;
   }
 
   public static void main(String[] args) {
     AmpCommandLineRunner runner = new AmpCommandLineRunner(args);
 
     // Scan for TYPECHECK_ONLY string which we pass in as a --define
     for (String arg : args) {
-      System.out.println(arg);
       if (arg.contains("--define=TYPECHECK_ONLY=true")) {
         runner.setTypeCheckOnly(true);
-      }
-      if (arg.contains("--define=FORTESTING=true")) {
+      } else if (arg.contains("--define=FORTESTING=true")) {
         runner.setProductionFlag(false);
       }
     }

diff --git a/build-system/runner/src/org/ampproject/AmpPass.java b/build-system/runner/src/org/ampproject/AmpPass.java
@@ -81,11 +81,6 @@ public AmpPass(AbstractCompiler compiler, boolean isProd, Set<String> stripTypeS
    * Predicate for any <code>fnQualifiedName</code>.<code>props</code> call.
    * example:
    *   isFunctionInvokeAndPropAccess(n, "getMode", "test"); // matches `getMode().test`
-   *
-   * @param n
-   * @param fnQualifiedName
-   * @param props
-   * @return
    */
   private boolean isFunctionInvokeAndPropAccess(Node n, String fnQualifiedName, Set<String> props) {
     // mode.getMode().localDev
@@ -111,7 +106,6 @@ private boolean isFunctionInvokeAndPropAccess(Node n, String fnQualifiedName, Se
          String name = maybeProp.getString();
          for (String prop : props) {
            if (prop == name) {
-             System.out.println(qualifiedName);
              return true;
            }
          }

diff --git a/build-system/runner/test/org/ampproject/AmpPassTest.java b/build-system/runner/test/org/ampproject/AmpPassTest.java
@@ -17,7 +17,7 @@ public class AmpPassTest extends Es6CompilerTestCase {
       "dev.fine");
 
   @Override protected CompilerPass getProcessor(Compiler compiler) {
-    return new AmpPass(compiler, true, suffixTypes);
+    return new AmpPass(compiler, /* isProd */ true, suffixTypes);
   }
 
   @Override protected int getNumRepetitions() {
@@ -189,4 +189,41 @@ public void testGetModeMinifiedPropertyReplacement() throws Exception {
              "  }",
             "})()"));
   }
+
+  public void testGetModePreserve() throws Exception {
+    test(
+        LINE_JOINER.join(
+             "(function() {",
+             "function getMode() { return { minified: false } }",
+             "var $mode = { getMode: getMode };",
+             "  if ($mode.getMode()) {",
+             "    console.log('hello world');",
+             "  }",
+            "})()"),
+        LINE_JOINER.join(
+             "(function() {",
+             "function getMode() { return { minified: false }; }",
+             "var $mode = { getMode: getMode };",
+             "  if ($mode.getMode()) {",
+             "    console.log('hello world');",
+             "  }",
+            "})()"));
+    test(
+        LINE_JOINER.join(
+             "(function() {",
+             "function getMode() { return { otherProp: true } }",
+             "var $mode = { getMode: getMode };",
+             "  if ($mode.getMode().otherProp) {",
+             "    console.log('hello world');",
+             "  }",
+            "})()"),
+        LINE_JOINER.join(
+             "(function() {",
+             "function getMode() { return { otherProp: true }; }",
+             "var $mode = { getMode: getMode };",
+             "  if ($mode.getMode().otherProp) {",
+             "    console.log('hello world');",
+             "  }",
+            "})()"));
+  }
 }
diff --git a/build-system/runner/test/org/ampproject/AmpPassTestEnvTest.java b/build-system/runner/test/org/ampproject/AmpPassTestEnvTest.java
@@ -15,7 +15,7 @@ public class AmpPassTestEnvTest extends Es6CompilerTestCase {
   ImmutableSet<String> suffixTypes = ImmutableSet.of();
 
   @Override protected CompilerPass getProcessor(Compiler compiler) {
-    return new AmpPass(compiler, false, suffixTypes);
+    return new AmpPass(compiler, /* isProd */ false, suffixTypes);
   }
 
   @Override protected int getNumRepetitions() {

diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -63,6 +63,15 @@ var forbiddenTerms = {
       'validator/validator.js',
     ]
   },
+  // Match `getMode` that is not followed by a "()." and is assigned
+  // as a variable.
+  '(?:var|let|const).*?getMode(?!\\(\\)\\.)': {
+    message: 'Do not re-alias getMode or its return so it can be DCE\'d.' +
+        ' Use explicitly like "getMode().localDev" instead.',
+    whitelist: [
+      'dist.3p/current/integration.js'
+    ]
+  },
   'iframePing': {
     message: 'This is only available in vendor config for ' +
         'temporary workarounds.',

diff --git a/src/log.js b/src/log.js
@@ -87,15 +87,13 @@ export class Log {
    * @private
    */
   calcLevel_() {
-    const mode = getMode();
-
     // No console - can't enable logging.
     if (!this.win.console || !this.win.console.log) {
       return LogLevel.OFF;
     }
 
     // Logging has been explicitly disabled.
-    if (mode.log == '0') {
+    if (getMode().log == '0') {
       return LogLevel.OFF;
     }
 
@@ -105,12 +103,12 @@ export class Log {
     }
 
     // LocalDev by default allows INFO level, unless overriden by `#log`.
-    if (getMode().localDev && !mode.log) {
+    if (getMode().localDev && !getMode().log) {
       return LogLevel.INFO;
     }
 
     // Delegate to the specific resolver.
-    return this.levelFunc_(mode);
+    return this.levelFunc_(getMode());
   }
 
   /**