Address review comments from goodov

brave · Feb 2, 2023 · d03b6b3 · d03b6b3
1 parent b41aa69
commit d03b6b3
Show file tree

Hide file tree

Showing 22 changed files with 194 additions and 135 deletions.
diff --git a/browser/BUILD.gn b/browser/BUILD.gn
@@ -23,7 +23,6 @@ if (enable_sparkle && !build_sparkle) {
 source_set("browser_process") {
   visibility = [
     "//brave/browser/*",
-    "//brave/components/brave_shields/*",
     "//chrome/browser/*",
   ]
 

diff --git a/browser/brave_browser_process.h b/browser/brave_browser_process.h
@@ -29,7 +29,6 @@ namespace brave_component_updater {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 class ExtensionWhitelistService;
 #endif
-class HttpsUpgradeExceptionsService;
 class LocalDataFilesService;
 }  // namespace brave_component_updater
 
@@ -38,6 +37,10 @@ class AdBlockService;
 class HTTPSEverywhereService;
 }  // namespace brave_shields
 
+namespace https_upgrade_exceptions {
+class HttpsUpgradeExceptionsService;
+}  // namespace https_upgrade_exceptions
+
 namespace brave_stats {
 class BraveStatsUpdater;
 }  // namespace brave_stats
@@ -59,7 +62,7 @@ class NTPBackgroundImagesService;
 namespace tor {
 class BraveTorClientUpdater;
 class BraveTorPluggableTransportUpdater;
-}
+}  // namespace tor
 
 namespace ipfs {
 class BraveIpfsClientUpdater;
@@ -83,7 +86,7 @@ class BraveBrowserProcess {
   virtual brave_component_updater::ExtensionWhitelistService*
   extension_whitelist_service() = 0;
 #endif
-  virtual brave_component_updater::HttpsUpgradeExceptionsService*
+  virtual https_upgrade_exceptions::HttpsUpgradeExceptionsService*
   https_upgrade_exceptions_service() = 0;
 #if BUILDFLAG(ENABLE_GREASELION)
   virtual greaselion::GreaselionDownloadService*

diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc
@@ -33,6 +33,7 @@
 #include "brave/components/constants/pref_names.h"
 #include "brave/components/debounce/browser/debounce_component_installer.h"
 #include "brave/components/debounce/common/features.h"
+#include "brave/components/https_upgrade_exceptions/browser/https_upgrade_exceptions_service.h"
 #include "brave/components/ntp_background_images/browser/ntp_background_images_service.h"
 #include "brave/components/p3a/brave_p3a_service.h"
 #include "brave/components/p3a/buildflags.h"
@@ -63,8 +64,6 @@
 #include "brave/components/brave_component_updater/browser/extension_whitelist_service.h"
 #endif
 
-#include "brave/components/brave_component_updater/browser/https_upgrade_exceptions_service.h"
-
 #if BUILDFLAG(ENABLE_GREASELION)
 #include "brave/components/greaselion/browser/greaselion_download_service.h"
 #endif
@@ -260,11 +259,11 @@ BraveBrowserProcessImpl::extension_whitelist_service() {
 }
 #endif
 
-brave_component_updater::HttpsUpgradeExceptionsService*
+https_upgrade_exceptions::HttpsUpgradeExceptionsService*
 BraveBrowserProcessImpl::https_upgrade_exceptions_service() {
   if (!https_upgrade_exceptions_service_) {
     https_upgrade_exceptions_service_ =
-        brave_component_updater::HttpsUpgradeExceptionsServiceFactory(
+        https_upgrade_exceptions::HttpsUpgradeExceptionsServiceFactory(
             local_data_files_service());
   }
   return https_upgrade_exceptions_service_.get();

diff --git a/browser/brave_browser_process_impl.h b/browser/brave_browser_process_impl.h
@@ -32,7 +32,6 @@ namespace brave_component_updater {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 class ExtensionWhitelistService;
 #endif
-class HttpsUpgradeExceptionsService;
 class LocalDataFilesService;
 }  // namespace brave_component_updater
 
@@ -41,6 +40,10 @@ class AdBlockService;
 class HTTPSEverywhereService;
 }  // namespace brave_shields
 
+namespace https_upgrade_exceptions {
+class HttpsUpgradeExceptionsService;
+}  // namespace https_upgrade_exceptions
+
 namespace brave_stats {
 class BraveStatsUpdater;
 }  // namespace brave_stats
@@ -62,7 +65,7 @@ class NTPBackgroundImagesService;
 namespace tor {
 class BraveTorClientUpdater;
 class BraveTorPluggableTransportUpdater;
-}
+}  // namespace tor
 
 namespace ipfs {
 class BraveIpfsClientUpdater;
@@ -97,7 +100,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess,
   brave_component_updater::ExtensionWhitelistService*
   extension_whitelist_service() override;
 #endif
-  brave_component_updater::HttpsUpgradeExceptionsService*
+  https_upgrade_exceptions::HttpsUpgradeExceptionsService*
   https_upgrade_exceptions_service() override;
 #if BUILDFLAG(ENABLE_GREASELION)
   greaselion::GreaselionDownloadService* greaselion_download_service() override;
@@ -158,7 +161,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess,
   std::unique_ptr<brave_component_updater::ExtensionWhitelistService>
       extension_whitelist_service_;
 #endif
-  std::unique_ptr<brave_component_updater::HttpsUpgradeExceptionsService>
+  std::unique_ptr<https_upgrade_exceptions::HttpsUpgradeExceptionsService>
       https_upgrade_exceptions_service_;
 #if BUILDFLAG(ENABLE_GREASELION)
   std::unique_ptr<greaselion::GreaselionDownloadService>

diff --git a/browser/brave_shields/https_upgrade_browsertest.cc b/browser/brave_shields/https_upgrade_browsertest.cc
@@ -4,8 +4,8 @@
  * You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 #include "brave/browser/brave_browser_process.h"
-#include "brave/components/brave_component_updater/browser/https_upgrade_exceptions_service.h"
 #include "brave/components/brave_shields/browser/brave_shields_util.h"
+#include "brave/components/https_upgrade_exceptions/browser/https_upgrade_exceptions_service.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
@@ -28,6 +28,31 @@
 using blink::features::kHttpsByDefault;
 using brave_shields::ControlType;
 
+namespace {
+
+enum PageResult { HTTP, HTTPS, INTERSTITIAL };
+
+struct TestStruct {
+  bool init_secure;
+  const char* domain;
+  ControlType control_type;
+  PageResult expected_result;
+};
+
+const TestStruct test_combinations[] = {
+    {false, "insecure1.test", ControlType::ALLOW, PageResult::HTTP},
+    {false, "insecure2.test", ControlType::BLOCK_THIRD_PARTY, PageResult::HTTP},
+    {false, "insecure3.test", ControlType::BLOCK, PageResult::INTERSTITIAL},
+    {false, "upgradable1.test", ControlType::ALLOW, PageResult::HTTP},
+    {false, "upgradable2.test", ControlType::BLOCK_THIRD_PARTY,
+     PageResult::HTTPS},
+    {false, "upgradable3.test", ControlType::BLOCK, PageResult::HTTPS},
+    {true, "secure1.test", ControlType::ALLOW, PageResult::HTTPS},
+    {true, "secure2.test", ControlType::BLOCK_THIRD_PARTY, PageResult::HTTPS},
+    {true, "secure3.test", ControlType::BLOCK, PageResult::HTTPS}};
+
+}  // namespace
+
 class HttpsUpgradeBrowserTest : public InProcessBrowserTest {
  public:
   HttpsUpgradeBrowserTest() = default;
@@ -106,27 +131,6 @@ class HttpsUpgradeBrowserTest : public InProcessBrowserTest {
   content::ContentMockCertVerifier mock_cert_verifier_;
 };
 
-enum PageResult { HTTP, HTTPS, INTERSTITIAL };
-
-struct TestStruct {
-  bool init_secure;
-  const char* domain;
-  ControlType control_type;
-  PageResult expected_result;
-};
-
-const TestStruct test_combinations[] = {
-    {false, "insecure1.test", ControlType::ALLOW, PageResult::HTTP},
-    {false, "insecure2.test", ControlType::BLOCK_THIRD_PARTY, PageResult::HTTP},
-    {false, "insecure3.test", ControlType::BLOCK, PageResult::INTERSTITIAL},
-    {false, "upgradable1.test", ControlType::ALLOW, PageResult::HTTP},
-    {false, "upgradable2.test", ControlType::BLOCK_THIRD_PARTY,
-     PageResult::HTTPS},
-    {false, "upgradable3.test", ControlType::BLOCK, PageResult::HTTPS},
-    {true, "secure1.test", ControlType::ALLOW, PageResult::HTTPS},
-    {true, "secure2.test", ControlType::BLOCK_THIRD_PARTY, PageResult::HTTPS},
-    {true, "secure3.test", ControlType::BLOCK, PageResult::HTTPS}};
-
 // If the user navigates to an HTTP URL for a site that supports HTTPS, the
 // navigation should end up on the HTTPS version of the URL.
 IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest, CheckUpgrades) {

diff --git a/browser/sources.gni b/browser/sources.gni
@@ -166,6 +166,7 @@ brave_chrome_browser_deps = [
   "//brave/components/decentralized_dns/content",
   "//brave/components/decentralized_dns/core",
   "//brave/components/greaselion/browser/buildflags",
+  "//brave/components/https_upgrade_exceptions/browser:browser",
   "//brave/components/ipfs/buildflags",
   "//brave/components/l10n/common",
   "//brave/components/ntp_background_images/browser",

diff --git a/browser/ui/webui/brave_shields/shields_panel_data_handler.cc b/browser/ui/webui/brave_shields/shields_panel_data_handler.cc
@@ -101,8 +101,9 @@ void ShieldsPanelDataHandler::SetCookieBlockMode(CookieBlockMode mode) {
 }
 
 void ShieldsPanelDataHandler::SetHttpsUpgradeMode(HttpsUpgradeMode mode) {
-  if (!active_shields_data_controller_)
+  if (!active_shields_data_controller_) {
     return;
+  }
 
   active_shields_data_controller_->SetHttpsUpgradeMode(mode);
 }

diff --git a/chromium_src/chrome/browser/ssl/https_only_mode_navigation_throttle.cc b/chromium_src/chrome/browser/ssl/https_only_mode_navigation_throttle.cc
@@ -7,6 +7,7 @@
 
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
+#include "brave/browser/brave_browser_process.h"
 #include "brave/components/brave_shields/browser/brave_shields_util.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -36,7 +37,8 @@ bool ShouldUpgradeToHttps(content::NavigationHandle* handle) {
   const GURL& url = handle->GetURL();
   HostContentSettingsMap* map =
       HostContentSettingsMapFactory::GetForProfile(context);
-  return brave_shields::ShouldUpgradeToHttps(map, url);
+  return brave_shields::ShouldUpgradeToHttps(
+      map, url, g_brave_browser_process->https_upgrade_exceptions_service());
 }
 
 bool IsTor(content::NavigationHandle* handle) {

diff --git a/chromium_src/chrome/browser/ssl/https_only_mode_upgrade_interceptor.cc b/chromium_src/chrome/browser/ssl/https_only_mode_upgrade_interceptor.cc
@@ -5,6 +5,7 @@
 
 #include "chrome/browser/ssl/https_only_mode_upgrade_interceptor.h"
 
+#include "brave/browser/brave_browser_process.h"
 #include "brave/components/brave_shields/browser/brave_shields_util.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -26,7 +27,8 @@ bool ShouldUpgradeToHttps(content::BrowserContext* context, const GURL& url) {
   }
   HostContentSettingsMap* map =
       HostContentSettingsMapFactory::GetForProfile(context);
-  return brave_shields::ShouldUpgradeToHttps(map, url);
+  return brave_shields::ShouldUpgradeToHttps(
+      map, url, g_brave_browser_process->https_upgrade_exceptions_service());
 }
 
 }  // namespace

diff --git a/common/BUILD.gn b/common/BUILD.gn
@@ -78,6 +78,7 @@ source_set("common") {
 
     deps += [
       "//brave/components/brave_component_updater/browser",
+      "//brave/components/https_upgrade_exceptions/browser",
       "//extensions/common:common_constants",
     ]
 

diff --git a/components/brave_component_updater/browser/BUILD.gn b/components/brave_component_updater/browser/BUILD.gn
@@ -15,8 +15,6 @@ static_library("browser") {
     "dat_file_util.h",
     "features.cc",
     "features.h",
-    "https_upgrade_exceptions_service.cc",
-    "https_upgrade_exceptions_service.h",
     "local_data_files_observer.cc",
     "local_data_files_observer.h",
     "local_data_files_service.cc",

diff --git a/components/brave_component_updater/browser/https_upgrade_exceptions_service.cc b/components/brave_component_updater/browser/https_upgrade_exceptions_service.cc
diff --git a/components/brave_shields/browser/BUILD.gn b/components/brave_shields/browser/BUILD.gn
@@ -68,7 +68,6 @@ if (!is_ios) {
 
     deps = [
       "//base",
-      "//brave/browser:browser_process",
       "//brave/components/adblock_rust_ffi",
       "//brave/components/brave_component_updater/browser",
       "//brave/components/brave_shields/common",
@@ -77,6 +76,7 @@ if (!is_ios) {
       "//brave/components/content_settings/core/common",
       "//brave/components/debounce/common",
       "//brave/components/ephemeral_storage",
+      "//brave/components/https_upgrade_exceptions/browser:browser",
       "//brave/components/l10n/common",
       "//brave/components/p3a",
       "//brave/components/p3a_utils",

diff --git a/components/brave_shields/browser/DEPS b/components/brave_shields/browser/DEPS
@@ -1,6 +1,5 @@
 include_rules = [
   "+brave/components/constants",
-  "+brave/browser",
   "+components/content_settings/core",
   "+content/public/browser",
   "+content/public/common",