Replace CustomizeBrowserOptions() with CustomizeOptions() step 2

We generalize CustomizeBrowserOptions() to be able to customize all options.
New function will be call CustomizeOptions(). We do it in 3 steps:
1) Add CustomizeOptions() method to Benchmark.
2) Overload it in PerfBenchmark, instead of CustomizeBrowserOptions().
3) Remove CustomizeBrowserOptions() method from Benchmark.

This is step 2.

Bug: 801528
Change-Id: Ia1db40a04d5fddf1c634074bbd8f92ba4ccfc6f7
Reviewed-on: https://chromium-review.googlesource.com/c/1479965
Commit-Queue: Mikhail Khokhlov <khokhlov@google.com>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635599}

tools/perf/benchmarks/benchmark_unittest.py

@@ -68,7 +68,7 @@ class TestBenchmarkNamingMobile(unittest.TestCase):
                             str(disabled_tags), str(enabled_tags)]))
 
 
-class TestNoOverrideCustomizeBrowserOptions(unittest.TestCase):
+class TestNoOverrideCustomizeOptions(unittest.TestCase):
 
   def runTest(self):
     all_benchmarks = _GetAllPerfBenchmarks()
@@ -78,9 +78,9 @@ class TestNoOverrideCustomizeBrowserOptions(unittest.TestCase):
                         'Benchmark %s needs to subclass from PerfBenchmark'
                         % benchmark.Name())
       self.assertEquals(
-          benchmark.CustomizeBrowserOptions,
-          perf_benchmark.PerfBenchmark.CustomizeBrowserOptions,
-          'Benchmark %s should not override CustomizeBrowserOptions' %
+          benchmark.CustomizeOptions,
+          perf_benchmark.PerfBenchmark.CustomizeOptions,
+          'Benchmark %s should not override CustomizeOptions' %
           benchmark.Name())
 
 

tools/perf/contrib/leak_detection/leak_detection.py

@@ -24,7 +24,7 @@ class _LeakDetectionBase(perf_benchmark.PerfBenchmark):
     tbm_options.AddTimelineBasedMetric('leakDetectionMetric')
     return tbm_options
 
-  def CustomizeBrowserOptions(self, options):
+  def SetExtraBrowserOptions(self, options):
     options.AppendExtraBrowserArgs('--js-flags=--expose-gc')
 
 

tools/perf/core/perf_benchmark.py

@@ -76,13 +76,15 @@ class PerfBenchmark(benchmark.Benchmark):
     """To be overridden by perf benchmarks."""
     pass
 
-  def CustomizeBrowserOptions(self, options):
+  def CustomizeOptions(self, finder_options):
     # Subclass of PerfBenchmark should override  SetExtraBrowserOptions to add
-    # more browser options rather than overriding CustomizeBrowserOptions.
-    super(PerfBenchmark, self).CustomizeBrowserOptions(options)
+    # more browser options rather than overriding CustomizeOptions.
+    super(PerfBenchmark, self).CustomizeOptions(finder_options)
+
+    browser_options = finder_options.browser_options
 
     # Enable taking screen shot on failed pages for all perf benchmarks.
-    options.take_screenshot_for_failed_page = True
+    browser_options.take_screenshot_for_failed_page = True
 
     # The current field trial config is used for an older build in the case of
     # reference. This is a problem because we are then subjecting older builds
@@ -91,33 +93,35 @@ class PerfBenchmark(benchmark.Benchmark):
     #
     # The same logic applies to the ad filtering ruleset, which could be in a
     # binary format that an older build does not expect.
-    if options.browser_type != 'reference' and ('no-field-trials' not in
-        options.compatibility_mode):
-      variations = self._GetVariationsBrowserArgs(options.finder_options)
-      options.AppendExtraBrowserArgs(variations)
+    if (browser_options.browser_type != 'reference' and
+        'no-field-trials' not in browser_options.compatibility_mode):
+      variations = self._GetVariationsBrowserArgs(finder_options)
+      browser_options.AppendExtraBrowserArgs(variations)
 
-      options.profile_files_to_copy.extend(
-          GetAdTaggingProfileFiles(self._GetOutDirectoryEstimate(options)))
+      browser_options.profile_files_to_copy.extend(
+          GetAdTaggingProfileFiles(
+              self._GetOutDirectoryEstimate(finder_options)))
 
     # A non-sandboxed, 15-seconds-delayed gpu process is currently running in
     # the browser to collect gpu info. A command line switch is added here to
     # skip this gpu process for all perf tests to prevent any interference
     # with the test results.
-    options.AppendExtraBrowserArgs(
+    browser_options.AppendExtraBrowserArgs(
         '--disable-gpu-process-for-dx12-vulkan-info-collection')
 
     # TODO(crbug.com/881469): remove this once Webview support surface
     # synchronization and viz.
-    if options.browser_type and 'android-webview' in options.browser_type:
-      options.AppendExtraBrowserArgs(
+    if (browser_options.browser_type and
+        'android-webview' in browser_options.browser_type):
+      browser_options.AppendExtraBrowserArgs(
           '--disable-features=SurfaceSynchronization,VizDisplayCompositor')
 
     # Switch Chrome to use Perfetto instead of TraceLog as the tracing backend,
     # needed until the feature gets turned on by default everywhere.
-    if options.browser_type != 'reference':
-      options.AppendExtraBrowserArgs('--enable-perfetto')
+    if browser_options.browser_type != 'reference':
+      browser_options.AppendExtraBrowserArgs('--enable-perfetto')
 
-    self.SetExtraBrowserOptions(options)
+    self.SetExtraBrowserOptions(browser_options)
 
   @staticmethod
   def FixupTargetOS(target_os):
@@ -157,7 +161,7 @@ class PerfBenchmark(benchmark.Benchmark):
     return (p for p in possible_directories
             if os.path.basename(p).lower() == browser_type)
 
-  def _GetOutDirectoryEstimate(self, options):
+  def _GetOutDirectoryEstimate(self, finder_options):
     """Gets an estimate of the output directory for this build.
 
     Note that as an estimate, this may be incorrect. Callers should be aware of
@@ -165,12 +169,11 @@ class PerfBenchmark(benchmark.Benchmark):
     incorrect directory, nothing should critically break.
 
     """
-    finder_options = options.finder_options
     if finder_options.chromium_output_dir is not None:
       return finder_options.chromium_output_dir
 
     possible_directories = self._GetPossibleBuildDirectories(
-        finder_options.chrome_root, options.browser_type)
+        finder_options.chrome_root, finder_options.browser_options.browser_type)
     return next((p for p in possible_directories if os.path.exists(p)), None)
 
   @staticmethod

tools/perf/core/perf_benchmark_unittest.py

@@ -94,7 +94,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     with open(fieldtrial_path, "w") as f:
       f.write(testing_config)
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
 
     expected_args = [
       "--enable-features=Feature1<TestStudy,Feature2<TestStudy",
@@ -110,7 +110,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     options = options_for_unittests.GetCopy()
     options.chrome_root = self._output_dir
     options.browser_options.browser_type = 'reference'
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
 
     for arg in expected_args:
       self.assertNotIn(arg, options.browser_options.extra_browser_args)
@@ -120,7 +120,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     options = options_for_unittests.GetCopy()
     options.chrome_root = self._output_dir
     options.browser_options.compatibility_mode = ['no-field-trials']
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
 
     for arg in expected_args:
       self.assertNotIn(arg, options.browser_options.extra_browser_args)
@@ -132,7 +132,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     # Set the chrome root to avoid using a ruleset from an existing "Release"
     # out dir.
     options.chrome_root = self._output_dir
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, False)
 
   def testAdTaggingRulesetReference(self):
@@ -147,7 +147,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     # affecting other tests. See http://crbug.com/843994.
     options.chromium_output_dir = self._output_dir
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, False)
 
   def testAdTaggingRuleset(self):
@@ -161,7 +161,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     # affecting other tests. See http://crbug.com/843994.
     options.chromium_output_dir = self._output_dir
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, True)
 
   def testAdTaggingRulesetNoExplicitOutDir(self):
@@ -172,7 +172,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     options.chrome_root = self._chrome_root
     options.browser_options.browser_type = "release"
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, True)
 
   def testAdTaggingRulesetNoExplicitOutDirAndroidChromium(self):
@@ -185,7 +185,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     # android-chromium is special cased to search for anything.
     options.browser_options.browser_type = "android-chromium"
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, True)
 
   def testAdTaggingRulesetOutputDirNotFound(self):
@@ -199,7 +199,7 @@ class PerfBenchmarkTest(unittest.TestCase):
     options.chrome_root = self._chrome_root
     options.browser_options.browser_type = "release"
 
-    benchmark.CustomizeBrowserOptions(options.browser_options)
+    benchmark.CustomizeOptions(options)
     self._ExpectAdTaggingProfileFiles(options.browser_options, False)
 
   def testAdTaggingRulesetInvalidJson(self):
@@ -217,4 +217,4 @@ class PerfBenchmarkTest(unittest.TestCase):
 
     # Should fail due to invalid JSON.
     with self.assertRaises(ValueError):
-      benchmark.CustomizeBrowserOptions(options.browser_options)
+      benchmark.CustomizeOptions(options)