[Sheriff] Revert "Implement an ExternalConstants provider that reads from a base::Value."

This reverts commit 669044bf.

Reason for revert: ExternalConstantsOverriderTest.* tests were failing on many mac builders after adding them.
example: https://ci.chromium.org/p/chromium/builders/ci/Mac10.13%20Tests/33860
Original change's description:
> Implement an ExternalConstants provider that reads from a base::Value.
>
> This is going to get populated with a JSONFileDeserializer.
>
> Bug: 1154901
> Change-Id: I5285b3b24a4d574c311addd44e34572b9e863c8b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2598004
> Commit-Queue: Adam Norberg <norberg@google.com>
> Reviewed-by: Sorin Jianu <sorin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#843868}

TBR=sorin@chromium.org,waffles@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,milagreen@chromium.org,norberg@google.com

Change-Id: Ieefffd98bc15542b8758a70c5590cdf90d3acc34
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1154901
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632665Reviewed-by: Anatoliy Potapchuk <apotapchuk@chromium.org>
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843990}

chrome/updater/BUILD.gn

@@ -103,8 +103,6 @@ if (is_win || is_mac) {
       "external_constants.cc",
       "external_constants.h",
       "external_constants_impl.h",
-      "external_constants_override.cc",
-      "external_constants_override.h",
       "installer.cc",
       "installer.h",
       "lib_util.h",
@@ -319,7 +317,6 @@ if (is_win || is_mac) {
     sources = [
       "app/app_server_unittest.cc",
       "enum_traits_unittest.cc",
-      "external_constants_override_unittest.cc",
       "external_constants_unittest.cc",
       "external_constants_unittest.h",
       "lib_util_unittest.cc",

chrome/updater/constants.cc

@@ -50,9 +50,6 @@ const char kDevOverrideKeyUrl[] = "url";
 const char kDevOverrideKeyUseCUP[] = "use_cup";
 const char kDevOverrideKeyInitialDelay[] = "initial_delay";
 
-// Developer override file name, relative to app data directory.
-const char kDevOverrideFileName[] = "overrides.json";
-
 // Policy Management constants.
 const char kProxyModeDirect[] = "direct";
 const char kProxyModeAutoDetect[] = "auto_detect";

chrome/updater/constants.h

@@ -128,9 +128,6 @@ extern const char kDevOverrideKeyUrl[];
 extern const char kDevOverrideKeyUseCUP[];
 extern const char kDevOverrideKeyInitialDelay[];
 
-// File name of developer overrides file.
-extern const char kDevOverrideFileName[];
-
 // Timing constants.
 #if defined(OS_WIN)
 // How long to wait for an application installer (such as

chrome/updater/external_constants_override.cc

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <memory>
-#include <string>
-#include <utility>
-#include <vector>
-
-#include "base/containers/flat_map.h"
-#include "base/files/file_path.h"
-#include "base/json/json_file_value_serializer.h"
-#include "base/json/json_reader.h"
-#include "base/logging.h"
-#include "base/notreached.h"
-#include "base/values.h"
-#include "build/build_config.h"
-#include "chrome/updater/constants.h"
-#include "chrome/updater/external_constants.h"
-#include "chrome/updater/external_constants_override.h"
-#include "chrome/updater/updater_branding.h"
-#include "chrome/updater/updater_version.h"
-#include "url/gurl.h"
-
-#if defined(OS_MAC)
-#include "base/mac/foundation_util.h"
-#elif defined(OS_WIN)
-#include "base/path_service.h"
-#endif
-
-namespace {
-
-// Calculates application data directory path. Requires PathService to be
-// ready. If it is not, this returns an empty FilePath.
-base::FilePath GetDataDirPath() {
-  // TODO(crbug/1096654): Support machine-wide installs.
-#if defined(OS_MAC)
-  return base::mac::GetUserLibraryPath()
-      .AppendASCII("Application Support")
-      .AppendASCII(COMPANY_SHORTNAME_STRING)
-      .AppendASCII(PRODUCT_FULLNAME_STRING);
-#elif defined(OS_WIN)
-  base::FilePath app_data_dir;
-  if (!base::PathService::Get(base::DIR_LOCAL_APP_DATA, &app_data_dir)) {
-    LOG(ERROR) << "Local app data dir not defined in PathService.";
-    return {};
-  }
-  return app_data_dir.AppendASCII(COMPANY_SHORTNAME_STRING)
-      .AppendASCII(PRODUCT_FULLNAME_STRING);
-#else
-  LOG(FATAL) << "Unexpected OS. Don't know how to get application data path.";
-#endif
-}
-
-std::vector<GURL> GURLVectorFromStringList(
-    base::Value::ConstListView update_url_list) {
-  std::vector<GURL> ret;
-  ret.reserve(update_url_list.size());
-  for (const base::Value& url : update_url_list) {
-    CHECK(url.is_string()) << "Non-string Value in update URL list";
-    ret.push_back(GURL(url.GetString()));
-  }
-  return ret;
-}
-
-}  // anonymous namespace
-
-namespace updater {
-
-ExternalConstantsOverrider::ExternalConstantsOverrider(
-    base::flat_map<std::string, base::Value> override_values,
-    std::unique_ptr<ExternalConstants> next_provider)
-    : ExternalConstants(std::move(next_provider)),
-      override_values_(std::move(override_values)) {}
-
-ExternalConstantsOverrider::~ExternalConstantsOverrider() = default;
-
-std::vector<GURL> ExternalConstantsOverrider::UpdateURL() const {
-  if (!override_values_.contains(kDevOverrideKeyUrl)) {
-    return next_provider_->UpdateURL();
-  }
-  const base::Value& update_url_value = override_values_.at(kDevOverrideKeyUrl);
-  switch (update_url_value.type()) {
-    case base::Value::Type::STRING:
-      return {GURL(update_url_value.GetString())};
-    case base::Value::Type::LIST:
-      return GURLVectorFromStringList(update_url_value.GetList());
-    default:
-      LOG(FATAL) << "Unexpected type of override[" << kDevOverrideKeyUrl
-                 << "]: " << base::Value::GetTypeName(update_url_value.type());
-      NOTREACHED();
-  }
-  NOTREACHED();
-  return {};
-}
-
-bool ExternalConstantsOverrider::UseCUP() const {
-  if (!override_values_.contains(kDevOverrideKeyUseCUP)) {
-    return next_provider_->UseCUP();
-  }
-  const base::Value& use_cup_value = override_values_.at(kDevOverrideKeyUseCUP);
-  CHECK(use_cup_value.is_bool())
-      << "Unexpected type of override[" << kDevOverrideKeyUseCUP
-      << "]: " << base::Value::GetTypeName(use_cup_value.type());
-
-  return use_cup_value.GetBool();
-}
-
-int ExternalConstantsOverrider::InitialDelay() const {
-  if (!override_values_.contains(kDevOverrideKeyInitialDelay)) {
-    return next_provider_->InitialDelay();
-  }
-
-  const base::Value& initial_delay_value =
-      override_values_.at(kDevOverrideKeyInitialDelay);
-  CHECK(initial_delay_value.is_int())
-      << "Unexpected type of override[" << kDevOverrideKeyInitialDelay
-      << "]: " << base::Value::GetTypeName(initial_delay_value.type());
-  return initial_delay_value.GetInt();
-}
-
-// static
-std::unique_ptr<ExternalConstantsOverrider>
-ExternalConstantsOverrider::FromDefaultJSONFile(
-    std::unique_ptr<ExternalConstants> next_provider) {
-  const base::FilePath data_dir_path = GetDataDirPath();
-  if (data_dir_path.empty()) {
-    LOG(ERROR) << "Cannot find app data path. Is PathService initialized?";
-    return nullptr;
-  }
-  const base::FilePath override_file_path =
-      data_dir_path.AppendASCII(kDevOverrideFileName);
-
-  JSONFileValueDeserializer parser(override_file_path,
-                                   base::JSON_ALLOW_TRAILING_COMMAS);
-  int error_code = 0;
-  std::string error_message;
-  std::unique_ptr<base::Value> parsed_value(
-      parser.Deserialize(&error_code, &error_message));
-  if (error_code || !parsed_value) {
-    LOG(ERROR) << "Could not parse " << override_file_path << ": error "
-               << error_code << ": " << error_message;
-    return nullptr;
-  }
-
-  if (!parsed_value->is_dict()) {
-    LOG(ERROR) << "Invalid data in " << override_file_path << ": not a dict";
-    return nullptr;
-  }
-
-  return std::make_unique<ExternalConstantsOverrider>(parsed_value->TakeDict(),
-                                                      std::move(next_provider));
-}
-
-}  // namespace updater

chrome/updater/external_constants_override.h

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_UPDATER_EXTERNAL_CONSTANTS_OVERRIDE_H_
-#define CHROME_UPDATER_EXTERNAL_CONSTANTS_OVERRIDE_H_
-
-#include <memory>
-#include <string>
-#include <vector>
-
-#include "base/containers/flat_map.h"
-#include "chrome/updater/external_constants.h"
-
-class GURL;
-
-namespace base {
-class Value;
-}
-
-namespace updater {
-
-class ExternalConstantsOverrider : public ExternalConstants {
- public:
-  ExternalConstantsOverrider(
-      base::flat_map<std::string, base::Value> override_values,
-      std::unique_ptr<ExternalConstants> next_provider);
-  ~ExternalConstantsOverrider() override;
-
-  // Loads a dictionary from overrides.json in the local application data
-  // directory to construct a ExternalConstantsOverrider.
-  //
-  // Returns nullptr (and logs appropriate errors) if the file cannot be found
-  // or cannot be parsed.
-  static std::unique_ptr<ExternalConstantsOverrider> FromDefaultJSONFile(
-      std::unique_ptr<ExternalConstants> next_provider);
-
-  // Overrides of ExternalConstants:
-  std::vector<GURL> UpdateURL() const override;
-  bool UseCUP() const override;
-  int InitialDelay() const override;
-
- private:
-  const base::flat_map<std::string, base::Value> override_values_;
-};
-
-}  // namespace updater
-
-#endif  // CHROME_UPDATER_EXTERNAL_CONSTANTS_OVERRIDE_H_

chrome/updater/external_constants_override_unittest.cc

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <utility>
-#include <vector>
-
-#include "base/values.h"
-#include "chrome/updater/constants.h"
-#include "chrome/updater/external_constants.h"
-#include "chrome/updater/external_constants_override.h"
-#include "chrome/updater/updater_branding.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
-
-namespace updater {
-
-class ExternalConstantsOverriderTest : public ::testing::Test {};
-
-TEST_F(ExternalConstantsOverriderTest, TestEmptyDictValue) {
-  ExternalConstantsOverrider overrider(
-      base::flat_map<std::string, base::Value>{}, CreateExternalConstants());
-
-  EXPECT_TRUE(overrider.UseCUP());
-
-  std::vector<GURL> urls = overrider.UpdateURL();
-  ASSERT_EQ(urls.size(), 1ul);
-  EXPECT_EQ(urls[0], GURL(UPDATE_CHECK_URL));
-  EXPECT_TRUE(urls[0].is_valid());
-
-  EXPECT_EQ(overrider.InitialDelay(), kInitialDelay);
-}
-
-TEST_F(ExternalConstantsOverriderTest, TestFullOverrides) {
-  base::Value::DictStorage overrides;
-  base::Value::ListStorage url_list;
-  url_list.push_back(base::Value("https://www.example.com"));
-  url_list.push_back(base::Value("https://www.google.com"));
-  overrides[kDevOverrideKeyUseCUP] = base::Value(false);
-  overrides[kDevOverrideKeyUrl] = base::Value(std::move(url_list));
-  overrides[kDevOverrideKeyInitialDelay] = base::Value(137);
-  ExternalConstantsOverrider overrider(std::move(overrides),
-                                       CreateExternalConstants());
-
-  EXPECT_FALSE(overrider.UseCUP());
-
-  std::vector<GURL> urls = overrider.UpdateURL();
-  ASSERT_EQ(urls.size(), 2ul);
-  EXPECT_EQ(urls[0], GURL("https://www.example.com"));
-  EXPECT_TRUE(urls[0].is_valid());
-  EXPECT_EQ(urls[1], GURL("https://www.google.com"));
-  EXPECT_TRUE(urls[1].is_valid());
-
-  EXPECT_EQ(overrider.InitialDelay(), 137);
-}
-
-TEST_F(ExternalConstantsOverriderTest, TestOverrideUnwrappedURL) {
-  base::Value::DictStorage overrides;
-  overrides[kDevOverrideKeyUrl] = base::Value("https://www.example.com");
-  ExternalConstantsOverrider overrider(std::move(overrides),
-                                       CreateExternalConstants());
-
-  std::vector<GURL> urls = overrider.UpdateURL();
-  ASSERT_EQ(urls.size(), 1ul);
-  EXPECT_EQ(urls[0], GURL("https://www.example.com"));
-  EXPECT_TRUE(urls[0].is_valid());
-
-  // Non-overridden items should fall back to defaults
-  EXPECT_TRUE(overrider.UseCUP());
-  EXPECT_EQ(overrider.InitialDelay(), kInitialDelay);
-}
-
-}  // namespace updater