Fix #17462: Refactor component-updater URL overwrites #9933

jumde · 2021-08-31T22:39:56Z

Resolves brave/brave-browser#17462

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Specified here: https://github.com/brave/internal/issues/802

bridiver · 2021-09-01T17:42:15Z

test/BUILD.gn

@@ -146,6 +147,7 @@ test("brave_unit_tests") {
  ]

  defines = brave_service_key_defines
+  defines += brave_updater_defines


you shouldn't do this. brave_updater_defines and brave_service_key_defines should either go in a public config of the component updater target or go in //brave/build:compiler

bridiver · 2021-09-01T17:43:26Z

components/greaselion/browser/BUILD.gn

@@ -11,6 +11,8 @@ static_library("browser") {
    "switches.h",
  ]

+  configs += [ "//brave/common:updater_config" ]


this is not correct, this should be automatically inherited as a public config or from build:compiler as specified below https://github.com/brave/brave-core/pull/9933/files#r700430714

bridiver · 2021-09-01T17:45:27Z

components/greaselion/browser/greaselion_service_impl.cc

@@ -27,8 +27,8 @@
 #include "base/task_runner_util.h"
 #include "base/values.h"
 #include "base/version.h"
-#include "brave/components/brave_component_updater/browser/features.h"
-#include "brave/components/brave_component_updater/browser/switches.h"
+#include "brave/common/brave_features.h"


this change seems backwards, why would we move something component updater specific from components -> common? That creates a layering violation where none existed before

bridiver · 2021-09-07T21:35:50Z

chromium_src/extensions/common/extension_urls.cc

+namespace extension_urls {
+
+GURL GetDefaultWebstoreUpdateUrl() {
+  return GURL(brave_component_updater::GetUpdateURLHost());


this creates a dep on //brave/components/brave_component_updater for //extensions which does not currently exist and is not necessary to change the updater url. Subclass ChromeExtensionsClient and override GetWebstoreUpdateURL

app/brave_main_delegate.cc

bridiver · 2021-09-07T21:40:23Z

components/brave_component_updater/browser/brave_component.h

@@ -19,6 +19,8 @@ class PrefService;

 namespace brave_component_updater {

+std::string GetUpdateURLHost();


this is not a good place to put this, we should be able to have a dep for GetUpdateURLHost without including all of components/brave_component_updater/browser

iefremov · 2021-09-27T13:36:41Z

app/brave_main_delegate.cc

@@ -86,6 +89,17 @@ const char kBraveOriginTrialsPublicKey[] =

 const char kDummyUrl[] = "https://no-thanks.invalid";

+std::string GetUpdateURLHost() {


this should belong to the anonymous namespace, together we some other vars above

app/brave_main_delegate.cc

iefremov · 2021-09-27T13:42:53Z

app/brave_main_delegate.cc

@@ -165,6 +179,12 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) {
  command_line.AppendSwitch(switches::kEnableDomDistiller);
  command_line.AppendSwitch(switches::kNoPings);

+  auto update_url = GetUpdateURLHost();


I'd rather use std::string instead of auto, because it's not clear whether this is GURL or string

iefremov · 2021-09-27T13:51:52Z

browser/net/brave_common_static_redirect_network_delegate_helper.cc

-
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (!command_line.HasSwitch(brave_component_updater::kUseGoUpdateDev) &&


command_line.h feature_list.h extensions/buildflags/buildflags.h are not used now, pls check other includes

iefremov · 2021-09-27T13:53:09Z

browser/net/brave_common_static_redirect_network_delegate_helper_unittest.cc

@@ -11,7 +11,6 @@
 #include "base/command_line.h"
 #include "brave/browser/net/url_context.h"
 #include "brave/common/network_constants.h"
-#include "brave/components/brave_component_updater/browser/switches.h"


component_updater_url_constants not used?

iefremov · 2021-09-27T14:07:05Z

chromium_src/chrome/common/extensions/chrome_extensions_client.h

+
+namespace extensions {
+
+class ChromeExtensionsClient : public ChromeExtensionsClient_ChromiumImpl {


why not we add a fully-functioning BraveExtensionsClient and override only instantiation spot? @bridiver

added: https://github.com/brave/brave-core/pull/9933/files#diff-40662399037b3c7f1c12c31b0161cf86dcb72542716c8e425a9a79b8a2cad470

iefremov · 2021-09-27T14:34:21Z

chromium_src/chrome/common/extensions/chrome_extensions_client.cc

+      options, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+  for (const auto& flag : flags) {
+    if (flag.empty()) {


it can't be empty

iefremov · 2021-09-27T17:15:39Z

common/extensions/brave_extensions_client.cc

+
+void BraveExtensionsClient::InitializeWebStoreUrls(
+    base::CommandLine* command_line) {
+  webstore_update_url_ = GURL(ParseUpdateUrlHost(


I wonder whether there is a step somewhere on CI that validates that the URL is correct...

npm run network-audit verifies the URL

jumde requested review from iefremov and a team as code owners August 31, 2021 22:39

jumde self-assigned this Aug 31, 2021

jumde force-pushed the component_updater_refactor branch 3 times, most recently from dedf0a7 to 2a7d689 Compare September 1, 2021 17:00

jumde requested a review from a team as a code owner September 1, 2021 17:00

bridiver reviewed Sep 1, 2021

View reviewed changes

jumde force-pushed the component_updater_refactor branch 3 times, most recently from aa65ca2 to 923a127 Compare September 2, 2021 16:19

jumde requested a review from a team as a code owner September 2, 2021 16:19

jumde force-pushed the component_updater_refactor branch from 923a127 to 85ffaeb Compare September 2, 2021 18:07

jumde requested a review from a team as a code owner September 2, 2021 18:07

jumde force-pushed the component_updater_refactor branch 3 times, most recently from 83b9167 to 9cd313f Compare September 3, 2021 02:21

jumde added the CI/run-network-audit Run network-audit label Sep 3, 2021

jumde requested a review from bridiver September 3, 2021 02:24

jumde changed the title ~~[WIP] Fix #17462: Refactor component-updater URL overwrites~~ Fix #17462: Refactor component-updater URL overwrites Sep 3, 2021

jumde force-pushed the component_updater_refactor branch 3 times, most recently from 7e7c71f to 4cca4ed Compare September 3, 2021 19:07

bridiver reviewed Sep 7, 2021

View reviewed changes

app/brave_main_delegate.cc Show resolved Hide resolved

bridiver reviewed Sep 7, 2021

View reviewed changes

jumde force-pushed the component_updater_refactor branch 2 times, most recently from 292f826 to 3163090 Compare September 9, 2021 17:57

jumde force-pushed the component_updater_refactor branch from 3163090 to 61fcb3c Compare September 14, 2021 21:25

bridiver approved these changes Sep 15, 2021

View reviewed changes

jumde force-pushed the component_updater_refactor branch 5 times, most recently from bc61f08 to 804785b Compare September 24, 2021 20:58

iefremov reviewed Sep 27, 2021

View reviewed changes

app/brave_main_delegate.cc Show resolved Hide resolved

iefremov reviewed Sep 27, 2021

View reviewed changes

jumde requested a review from iefremov September 27, 2021 17:16

iefremov approved these changes Sep 27, 2021

View reviewed changes

jumde force-pushed the component_updater_refactor branch 2 times, most recently from d729bc4 to 76d21cd Compare September 27, 2021 17:57

Fix #17462 - Refactor component-updater URL overwrites

b3802c6

jumde force-pushed the component_updater_refactor branch from 76d21cd to b3802c6 Compare September 27, 2021 21:39

jumde merged commit f69b13e into master Sep 28, 2021

jumde deleted the component_updater_refactor branch September 28, 2021 14:55

jumde added this to the 1.32.x - Nightly milestone Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #17462: Refactor component-updater URL overwrites #9933

Fix #17462: Refactor component-updater URL overwrites #9933

jumde commented Aug 31, 2021 •

edited

Loading

bridiver Sep 1, 2021

jumde Sep 2, 2021

bridiver Sep 1, 2021

jumde Sep 2, 2021

bridiver Sep 1, 2021

jumde Sep 2, 2021

bridiver Sep 7, 2021

bridiver Sep 7, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

iefremov Sep 27, 2021

jumde Sep 27, 2021

		@@ -19,6 +19,8 @@ class PrefService;

		namespace brave_component_updater {

		std::string GetUpdateURLHost();

		@@ -86,6 +89,17 @@ const char kBraveOriginTrialsPublicKey[] =

		const char kDummyUrl[] = "https://no-thanks.invalid";

		std::string GetUpdateURLHost() {


		namespace extensions {

		class ChromeExtensionsClient : public ChromeExtensionsClient_ChromiumImpl {

Fix #17462: Refactor component-updater URL overwrites #9933

Fix #17462: Refactor component-updater URL overwrites #9933

Conversation

jumde commented Aug 31, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumde commented Aug 31, 2021 •

edited

Loading